Skip to content

Commit 4b4b15d

Browse files
Fix pnpm lock/workspace readers missing a BOM (#903, #904, #905) (#909)
* Start fix for #903, #904, #905 Assisted-by: Claude Code:claude-opus-5-5 * Read BOM-prefixed pnpm locks and workspace files A pnpm-lock.yaml or pnpm-workspace.yaml saved with a UTF-8 BOM (as Windows editors do with "UTF-8 with signature") is read normally by pnpm, but socket-patch's pnpm readers matched column-0 text and missed the first line: - hosted scan skipped the trustLockfile auto-config, so frozen pnpm 11/12 installs failed; rollback, remove and list then refused the lock the scan had just pinned, and vendored mode refused it as having no lockfileVersion (#903) - a BOM first key in pnpm-workspace.yaml was missed, so hosted and vendored appended a duplicate trustLockfile / overrides key that pnpm refuses to parse (#904) Add formats::text with one split_bom/strip_bom pair, replace the four named copies with it, and make the pnpm lock sniffs and the workspace key reader skip one leading BOM. Splices keep the original line, so the BOM stays byte-exact. lockfileDir is now read through the same workspace key reader (#905). Assisted-by: Claude Code:claude-opus-5-5 * Test hosted pnpm scan/rollback on BOM files Covers the #903 and #904 user flows end to end: a BOM lock gets the trustLockfile auto-config and rolls back byte-exact, and a BOM first trustLockfile key in pnpm-workspace.yaml is respected instead of duplicated. Assisted-by: Claude Code:claude-opus-5-5 * Route Gradle digests through utils::digest main has failed socket-patch-core's lib tests since Gradle support (#646) and the digest helpers (#865) both landed. The guard test production_digests_go_through_the_helpers flags three files #646 added that still hash inline: crawlers/gradle_cache.rs, patch/jvm_jar.rs and patch/sidecars/maven.rs. That breaks test, test-release and coverage on every open PR. Each inline sha1/sha256 call now goes through sha1_hex_of or sha256_hex_of, which compute the same lowercase hex. Behaviour is unchanged. Assisted-by: Claude Code:claude-opus-5-5 (cherry picked from commit 659ac2c) --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 04885c3 commit 4b4b15d

23 files changed

Lines changed: 455 additions & 107 deletions

File tree

‎crates/socket-patch-cli/tests/in_process_redirect_pnpm.rs‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,78 @@ async fn hosted_trust_edit_reads_the_workspace_yaml_shape() {
405405
}
406406
}
407407

408+
/// #903 / #904: a `pnpm-lock.yaml` and `pnpm-workspace.yaml` saved with a
409+
/// UTF-8 BOM read like their plain twins. The BOM lock gets the
410+
/// `trustLockfile: true` auto-config (it used to read as unversioned and
411+
/// skip it), `rollback` unwinds the pin it just wrote (it used to refuse the
412+
/// lock as "not a pnpm lockfile") byte-exact, BOM included, and a BOM first
413+
/// `trustLockfile: false` key is the user's explicit opt-out, not a missing
414+
/// key a duplicate is appended after.
415+
#[tokio::test]
416+
#[serial]
417+
async fn hosted_bom_lock_and_workspace_read_like_their_plain_twins() {
418+
let server = MockServer::start().await;
419+
mock_discovery(&server).await;
420+
mock_reference(&server).await;
421+
422+
// A BOM lock, no workspace file: the trust scaffold is created and the
423+
// rollback restores the lock byte for byte.
424+
let tmp = tempfile::tempdir().unwrap();
425+
write_pnpm_project(tmp.path());
426+
let lock_path = tmp.path().join("pnpm-lock.yaml");
427+
let pristine = format!("\u{feff}{}", std::fs::read_to_string(&lock_path).unwrap());
428+
std::fs::write(&lock_path, &pristine).unwrap();
429+
430+
let code = run(hosted_args(tmp.path(), server.uri())).await;
431+
assert_eq!(code, 0, "scan --mode hosted should succeed on a BOM lock");
432+
let lock = std::fs::read_to_string(&lock_path).unwrap();
433+
assert!(lock.starts_with("\u{feff}lockfileVersion:"), "{lock}");
434+
assert!(lock.contains(HOSTED_URL), "the BOM lock is redirected: {lock}");
435+
let ws_path = tmp.path().join("pnpm-workspace.yaml");
436+
assert_eq!(
437+
std::fs::read_to_string(&ws_path).ok().as_deref(),
438+
Some("packages:\n - '.'\ntrustLockfile: true\n"),
439+
"a BOM v9 lock gets the trustLockfile auto-config"
440+
);
441+
442+
let code = rollback_hosted(tmp.path(), &server).await;
443+
assert_eq!(code, 0, "rollback must unwind the pin on a BOM lock");
444+
assert_eq!(
445+
std::fs::read_to_string(&lock_path).unwrap(),
446+
pristine,
447+
"rollback restores the BOM lock byte for byte"
448+
);
449+
assert!(!ws_path.exists(), "the auto-created workspace file goes too");
450+
451+
// A BOM workspace file whose first key is the user's opt-out: left
452+
// byte-identical (no duplicate `trustLockfile`), lock still redirected.
453+
// One whose first key is something else gains the key once, BOM kept.
454+
for (user_ws, want) in [
455+
("\u{feff}trustLockfile: false\npackages:\n - '.'\n", None),
456+
("\u{feff}trustLockfile: true\npackages:\n - '.'\n", None),
457+
(
458+
"\u{feff}packages:\n - '.'\n",
459+
Some("\u{feff}packages:\n - '.'\ntrustLockfile: true\n"),
460+
),
461+
] {
462+
let tmp = tempfile::tempdir().unwrap();
463+
write_pnpm_project(tmp.path());
464+
std::fs::write(tmp.path().join("pnpm-workspace.yaml"), user_ws).unwrap();
465+
466+
let code = run(hosted_args(tmp.path(), server.uri())).await;
467+
assert_eq!(code, 0, "scan --mode hosted should succeed for {user_ws:?}");
468+
assert!(
469+
std::fs::read_to_string(tmp.path().join("pnpm-lock.yaml"))
470+
.unwrap()
471+
.contains(HOSTED_URL),
472+
"the lock is still redirected for {user_ws:?}"
473+
);
474+
let ws = std::fs::read_to_string(tmp.path().join("pnpm-workspace.yaml")).unwrap();
475+
assert_eq!(ws, want.unwrap_or(user_ws), "workspace file for {user_ws:?}");
476+
assert_eq!(ws.matches("trustLockfile").count(), 1, "{ws:?}");
477+
}
478+
}
479+
408480
/// `--dry-run` previews: NOTHING lands on disk — no lock rewrite, no
409481
/// pnpm-workspace.yaml, no ledger — while the envelope still reports both
410482
/// files as would-be-rewritten (`dryRun: true`).

‎crates/socket-patch-core/src/crawlers/npm_crawler.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ fn pnpm_modules_dir_setting(start_path: &Path) -> Option<String> {
108108
.ancestors()
109109
.find_map(|dir| read(dir.join("pnpm-workspace.yaml")))
110110
.and_then(|yaml| {
111-
crate::utils::serde::strip_bom(&yaml)
111+
crate::formats::text::strip_bom(&yaml)
112112
.lines()
113113
.filter_map(crate::formats::pnpm::workspace::top_level_key)
114114
.rfind(|(key, _)| key == "modulesDir")
@@ -367,7 +367,7 @@ fn parse_package_json_identity(content: &str) -> Option<(String, String)> {
367367
// (Windows-authored packages ship them), but serde_json rejects it —
368368
// a BOM'd install would be invisible to scan and unpatchable.
369369
let pkg: PackageJsonPartial =
370-
serde_json::from_str(crate::utils::serde::strip_bom(content)).ok()?;
370+
serde_json::from_str(crate::formats::text::strip_bom(content)).ok()?;
371371
let name = pkg.name?;
372372
let version = pkg.version?;
373373
if name.is_empty() || version.is_empty() {
@@ -591,7 +591,7 @@ const PNPM_MODULES_YAML: &str = ".modules.yaml";
591591
/// The `virtualStoreDir` value of a `.modules.yaml`: JSON on pnpm 10+,
592592
/// YAML before (a top-level `virtualStoreDir:` scalar, maybe quoted).
593593
fn parse_modules_yaml_virtual_store_dir(text: &str) -> Option<String> {
594-
let text = crate::utils::serde::strip_bom(text);
594+
let text = crate::formats::text::strip_bom(text);
595595
if let Ok(value) = serde_json::from_str::<serde_json::Value>(text) {
596596
return value
597597
.get("virtualStoreDir")?

‎crates/socket-patch-core/src/formats/mod.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,16 @@
2626
//! [`registry()`] is the one table of which project files carry a lock or
2727
//! its wiring, and in which roles.
2828
29+
pub(crate) mod bun;
2930
pub mod cargo;
3031
pub mod composer;
3132
pub mod gem;
3233
pub(crate) mod maven;
3334
pub(crate) mod nuget;
3435
pub mod pnpm;
35-
pub(crate) mod bun;
3636
pub mod registry;
3737
pub mod sbt;
38+
pub mod text;
3839
pub mod yarn;
3940

4041
pub use registry::registry;
@@ -82,7 +83,11 @@ mod architecture_tests {
8283
.filter(|l| !l.trim_start().starts_with("//"))
8384
.collect::<Vec<_>>()
8485
.join("\n");
85-
let used: Vec<&str> = IMPURE.iter().copied().filter(|n| code.contains(n)).collect();
86+
let used: Vec<&str> = IMPURE
87+
.iter()
88+
.copied()
89+
.filter(|n| code.contains(n))
90+
.collect();
8691
assert!(
8792
used.is_empty(),
8893
"{}: a format model uses {used:?} — models are pure (module docs)",

‎crates/socket-patch-core/src/formats/pnpm/grammar.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77
88
use std::ops::Range;
99

10+
use crate::formats::text::strip_bom;
11+
1012
/// Early pnpm 1 writes shrinkwrapVersion 3 without a minor version and
1113
/// unconditionally drops registry tarball URLs on install. Its frozen flag
1214
/// cannot preserve this redirect (verified with pnpm 1.0.0).
1315
pub(crate) fn unsupported_early_shrinkwrap(content: &str) -> bool {
16+
let content = strip_bom(content);
1417
let version = content
1518
.lines()
1619
.find_map(|line| line.strip_prefix("shrinkwrapVersion:"));
@@ -77,9 +80,11 @@ pub(crate) fn unquote(s: &str) -> &str {
7780

7881
/// Whether `text` is a pnpm lock at all: a column-0 `lockfileVersion:`
7982
/// (pnpm >= 3) or `shrinkwrapVersion:` (pnpm 1 / 2) line — lockfile
80-
/// discovery's sniff before it reads any entry.
83+
/// discovery's sniff before it reads any entry. A leading BOM is encoding,
84+
/// not key text: pnpm reads a BOM lock like its plain twin (#903).
8185
pub(crate) fn is_pnpm_lock_text(text: &str) -> bool {
82-
text.lines()
86+
strip_bom(text)
87+
.lines()
8388
.any(|line| line.starts_with("lockfileVersion:") || line.starts_with("shrinkwrapVersion:"))
8489
}
8590

‎crates/socket-patch-core/src/formats/pnpm/mod.rs‎

Lines changed: 136 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,11 @@ pub(crate) use hosted::plan_hosted;
3535
use std::collections::HashSet;
3636

3737
use crate::constants::npm_family::PNPM_LOCK;
38+
use crate::formats::text::strip_bom;
3839
use crate::utils::digest::is_sri_pin;
3940
use crate::vendor::lock_inventory::{http_url, LockIntegrity, LockfileEntry};
4041
use crate::vendor::path::parse_vendor_path;
4142

42-
4343
// ── entry model ──
4444

4545
/// One `packages:` entry of a pnpm lock, read with the entry grammar
@@ -235,9 +235,10 @@ impl PnpmLockGrammar {
235235
}
236236

237237
/// The `lockfileVersion:` a lock head (its first five lines) declares,
238-
/// unquoted.
238+
/// unquoted. A leading BOM is encoding, not key text (#903).
239239
fn head_lock_version(text: &str) -> Option<String> {
240-
text.lines()
240+
strip_bom(text)
241+
.lines()
241242
.take(5)
242243
.find_map(|line| line.strip_prefix("lockfileVersion:"))
243244
.map(|rest| rest.trim().trim_matches(['\'', '"']).to_string())
@@ -276,14 +277,18 @@ pub fn sniff_lock_grammar(text: &str) -> Result<PnpmLockGrammar, String> {
276277
}
277278

278279
/// The `(major, minor)` of every `lockfileVersion:` line of a lock (the
279-
/// first one decides), unquoted; a missing minor reads as 0.
280+
/// first one decides), unquoted; a missing minor reads as 0. A leading BOM
281+
/// is encoding, not key text (#903).
280282
fn lock_versions(text: &str) -> impl Iterator<Item = (Option<u32>, u32)> + '_ {
281-
text.lines().filter_map(|line| {
283+
strip_bom(text).lines().filter_map(|line| {
282284
let rest = line.strip_prefix("lockfileVersion:")?;
283285
let value = rest.trim().trim_matches(|c| c == '\'' || c == '"');
284286
let mut parts = value.split('.');
285287
let major = parts.next().and_then(|m| m.parse::<u32>().ok());
286-
let minor = parts.next().and_then(|m| m.parse::<u32>().ok()).unwrap_or(0);
288+
let minor = parts
289+
.next()
290+
.and_then(|m| m.parse::<u32>().ok())
291+
.unwrap_or(0);
287292
Some((major, minor))
288293
})
289294
}
@@ -303,10 +308,18 @@ pub fn lock_version_major(text: &str) -> Option<u32> {
303308
/// rejects it): a `shrinkwrapVersion` lock (pnpm 1–2) or lockfileVersion
304309
/// 5.0–5.2 (pnpm 3–5). Later locks never get the `--store` note.
305310
pub fn may_need_store_flag(text: &str) -> bool {
306-
text.lines().any(|line| line.starts_with("shrinkwrapVersion:"))
311+
is_shrinkwrap_lock(text)
307312
|| lock_versions(text).any(|(major, minor)| major == Some(5) && minor <= 2)
308313
}
309314

315+
/// Whether a pnpm lock is a pnpm 1–2 `shrinkwrapVersion:` lock (a leading
316+
/// BOM skipped).
317+
pub fn is_shrinkwrap_lock(text: &str) -> bool {
318+
strip_bom(text)
319+
.lines()
320+
.any(|line| line.starts_with("shrinkwrapVersion:"))
321+
}
322+
310323
/// The lockfileVersion the v9 vendored planner splices.
311324
const V9_LOCK_VERSION: &str = "9.0";
312325

@@ -491,7 +504,9 @@ pub(crate) fn vendored_npm_uuids(text: &str) -> HashSet<String> {
491504
if !in_section {
492505
continue;
493506
}
494-
if let Some(uuid) = lines::parse_key_line(line, 2).and_then(|(key, _, _)| vendored_npm_uuid(key)) {
507+
if let Some(uuid) =
508+
lines::parse_key_line(line, 2).and_then(|(key, _, _)| vendored_npm_uuid(key))
509+
{
495510
out.insert(uuid);
496511
}
497512
}
@@ -508,17 +523,52 @@ mod tests {
508523
fn resolves_reads_every_key_generation_boundary_anchored() {
509524
let lock = |keys: &str| format!("lockfileVersion: '9.0'\n\npackages:\n\n{keys}");
510525
let yes = [
511-
(" left-pad@1.3.0:\n resolution: {integrity: sha512-x}\n", "left-pad", "1.3.0"),
512-
(" /left-pad@1.3.0:\n resolution: {}\n", "left-pad", "1.3.0"),
513-
(" /left-pad/1.3.0:\n resolution: {}\n", "left-pad", "1.3.0"),
514-
(" 'left-pad@1.3.0(react@18.0.0)':\n dev: false\n", "left-pad", "1.3.0"),
515-
(" /left-pad/1.3.0_react@18.0.0:\n dev: false\n", "left-pad", "1.3.0"),
516-
(" '@scope/name@1.0.0':\n dev: false\n", "@scope/name", "1.0.0"),
517-
(" /@scope/name@1.0.0:\n dev: false\n", "@scope/name", "1.0.0"),
518-
(" /@scope/name/1.0.0:\n dev: false\n", "@scope/name", "1.0.0"),
526+
(
527+
" left-pad@1.3.0:\n resolution: {integrity: sha512-x}\n",
528+
"left-pad",
529+
"1.3.0",
530+
),
531+
(
532+
" /left-pad@1.3.0:\n resolution: {}\n",
533+
"left-pad",
534+
"1.3.0",
535+
),
536+
(
537+
" /left-pad/1.3.0:\n resolution: {}\n",
538+
"left-pad",
539+
"1.3.0",
540+
),
541+
(
542+
" 'left-pad@1.3.0(react@18.0.0)':\n dev: false\n",
543+
"left-pad",
544+
"1.3.0",
545+
),
546+
(
547+
" /left-pad/1.3.0_react@18.0.0:\n dev: false\n",
548+
"left-pad",
549+
"1.3.0",
550+
),
551+
(
552+
" '@scope/name@1.0.0':\n dev: false\n",
553+
"@scope/name",
554+
"1.0.0",
555+
),
556+
(
557+
" /@scope/name@1.0.0:\n dev: false\n",
558+
"@scope/name",
559+
"1.0.0",
560+
),
561+
(
562+
" /@scope/name/1.0.0:\n dev: false\n",
563+
"@scope/name",
564+
"1.0.0",
565+
),
519566
];
520567
for (keys, name, version) in yes {
521-
assert!(PnpmLock::parse(&lock(keys)).resolves(name, version), "{keys}");
568+
assert!(
569+
PnpmLock::parse(&lock(keys)).resolves(name, version),
570+
"{keys}"
571+
);
522572
}
523573
let no = [
524574
(" left-pad@1.3.0-beta.1:\n dev: false\n", "left-pad", "1.3.0"),
@@ -534,7 +584,10 @@ mod tests {
534584
),
535585
];
536586
for (keys, name, version) in no {
537-
assert!(!PnpmLock::parse(&lock(keys)).resolves(name, version), "{keys}");
587+
assert!(
588+
!PnpmLock::parse(&lock(keys)).resolves(name, version),
589+
"{keys}"
590+
);
538591
}
539592
// Keys outside `packages:` (importers, overrides) resolve nothing.
540593
let importers = "lockfileVersion: '9.0'\n\nimporters:\n\n left-pad@1.3.0:\n x: y\n";
@@ -557,7 +610,10 @@ mod tests {
557610
let other = "22222222-2222-4222-8222-222222222222";
558611
assert!(!PnpmLock::parse(text).vendored_in_use(other));
559612
let crlf = text.replace('\n', "\r\n");
560-
assert!(PnpmLock::parse(&crlf).vendored_in_use(UUID), "CRLF reads like LF");
613+
assert!(
614+
PnpmLock::parse(&crlf).vendored_in_use(UUID),
615+
"CRLF reads like LF"
616+
);
561617
}
562618
// An overrides declaration alone is not usage.
563619
let overrides = format!(
@@ -582,4 +638,65 @@ mod tests {
582638
);
583639
assert_eq!(PnpmLock::parse(&neighbour).wired_integrity(&rel), None);
584640
}
641+
642+
/// #903 / #905: a leading UTF-8 BOM is encoding, not content — pnpm
643+
/// reads a BOM lock like its plain twin, so every sniff here must too.
644+
/// Before the fix the BOM twin read as "not a pnpm lock", unversioned
645+
/// and unsupported while its entries still parsed.
646+
#[test]
647+
fn bom_lock_reads_like_its_plain_twin() {
648+
let v9 = "lockfileVersion: '9.0'\n\nimporters:\n\n .:\n dependencies:\n left-pad:\n specifier: 1.3.0\n version: 1.3.0\n\npackages:\n\n left-pad@1.3.0:\n resolution: {integrity: sha512-x}\n";
649+
let v6 = "lockfileVersion: '6.0'\n\ndependencies:\n left-pad:\n specifier: 1.3.0\n version: 1.3.0\n\npackages:\n\n /left-pad@1.3.0:\n resolution: {integrity: sha512-x}\n dev: false\n";
650+
let v54 = "lockfileVersion: 5.4\n\nspecifiers:\n left-pad: 1.3.0\n\ndependencies:\n left-pad: 1.3.0\n\npackages:\n\n /left-pad/1.3.0:\n resolution: {integrity: sha512-x}\n dev: false\n";
651+
let v52 = "lockfileVersion: 5.2\n\npackages:\n\n /left-pad/1.3.0:\n resolution: {integrity: sha512-x}\n";
652+
let shrinkwrap = "shrinkwrapVersion: 3\nshrinkwrapMinorVersion: 7\n\npackages:\n\n /left-pad/1.3.0:\n resolution: {integrity: sha512-x}\n";
653+
for plain in [v9, v6, v54, v52, shrinkwrap] {
654+
let bom = format!("\u{feff}{plain}");
655+
assert!(PnpmLock::parse(plain).is_pnpm_lock(), "{plain}");
656+
assert!(PnpmLock::parse(&bom).is_pnpm_lock(), "BOM twin of {plain}");
657+
assert!(is_pnpm_lock_text(&bom), "{plain}");
658+
assert_eq!(
659+
sniff_lock_grammar(&bom),
660+
sniff_lock_grammar(plain),
661+
"{plain}"
662+
);
663+
assert_eq!(
664+
lock_version_major(&bom),
665+
lock_version_major(plain),
666+
"{plain}"
667+
);
668+
assert_eq!(
669+
may_need_store_flag(&bom),
670+
may_need_store_flag(plain),
671+
"{plain}"
672+
);
673+
assert_eq!(
674+
check_v9_lock_version(&bom),
675+
check_v9_lock_version(plain),
676+
"{plain}"
677+
);
678+
let keys = |text: &str| {
679+
PnpmLock::parse(text).entries().map(|e| {
680+
e.into_iter()
681+
.map(|e| (e.name, e.version))
682+
.collect::<Vec<_>>()
683+
})
684+
};
685+
assert_eq!(keys(&bom), keys(plain), "{plain}");
686+
}
687+
assert_eq!(
688+
sniff_lock_grammar(&format!("\u{feff}{v9}")),
689+
Ok(PnpmLockGrammar::V9)
690+
);
691+
assert_eq!(lock_version_major(&format!("\u{feff}{v9}")), Some(9));
692+
assert!(may_need_store_flag(&format!("\u{feff}{v52}")));
693+
assert!(may_need_store_flag(&format!("\u{feff}{shrinkwrap}")));
694+
assert!(grammar::unsupported_early_shrinkwrap(
695+
"\u{feff}shrinkwrapVersion: 3\n"
696+
));
697+
// Exactly one BOM is encoding; a second one is content, as for pnpm.
698+
assert!(!is_pnpm_lock_text(
699+
"\u{feff}\u{feff}lockfileVersion: '9.0'\n"
700+
));
701+
}
585702
}

0 commit comments

Comments
 (0)