Fix pnpm YAML settings read by drifted readers (#1918)

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

@@ -1421,29 +1421,9 @@ fn decode_npm_store_entry_name(entry_name: &str) -> Option<(String, String)> {
     Some((key[..at].to_string(), version.to_string()))
 }
 
-/// The `virtualStoreDir` value of a `.modules.yaml`: JSON on pnpm 10+,
-/// YAML before (a top-level `virtualStoreDir:` scalar, maybe quoted).
+/// The non-empty `virtualStoreDir` value of a `.modules.yaml`.
 fn parse_modules_yaml_virtual_store_dir(text: &str) -> Option<String> {
-    let text = crate::formats::text::strip_bom(text);
-    if let Ok(value) = serde_json::from_str::<serde_json::Value>(text) {
-        return value
-            .get("virtualStoreDir")?
-            .as_str()
-            .filter(|s| !s.is_empty())
-            .map(str::to_string);
-    }
-    let raw = text
-        .lines()
-        .find_map(|line| line.strip_prefix("virtualStoreDir:"))?
-        .trim();
-    let value = if raw.starts_with('"') {
-        serde_json::from_str::<String>(raw).ok()?
-    } else if let Some(inner) = raw.strip_prefix('\'').and_then(|r| r.strip_suffix('\'')) {
-        inner.replace("''", "'")
-    } else {
-        raw.to_string()
-    };
-    (!value.is_empty()).then_some(value)
+    crate::formats::pnpm::modules_yaml::field(text, "virtualStoreDir").filter(|s| !s.is_empty())
 }
 
 /// `store` relative to `importer`, when it names a directory strictly
@@ -4820,6 +4800,28 @@ fn is_safe_npm_component(component: &str) -> bool {
 mod tests {
     use super::*;
 
+    /// #1918: `.modules.yaml`'s `virtualStoreDir` is read through the shared
+    /// pnpm settings reader: every key spelling, the last key, no comment.
+    #[test]
+    fn modules_yaml_virtual_store_dir_uses_the_shared_reader() {
+        let read = parse_modules_yaml_virtual_store_dir;
+        assert_eq!(read("virtualStoreDir: .pnpm\n").as_deref(), Some(".pnpm"));
+        assert_eq!(
+            read("virtualStoreDir: .pnpm # default\n").as_deref(),
+            Some(".pnpm")
+        );
+        assert_eq!(
+            read("virtualStoreDir: a\nvirtualStoreDir: 'b''c'\n").as_deref(),
+            Some("b'c")
+        );
+        assert_eq!(read("\"virtualStoreDir\": \"x\"\n").as_deref(), Some("x"));
+        assert_eq!(read("virtualStoreDir: ''\n"), None);
+        assert_eq!(
+            read("{\"virtualStoreDir\":\"/abs/.pnpm\"}").as_deref(),
+            Some("/abs/.pnpm")
+        );
+    }
+
     tokio::task_local! {
         pub(super) static VARIANT_STORE_SCANS: std::cell::RefCell<Vec<PathBuf>>;
     }

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

@@ -142,33 +142,17 @@ fn setting_from(workspace_yaml: Option<&str>, npmrc: Option<&str>) -> Option<Str
         .filter(|value| !value.is_empty())
 }
 
-/// The last top-level `modulesDir:` of a `pnpm-workspace.yaml`.
+/// The last top-level `modulesDir:` of a `pnpm-workspace.yaml`. An empty
+/// value is kept: it still beats `.npmrc`.
 fn workspace_modules_dir(yaml: &str) -> Option<String> {
-    crate::formats::text::strip_bom(yaml)
-        .lines()
-        .filter_map(crate::formats::pnpm::workspace::top_level_key)
-        .rfind(|(key, _)| key == "modulesDir")
-        .map(|(_, value)| unquote_yaml_scalar(value))
+    crate::formats::pnpm::workspace::top_level_scalar(yaml, "modulesDir")
 }
 
 /// The top-level `modules-dir` of an `.npmrc`.
 fn npmrc_modules_dir(npmrc: &str) -> Option<String> {
     crate::patch::redirect::npmrc::npmrc_top_level_value(npmrc, "modules-dir")
 }
 
-/// A YAML flow scalar's value: quotes removed (`''` is a literal quote
-/// inside single quotes), a plain scalar as is.
-fn unquote_yaml_scalar(raw: &str) -> String {
-    if raw.starts_with('"') {
-        if let Ok(value) = serde_json::from_str::<String>(raw) {
-            return value;
-        }
-    } else if let Some(inner) = raw.strip_prefix('\'').and_then(|r| r.strip_suffix('\'')) {
-        return inner.replace("''", "'");
-    }
-    raw.to_string()
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

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

@@ -27,6 +27,7 @@
 pub(crate) mod grammar;
 pub(crate) mod hosted;
 pub(crate) mod lines;
+pub(crate) mod modules_yaml;
 mod version;
 pub(crate) mod workspace;
 

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

@@ -0,0 +1,47 @@
+//! pnpm's install record, `node_modules/.modules.yaml`: JSON on pnpm 10+,
+//! YAML before. Its top-level fields are read here, once, so the crawler,
+//! the version gate and the redirect upstream agree on quoting and on
+//! which of two duplicate keys wins (#1918).
+
+use crate::formats::text::strip_bom;
+
+/// The top-level string `key` of a `.modules.yaml`: from the JSON object
+/// on pnpm 10+, else the last top-level YAML scalar (the shared pnpm
+/// settings reader, [`super::workspace::top_level_scalar`]). An empty
+/// value is `Some("")`; callers that read empty as unset filter it.
+pub(crate) fn field(text: &str, key: &str) -> Option<String> {
+    let text = strip_bom(text);
+    if let Ok(value) = serde_json::from_str::<serde_json::Value>(text) {
+        return value.get(key)?.as_str().map(str::to_string);
+    }
+    super::workspace::top_level_scalar(text, key)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn reads_json_and_yaml_records_alike() {
+        for text in [
+            "{\"packageManager\":\"pnpm@9.1.0\"}",
+            "\u{feff}{\"packageManager\":\"pnpm@9.1.0\"}",
+            "packageManager: pnpm@9.1.0\n",
+            "\u{feff}layoutVersion: 5\npackageManager: 'pnpm@9.1.0'\n",
+            "packageManager: pnpm@8.0.0\npackageManager: \"pnpm@9.1.0\" # last\n",
+        ] {
+            assert_eq!(
+                field(text, "packageManager").as_deref(),
+                Some("pnpm@9.1.0"),
+                "{text:?}"
+            );
+        }
+        assert_eq!(field("{}", "packageManager"), None);
+        assert_eq!(field("{\"packageManager\":1}", "packageManager"), None);
+        assert_eq!(
+            field("packageManager: ''\n", "packageManager").as_deref(),
+            Some("")
+        );
+        assert_eq!(field("layoutVersion: 5\n", "packageManager"), None);
+    }
+}

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

@@ -98,26 +98,15 @@ fn version_bounds(spec: &str) -> Option<(Release, Release)> {
     })
 }
 
-/// The `packageManager` value of a `.modules.yaml`: JSON on pnpm 10+,
-/// YAML before (a top-level scalar, maybe quoted).
-fn modules_yaml_package_manager(text: &str) -> Option<String> {
-    let text = strip_bom(text);
-    if let Ok(value) = serde_json::from_str::<serde_json::Value>(text) {
-        return value.get("packageManager")?.as_str().map(str::to_string);
-    }
-    text.split('\n').find_map(|line| {
-        let (key, value) = super::workspace::top_level_key(line)?;
-        (key == "packageManager").then(|| value.trim_matches(|c| c == '\'' || c == '"').to_string())
-    })
-}
-
 /// Every pnpm version pin the project's files carry, as `(where, spec)`.
 fn pnpm_pins(
     package_json: Option<&str>,
     modules_yaml: Option<&str>,
 ) -> Vec<(&'static str, String)> {
     let mut pins = Vec::new();
-    if let Some(spec) = modules_yaml.and_then(modules_yaml_package_manager) {
+    if let Some(spec) =
+        modules_yaml.and_then(|text| super::modules_yaml::field(text, "packageManager"))
+    {
         if let Some(version) = pinned_version(&spec, "pnpm") {
             pins.push(("node_modules/.modules.yaml", version.to_string()));
         }
@@ -206,6 +195,18 @@ pub fn matches_file_id_build_keys(
 mod tests {
     use super::*;
 
+    /// #1918: a YAML `.modules.yaml` is read like every other pnpm settings
+    /// file: the last top-level key wins, comments are not part of the value.
+    #[test]
+    fn modules_yaml_pin_reads_the_last_key() {
+        let pins = |text: &str| pnpm_pins(None, Some(text));
+        let text = "packageManager: pnpm@9.0.0\npackageManager: pnpm@11.0.0 # newer\n";
+        let want = vec![("node_modules/.modules.yaml", "11.0.0".to_string())];
+        assert_eq!(pins(text), want);
+        assert_eq!(pins("packageManager: 'pnpm@11.0.0'\n"), want);
+        assert_eq!(pins("{\"packageManager\":\"pnpm@11.0.0\"}"), want);
+    }
+
     #[test]
     fn file_id_build_keys_start_at_10_34_2_and_11_5_3() {
         let pm = |spec: &str| format!("{{\"packageManager\":\"pnpm@{spec}\"}}");

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

@@ -57,14 +57,41 @@ pub(crate) fn top_level_key(line: &str) -> Option<(String, &str)> {
 }
 
 /// The value of the last top-level `key` in a YAML settings file
-/// (pnpm-workspace.yaml, .yarnrc.yml), quotes removed. [`top_level_key`]
-/// skips the first line's BOM.
-pub(crate) fn yaml_top_level_value(text: &str, key: &str) -> Option<String> {
+/// (pnpm-workspace.yaml, .yarnrc.yml, a YAML `.modules.yaml`), decoded by
+/// [`yaml_scalar`]. An empty value or a nested block is `Some("")`
+/// (callers that read empty as unset use [`yaml_top_level_value`]); a
+/// block scalar (`key: |`) is `None`, unread. [`top_level_key`]
+/// skips the first line's BOM. Every pnpm settings reader goes through
+/// this one function so a quoted value reads the same everywhere (#1918).
+pub(crate) fn top_level_scalar(text: &str, key: &str) -> Option<String> {
     text.lines()
         .filter_map(top_level_key)
         .rfind(|(k, _)| k == key)
-        .map(|(_, value)| value.trim_matches(['"', '\'']).to_string())
-        .filter(|value| !value.is_empty())
+        .and_then(|(_, value)| {
+            // A block scalar (`|`, `>-`) continues on the next lines; its
+            // value is not on this one, so it is not read here.
+            (!value.starts_with(['|', '>'])).then(|| yaml_scalar(value))
+        })
+}
+
+/// [`top_level_scalar`], with an empty value read as unset.
+pub(crate) fn yaml_top_level_value(text: &str, key: &str) -> Option<String> {
+    top_level_scalar(text, key).filter(|value| !value.is_empty())
+}
+
+/// A YAML flow scalar's value: a whole single-quoted (`''` is one quote)
+/// or double-quoted (escapes decoded) scalar is unquoted; anything else,
+/// an unpaired or trailing quote included, is a plain scalar kept as is.
+pub(crate) fn yaml_scalar(raw: &str) -> String {
+    let quoted = match raw.as_bytes().first() {
+        Some(b'"') => double_quoted(raw),
+        Some(b'\'') => single_quoted(raw),
+        _ => None,
+    };
+    match quoted {
+        Some((value, len)) if len == raw.len() => value,
+        _ => raw.to_string(),
+    }
 }
 
 /// The line index a new top-level key is inserted at: after the document's
@@ -310,8 +337,13 @@ fn is_marker(line: &str, marker: &str) -> bool {
 pub(crate) fn strip_comment(text: &str) -> &str {
     let bytes = text.as_bytes();
     let mut quote = None;
+    let mut escaped = false;
     for (i, &b) in bytes.iter().enumerate() {
         match quote {
+            // Inside double quotes a backslash escapes the next byte, so
+            // `\"` does not close the scalar (#1918).
+            Some(b'"') if escaped => escaped = false,
+            Some(b'"') if b == b'\\' => escaped = true,
             Some(q) if b == q => quote = None,
             Some(_) => {}
             None if b == b'\'' || b == b'"' => quote = Some(b),
@@ -325,25 +357,60 @@ pub(crate) fn strip_comment(text: &str) -> &str {
 }
 
 /// A double-quoted scalar at the start of `s`: its value and byte length.
+/// Escapes decode as YAML 1.2 (and js-yaml) does, `\uD83D\uDE00`
+/// surrogate pairs included; a malformed escape is `None`, as the file
+/// does not parse.
 fn double_quoted(s: &str) -> Option<(String, usize)> {
+    /// `n` hex digits as a code point (`None` when short or not hex).
+    fn hex(chars: &mut impl Iterator<Item = (usize, char)>, n: usize) -> Option<u32> {
+        let digits: String = (0..n).map_while(|_| chars.next().map(|(_, h)| h)).collect();
+        (digits.len() == n && digits.chars().all(|c| c.is_ascii_hexdigit()))
+            .then(|| u32::from_str_radix(&digits, 16).ok())
+            .flatten()
+    }
     let mut out = String::new();
     let mut chars = s.char_indices().skip(1);
     while let Some((i, c)) = chars.next() {
         match c {
             '"' => return Some((out, i + 1)),
             '\\' => {
                 let (_, esc) = chars.next()?;
-                match esc {
-                    'n' => out.push('\n'),
-                    't' => out.push('\t'),
-                    '"' | '\\' | '/' | ' ' => out.push(esc),
-                    // Any other escape decodes to text no pnpm key uses;
-                    // keep it visibly distinct rather than guessing.
-                    other => {
-                        out.push('\\');
-                        out.push(other);
+                let decoded = match esc {
+                    '0' => '\0',
+                    'a' => '\u{7}',
+                    'b' => '\u{8}',
+                    't' | '\t' => '\t',
+                    'n' => '\n',
+                    'v' => '\u{b}',
+                    'f' => '\u{c}',
+                    'r' => '\r',
+                    'e' => '\u{1b}',
+                    'N' => '\u{85}',
+                    '_' => '\u{a0}',
+                    'L' => '\u{2028}',
+                    'P' => '\u{2029}',
+                    '"' | '\\' | '/' | ' ' => esc,
+                    'x' => char::from_u32(hex(&mut chars, 2)?)?,
+                    'U' => char::from_u32(hex(&mut chars, 8)?)?,
+                    'u' => {
+                        let unit = hex(&mut chars, 4)?;
+                        if (0xD800..0xDC00).contains(&unit) {
+                            // A high surrogate pairs with a following `\uDCxx`.
+                            if chars.next()?.1 != '\\' || chars.next()?.1 != 'u' {
+                                return None;
+                            }
+                            let low = hex(&mut chars, 4)?;
+                            if !(0xDC00..0xE000).contains(&low) {
+                                return None;
+                            }
+                            char::from_u32(0x10000 + ((unit - 0xD800) << 10) + (low - 0xDC00))?
+                        } else {
+                            char::from_u32(unit)?
+                        }
                     }
-                }
+                    _ => return None,
+                };
+                out.push(decoded);
             }
             c => out.push(c),
         }
@@ -376,6 +443,55 @@ fn single_quoted(s: &str) -> Option<(String, usize)> {
 mod tests {
     use super::*;
 
+    /// #1918: the one pnpm settings scalar reader every caller uses.
+    #[test]
+    fn top_level_scalar_table() {
+        for (text, want) in [
+            ("k: plain\n", Some("plain")),
+            ("k: plain # comment\n", Some("plain")),
+            ("k: \"a\\\"b\\n\\u0041\"\n", Some("a\"b\nA")),
+            ("k: \"a # not a comment\"\n", Some("a # not a comment")),
+            ("k: 'it''s'\n", Some("it's")),
+            ("k: ''\n", Some("")),
+            ("k:\n", Some("")),
+            ("k: \"a'\n", Some("\"a'")),
+            ("k: 'a\"\n", Some("'a\"")),
+            ("k: 'a' b\n", Some("'a' b")),
+            ("\u{feff}k: x\n", Some("x")),
+            ("k: a\r\nk: 'b'\r\n", Some("b")),
+            // Review on #1973: escapes the old JSON decoder read, an escaped
+            // quote before ` #`, a malformed escape, and a block scalar.
+            ("k: \"/p/a\\\" #b/.pnpm\"\n", Some("/p/a\" #b/.pnpm")),
+            ("k: \"nm-\\ud83d\\ude00\"\n", Some("nm-\u{1f600}")),
+            ("k: \"a\\bb\\x41\\U0001F600\"\n", Some("a\u{8}bA\u{1f600}")),
+            ("k: \"\\u+041\"\n", Some("\"\\u+041\"")),
+            ("k: \"\\ud83d\"\n", Some("\"\\ud83d\"")),
+            ("k: >-\n  long value\n", None),
+            ("k: |\n  text\n", None),
+            ("  k: nested\n", None),
+            ("other: x\n", None),
+        ] {
+            assert_eq!(top_level_scalar(text, "k").as_deref(), want, "{text:?}");
+        }
+    }
+
+    /// #1918: a settings scalar is decoded the way pnpm's YAML parser reads
+    /// it: `''` inside single quotes is one quote, an unpaired quote is not
+    /// stripped, and an empty value stays unset for this non-empty reader.
+    #[test]
+    fn yaml_top_level_value_decodes_yaml_quoting() {
+        let read = |text: &str| yaml_top_level_value(text, "k");
+        assert_eq!(read("k: 'it''s'\n").as_deref(), Some("it's"));
+        assert_eq!(read("k: \"a'\n").as_deref(), Some("\"a'"));
+        assert_eq!(read("k: ''a''\n").as_deref(), Some("''a''"));
+        assert_eq!(
+            read("k: \"say \\\"hi\\\"\"\n").as_deref(),
+            Some("say \"hi\"")
+        );
+        assert_eq!(read("k: ''\n"), None);
+        assert_eq!(read("k:\n"), None);
+    }
+
     fn lines(text: &str) -> Vec<String> {
         text.split('\n').map(str::to_string).collect()
     }