Fix pnpm lock/workspace readers missing a BOM (#903, #904, #905)

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

@@ -405,6 +405,78 @@ async fn hosted_trust_edit_reads_the_workspace_yaml_shape() {
     }
 }
 
+/// #903 / #904: a `pnpm-lock.yaml` and `pnpm-workspace.yaml` saved with a
+/// UTF-8 BOM read like their plain twins. The BOM lock gets the
+/// `trustLockfile: true` auto-config (it used to read as unversioned and
+/// skip it), `rollback` unwinds the pin it just wrote (it used to refuse the
+/// lock as "not a pnpm lockfile") byte-exact, BOM included, and a BOM first
+/// `trustLockfile: false` key is the user's explicit opt-out, not a missing
+/// key a duplicate is appended after.
+#[tokio::test]
+#[serial]
+async fn hosted_bom_lock_and_workspace_read_like_their_plain_twins() {
+    let server = MockServer::start().await;
+    mock_discovery(&server).await;
+    mock_reference(&server).await;
+
+    // A BOM lock, no workspace file: the trust scaffold is created and the
+    // rollback restores the lock byte for byte.
+    let tmp = tempfile::tempdir().unwrap();
+    write_pnpm_project(tmp.path());
+    let lock_path = tmp.path().join("pnpm-lock.yaml");
+    let pristine = format!("\u{feff}{}", std::fs::read_to_string(&lock_path).unwrap());
+    std::fs::write(&lock_path, &pristine).unwrap();
+
+    let code = run(hosted_args(tmp.path(), server.uri())).await;
+    assert_eq!(code, 0, "scan --mode hosted should succeed on a BOM lock");
+    let lock = std::fs::read_to_string(&lock_path).unwrap();
+    assert!(lock.starts_with("\u{feff}lockfileVersion:"), "{lock}");
+    assert!(lock.contains(HOSTED_URL), "the BOM lock is redirected: {lock}");
+    let ws_path = tmp.path().join("pnpm-workspace.yaml");
+    assert_eq!(
+        std::fs::read_to_string(&ws_path).ok().as_deref(),
+        Some("packages:\n  - '.'\ntrustLockfile: true\n"),
+        "a BOM v9 lock gets the trustLockfile auto-config"
+    );
+
+    let code = rollback_hosted(tmp.path(), &server).await;
+    assert_eq!(code, 0, "rollback must unwind the pin on a BOM lock");
+    assert_eq!(
+        std::fs::read_to_string(&lock_path).unwrap(),
+        pristine,
+        "rollback restores the BOM lock byte for byte"
+    );
+    assert!(!ws_path.exists(), "the auto-created workspace file goes too");
+
+    // A BOM workspace file whose first key is the user's opt-out: left
+    // byte-identical (no duplicate `trustLockfile`), lock still redirected.
+    // One whose first key is something else gains the key once, BOM kept.
+    for (user_ws, want) in [
+        ("\u{feff}trustLockfile: false\npackages:\n  - '.'\n", None),
+        ("\u{feff}trustLockfile: true\npackages:\n  - '.'\n", None),
+        (
+            "\u{feff}packages:\n  - '.'\n",
+            Some("\u{feff}packages:\n  - '.'\ntrustLockfile: true\n"),
+        ),
+    ] {
+        let tmp = tempfile::tempdir().unwrap();
+        write_pnpm_project(tmp.path());
+        std::fs::write(tmp.path().join("pnpm-workspace.yaml"), user_ws).unwrap();
+
+        let code = run(hosted_args(tmp.path(), server.uri())).await;
+        assert_eq!(code, 0, "scan --mode hosted should succeed for {user_ws:?}");
+        assert!(
+            std::fs::read_to_string(tmp.path().join("pnpm-lock.yaml"))
+                .unwrap()
+                .contains(HOSTED_URL),
+            "the lock is still redirected for {user_ws:?}"
+        );
+        let ws = std::fs::read_to_string(tmp.path().join("pnpm-workspace.yaml")).unwrap();
+        assert_eq!(ws, want.unwrap_or(user_ws), "workspace file for {user_ws:?}");
+        assert_eq!(ws.matches("trustLockfile").count(), 1, "{ws:?}");
+    }
+}
+
 /// `--dry-run` previews: NOTHING lands on disk — no lock rewrite, no
 /// pnpm-workspace.yaml, no ledger — while the envelope still reports both
 /// files as would-be-rewritten (`dryRun: true`).

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

@@ -108,7 +108,7 @@ fn pnpm_modules_dir_setting(start_path: &Path) -> Option<String> {
         .ancestors()
         .find_map(|dir| read(dir.join("pnpm-workspace.yaml")))
         .and_then(|yaml| {
-            crate::utils::serde::strip_bom(&yaml)
+            crate::formats::text::strip_bom(&yaml)
                 .lines()
                 .filter_map(crate::formats::pnpm::workspace::top_level_key)
                 .rfind(|(key, _)| key == "modulesDir")
@@ -367,7 +367,7 @@ fn parse_package_json_identity(content: &str) -> Option<(String, String)> {
     // (Windows-authored packages ship them), but serde_json rejects it —
     // a BOM'd install would be invisible to scan and unpatchable.
     let pkg: PackageJsonPartial =
-        serde_json::from_str(crate::utils::serde::strip_bom(content)).ok()?;
+        serde_json::from_str(crate::formats::text::strip_bom(content)).ok()?;
     let name = pkg.name?;
     let version = pkg.version?;
     if name.is_empty() || version.is_empty() {
@@ -591,7 +591,7 @@ const PNPM_MODULES_YAML: &str = ".modules.yaml";
 /// The `virtualStoreDir` value of a `.modules.yaml`: JSON on pnpm 10+,
 /// YAML before (a top-level `virtualStoreDir:` scalar, maybe quoted).
 fn parse_modules_yaml_virtual_store_dir(text: &str) -> Option<String> {
-    let text = crate::utils::serde::strip_bom(text);
+    let text = crate::formats::text::strip_bom(text);
     if let Ok(value) = serde_json::from_str::<serde_json::Value>(text) {
         return value
             .get("virtualStoreDir")?

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

@@ -26,15 +26,16 @@
 //! [`registry()`] is the one table of which project files carry a lock or
 //! its wiring, and in which roles.
 
+pub(crate) mod bun;
 pub mod cargo;
 pub mod composer;
 pub mod gem;
 pub(crate) mod maven;
 pub(crate) mod nuget;
 pub mod pnpm;
-pub(crate) mod bun;
 pub mod registry;
 pub mod sbt;
+pub mod text;
 pub mod yarn;
 
 pub use registry::registry;
@@ -82,7 +83,11 @@ mod architecture_tests {
                 .filter(|l| !l.trim_start().starts_with("//"))
                 .collect::<Vec<_>>()
                 .join("\n");
-            let used: Vec<&str> = IMPURE.iter().copied().filter(|n| code.contains(n)).collect();
+            let used: Vec<&str> = IMPURE
+                .iter()
+                .copied()
+                .filter(|n| code.contains(n))
+                .collect();
             assert!(
                 used.is_empty(),
                 "{}: a format model uses {used:?} — models are pure (module docs)",

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

@@ -7,10 +7,13 @@
 
 use std::ops::Range;
 
+use crate::formats::text::strip_bom;
+
 /// Early pnpm 1 writes shrinkwrapVersion 3 without a minor version and
 /// unconditionally drops registry tarball URLs on install. Its frozen flag
 /// cannot preserve this redirect (verified with pnpm 1.0.0).
 pub(crate) fn unsupported_early_shrinkwrap(content: &str) -> bool {
+    let content = strip_bom(content);
     let version = content
         .lines()
         .find_map(|line| line.strip_prefix("shrinkwrapVersion:"));
@@ -77,9 +80,11 @@ pub(crate) fn unquote(s: &str) -> &str {
 
 /// Whether `text` is a pnpm lock at all: a column-0 `lockfileVersion:`
 /// (pnpm >= 3) or `shrinkwrapVersion:` (pnpm 1 / 2) line — lockfile
-/// discovery's sniff before it reads any entry.
+/// discovery's sniff before it reads any entry. A leading BOM is encoding,
+/// not key text: pnpm reads a BOM lock like its plain twin (#903).
 pub(crate) fn is_pnpm_lock_text(text: &str) -> bool {
-    text.lines()
+    strip_bom(text)
+        .lines()
         .any(|line| line.starts_with("lockfileVersion:") || line.starts_with("shrinkwrapVersion:"))
 }
 

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

@@ -35,11 +35,11 @@ pub(crate) use hosted::plan_hosted;
 use std::collections::HashSet;
 
 use crate::constants::npm_family::PNPM_LOCK;
+use crate::formats::text::strip_bom;
 use crate::utils::digest::is_sri_pin;
 use crate::vendor::lock_inventory::{http_url, LockIntegrity, LockfileEntry};
 use crate::vendor::path::parse_vendor_path;
 
-
 // ── entry model ──
 
 /// One `packages:` entry of a pnpm lock, read with the entry grammar
@@ -235,9 +235,10 @@ impl PnpmLockGrammar {
 }
 
 /// The `lockfileVersion:` a lock head (its first five lines) declares,
-/// unquoted.
+/// unquoted. A leading BOM is encoding, not key text (#903).
 fn head_lock_version(text: &str) -> Option<String> {
-    text.lines()
+    strip_bom(text)
+        .lines()
         .take(5)
         .find_map(|line| line.strip_prefix("lockfileVersion:"))
         .map(|rest| rest.trim().trim_matches(['\'', '"']).to_string())
@@ -276,14 +277,18 @@ pub fn sniff_lock_grammar(text: &str) -> Result<PnpmLockGrammar, String> {
 }
 
 /// The `(major, minor)` of every `lockfileVersion:` line of a lock (the
-/// first one decides), unquoted; a missing minor reads as 0.
+/// first one decides), unquoted; a missing minor reads as 0. A leading BOM
+/// is encoding, not key text (#903).
 fn lock_versions(text: &str) -> impl Iterator<Item = (Option<u32>, u32)> + '_ {
-    text.lines().filter_map(|line| {
+    strip_bom(text).lines().filter_map(|line| {
         let rest = line.strip_prefix("lockfileVersion:")?;
         let value = rest.trim().trim_matches(|c| c == '\'' || c == '"');
         let mut parts = value.split('.');
         let major = parts.next().and_then(|m| m.parse::<u32>().ok());
-        let minor = parts.next().and_then(|m| m.parse::<u32>().ok()).unwrap_or(0);
+        let minor = parts
+            .next()
+            .and_then(|m| m.parse::<u32>().ok())
+            .unwrap_or(0);
         Some((major, minor))
     })
 }
@@ -303,10 +308,18 @@ pub fn lock_version_major(text: &str) -> Option<u32> {
 /// rejects it): a `shrinkwrapVersion` lock (pnpm 1–2) or lockfileVersion
 /// 5.0–5.2 (pnpm 3–5). Later locks never get the `--store` note.
 pub fn may_need_store_flag(text: &str) -> bool {
-    text.lines().any(|line| line.starts_with("shrinkwrapVersion:"))
+    is_shrinkwrap_lock(text)
         || lock_versions(text).any(|(major, minor)| major == Some(5) && minor <= 2)
 }
 
+/// Whether a pnpm lock is a pnpm 1–2 `shrinkwrapVersion:` lock (a leading
+/// BOM skipped).
+pub fn is_shrinkwrap_lock(text: &str) -> bool {
+    strip_bom(text)
+        .lines()
+        .any(|line| line.starts_with("shrinkwrapVersion:"))
+}
+
 /// The lockfileVersion the v9 vendored planner splices.
 const V9_LOCK_VERSION: &str = "9.0";
 
@@ -491,7 +504,9 @@ pub(crate) fn vendored_npm_uuids(text: &str) -> HashSet<String> {
         if !in_section {
             continue;
         }
-        if let Some(uuid) = lines::parse_key_line(line, 2).and_then(|(key, _, _)| vendored_npm_uuid(key)) {
+        if let Some(uuid) =
+            lines::parse_key_line(line, 2).and_then(|(key, _, _)| vendored_npm_uuid(key))
+        {
             out.insert(uuid);
         }
     }
@@ -508,17 +523,52 @@ mod tests {
     fn resolves_reads_every_key_generation_boundary_anchored() {
         let lock = |keys: &str| format!("lockfileVersion: '9.0'\n\npackages:\n\n{keys}");
         let yes = [
-            ("  left-pad@1.3.0:\n    resolution: {integrity: sha512-x}\n", "left-pad", "1.3.0"),
-            ("  /left-pad@1.3.0:\n    resolution: {}\n", "left-pad", "1.3.0"),
-            ("  /left-pad/1.3.0:\n    resolution: {}\n", "left-pad", "1.3.0"),
-            ("  'left-pad@1.3.0(react@18.0.0)':\n    dev: false\n", "left-pad", "1.3.0"),
-            ("  /left-pad/1.3.0_react@18.0.0:\n    dev: false\n", "left-pad", "1.3.0"),
-            ("  '@scope/name@1.0.0':\n    dev: false\n", "@scope/name", "1.0.0"),
-            ("  /@scope/name@1.0.0:\n    dev: false\n", "@scope/name", "1.0.0"),
-            ("  /@scope/name/1.0.0:\n    dev: false\n", "@scope/name", "1.0.0"),
+            (
+                "  left-pad@1.3.0:\n    resolution: {integrity: sha512-x}\n",
+                "left-pad",
+                "1.3.0",
+            ),
+            (
+                "  /left-pad@1.3.0:\n    resolution: {}\n",
+                "left-pad",
+                "1.3.0",
+            ),
+            (
+                "  /left-pad/1.3.0:\n    resolution: {}\n",
+                "left-pad",
+                "1.3.0",
+            ),
+            (
+                "  'left-pad@1.3.0(react@18.0.0)':\n    dev: false\n",
+                "left-pad",
+                "1.3.0",
+            ),
+            (
+                "  /left-pad/1.3.0_react@18.0.0:\n    dev: false\n",
+                "left-pad",
+                "1.3.0",
+            ),
+            (
+                "  '@scope/name@1.0.0':\n    dev: false\n",
+                "@scope/name",
+                "1.0.0",
+            ),
+            (
+                "  /@scope/name@1.0.0:\n    dev: false\n",
+                "@scope/name",
+                "1.0.0",
+            ),
+            (
+                "  /@scope/name/1.0.0:\n    dev: false\n",
+                "@scope/name",
+                "1.0.0",
+            ),
         ];
         for (keys, name, version) in yes {
-            assert!(PnpmLock::parse(&lock(keys)).resolves(name, version), "{keys}");
+            assert!(
+                PnpmLock::parse(&lock(keys)).resolves(name, version),
+                "{keys}"
+            );
         }
         let no = [
             ("  left-pad@1.3.0-beta.1:\n    dev: false\n", "left-pad", "1.3.0"),
@@ -534,7 +584,10 @@ mod tests {
             ),
         ];
         for (keys, name, version) in no {
-            assert!(!PnpmLock::parse(&lock(keys)).resolves(name, version), "{keys}");
+            assert!(
+                !PnpmLock::parse(&lock(keys)).resolves(name, version),
+                "{keys}"
+            );
         }
         // Keys outside `packages:` (importers, overrides) resolve nothing.
         let importers = "lockfileVersion: '9.0'\n\nimporters:\n\n  left-pad@1.3.0:\n    x: y\n";
@@ -557,7 +610,10 @@ mod tests {
             let other = "22222222-2222-4222-8222-222222222222";
             assert!(!PnpmLock::parse(text).vendored_in_use(other));
             let crlf = text.replace('\n', "\r\n");
-            assert!(PnpmLock::parse(&crlf).vendored_in_use(UUID), "CRLF reads like LF");
+            assert!(
+                PnpmLock::parse(&crlf).vendored_in_use(UUID),
+                "CRLF reads like LF"
+            );
         }
         // An overrides declaration alone is not usage.
         let overrides = format!(
@@ -582,4 +638,65 @@ mod tests {
         );
         assert_eq!(PnpmLock::parse(&neighbour).wired_integrity(&rel), None);
     }
+
+    /// #903 / #905: a leading UTF-8 BOM is encoding, not content — pnpm
+    /// reads a BOM lock like its plain twin, so every sniff here must too.
+    /// Before the fix the BOM twin read as "not a pnpm lock", unversioned
+    /// and unsupported while its entries still parsed.
+    #[test]
+    fn bom_lock_reads_like_its_plain_twin() {
+        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";
+        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";
+        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";
+        let v52 = "lockfileVersion: 5.2\n\npackages:\n\n  /left-pad/1.3.0:\n    resolution: {integrity: sha512-x}\n";
+        let shrinkwrap = "shrinkwrapVersion: 3\nshrinkwrapMinorVersion: 7\n\npackages:\n\n  /left-pad/1.3.0:\n    resolution: {integrity: sha512-x}\n";
+        for plain in [v9, v6, v54, v52, shrinkwrap] {
+            let bom = format!("\u{feff}{plain}");
+            assert!(PnpmLock::parse(plain).is_pnpm_lock(), "{plain}");
+            assert!(PnpmLock::parse(&bom).is_pnpm_lock(), "BOM twin of {plain}");
+            assert!(is_pnpm_lock_text(&bom), "{plain}");
+            assert_eq!(
+                sniff_lock_grammar(&bom),
+                sniff_lock_grammar(plain),
+                "{plain}"
+            );
+            assert_eq!(
+                lock_version_major(&bom),
+                lock_version_major(plain),
+                "{plain}"
+            );
+            assert_eq!(
+                may_need_store_flag(&bom),
+                may_need_store_flag(plain),
+                "{plain}"
+            );
+            assert_eq!(
+                check_v9_lock_version(&bom),
+                check_v9_lock_version(plain),
+                "{plain}"
+            );
+            let keys = |text: &str| {
+                PnpmLock::parse(text).entries().map(|e| {
+                    e.into_iter()
+                        .map(|e| (e.name, e.version))
+                        .collect::<Vec<_>>()
+                })
+            };
+            assert_eq!(keys(&bom), keys(plain), "{plain}");
+        }
+        assert_eq!(
+            sniff_lock_grammar(&format!("\u{feff}{v9}")),
+            Ok(PnpmLockGrammar::V9)
+        );
+        assert_eq!(lock_version_major(&format!("\u{feff}{v9}")), Some(9));
+        assert!(may_need_store_flag(&format!("\u{feff}{v52}")));
+        assert!(may_need_store_flag(&format!("\u{feff}{shrinkwrap}")));
+        assert!(grammar::unsupported_early_shrinkwrap(
+            "\u{feff}shrinkwrapVersion: 3\n"
+        ));
+        // Exactly one BOM is encoding; a second one is content, as for pnpm.
+        assert!(!is_pnpm_lock_text(
+            "\u{feff}\u{feff}lockfileVersion: '9.0'\n"
+        ));
+    }
 }