Fix pnpm lock readers skipping main doc (#1949, #1952)

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

@@ -369,8 +369,13 @@ pub(crate) fn check_v9_lock_version(text: &str) -> Result<(), String> {
 /// One pnpm lock, parsed once (see the module docs).
 pub struct PnpmLock<'t> {
     text: &'t str,
+    /// The project document ([`grammar::main_document`]): pnpm 11+ writes
+    /// its env lockfile (`configDependencies`, `packageManagerDependencies`)
+    /// as a first document whose packages the install never links (#1949).
+    doc: &'t str,
+    /// Entries of the project document only, with offsets into `text`.
     packages: Vec<PnpmPackage<'t>>,
-    /// [`vendored_npm_uuids`] of the text.
+    /// [`vendored_npm_uuids`] of the project document.
     vendored: HashSet<String>,
 }
 
@@ -386,10 +391,18 @@ pub(crate) struct PnpmTarballRef<'p> {
 impl<'t> PnpmLock<'t> {
     /// Parse a lock text (any content: a non-pnpm text has no entries).
     pub fn parse(text: &'t str) -> Self {
+        let doc = grammar::main_document(strip_bom(text));
+        // `doc` is a suffix of `text`: parse the whole text so offsets stay
+        // relative to it, and keep the entries that start in `doc`.
+        let doc_start = text.len() - doc.len();
         PnpmLock {
             text,
-            packages: pnpm_packages(text),
-            vendored: vendored_npm_uuids(text),
+            doc,
+            packages: pnpm_packages(text)
+                .into_iter()
+                .filter(|p| p.entry.offset >= doc_start)
+                .collect(),
+            vendored: vendored_npm_uuids(doc),
         }
     }
 
@@ -408,7 +421,7 @@ impl<'t> PnpmLock<'t> {
     }
 
     fn has_packages_section(&self) -> bool {
-        self.text
+        self.doc
             .lines()
             .any(|l| l.trim_end_matches('\r') == "packages:")
     }
@@ -754,6 +767,51 @@ mod tests {
         assert!(!PnpmLock::parse(importers).resolves("left-pad", "1.3.0"));
     }
 
+    /// #1949: a pnpm 11+ lock's env document (config dependencies the
+    /// install never links) is not part of the inventory: `entries()`,
+    /// `resolves()`, the vendored uuids and `InstalledPackages` all read
+    /// the project document alone, and agree with each other.
+    #[test]
+    fn env_document_is_not_in_the_inventory() {
+        let env = format!(
+            "---\nlockfileVersion: '9.0'\n\nimporters:\n\n  .:\n    configDependencies:\n      cfg-dep:\n        specifier: 2.0.0\n        version: 2.0.0\n\npackages:\n\n  cfg-dep@2.0.0:\n    resolution: {{integrity: sha512-c}}\n\n  b@file:.socket/vendor/npm/{UUID}/b-1.0.0.tgz:\n    resolution: {{integrity: sha512-x, tarball: file:.socket/vendor/npm/{UUID}/b-1.0.0.tgz}}\n    version: 1.0.0\n\n"
+        );
+        let project = "---\nlockfileVersion: '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";
+        for text in [
+            format!("{env}{project}"),
+            format!("\u{feff}{env}{project}"),
+            format!("{env}{project}").replace('\n', "\r\n"),
+        ] {
+            let lock = PnpmLock::parse(&text);
+            let names: Vec<_> = lock
+                .entries()
+                .unwrap()
+                .into_iter()
+                .map(|e| (e.name, e.version))
+                .collect();
+            assert_eq!(
+                names,
+                [("left-pad".to_string(), "1.3.0".to_string())],
+                "{text:?}"
+            );
+            assert!(lock.resolves("left-pad", "1.3.0"));
+            assert!(!lock.resolves("cfg-dep", "2.0.0"));
+            assert!(!lock.vendored_in_use(UUID));
+            assert!(lock.packages().iter().all(|p| p.entry.offset >= env.len()));
+            let installed = InstalledPackages::from_lock_text(&text).unwrap();
+            assert!(installed.contains("left-pad", "1.3.0"));
+            assert!(!installed.contains("cfg-dep", "2.0.0"));
+        }
+        // Offsets still index the whole text.
+        let text = format!("{env}{project}");
+        let lock = PnpmLock::parse(&text);
+        let entry = &lock.packages()[0].entry;
+        assert_eq!(
+            &text[entry.offset..entry.offset + entry.body.len()],
+            entry.body
+        );
+    }
+
     #[test]
     fn vendored_in_use_reads_v9_and_legacy_keys_and_crlf() {
         let v9 = format!(

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

@@ -151,14 +151,20 @@ pub(crate) fn is_keyless(text: &str) -> bool {
 /// nested package on pnpm 8–10.4 (`packages` defaults to `['.', '**']`) but
 /// as the root alone on pnpm >= 10.5, so a lock made from one with member
 /// importers comes from pnpm <= 10.4, and the root-only
-/// [`PACKAGES_SCAFFOLD`] would drop those members.
-pub(crate) fn lock_has_member_importers(lines: &[String]) -> bool {
+/// [`PACKAGES_SCAFFOLD`] would drop those members. The same answer marks a
+/// root lock as a SHARED workspace lock (`utils::pnpm_workspace`). Comment
+/// lines are neither keys nor section ends (#1952).
+pub(crate) fn lock_has_member_importers<S: AsRef<str>>(lines: &[S]) -> bool {
     // CRLF-blind: a Windows checkout's lock reads like its LF twin.
     let mut in_importers = false;
     for line in lines {
+        let line = line.as_ref();
         let line = line.strip_suffix('\r').unwrap_or(line);
-        if !line.is_empty() && !line.starts_with(' ') {
-            in_importers = line == "importers:";
+        if line.trim_start().starts_with('#') {
+            continue;
+        }
+        if !line.is_empty() && !line.starts_with([' ', '\t']) {
+            in_importers = line.trim_end() == "importers:";
         } else if in_importers
             && super::lines::parse_key_line(line, 2).is_some_and(|(key, _, _)| key != ".")
         {
@@ -168,6 +174,14 @@ pub(crate) fn lock_has_member_importers(lines: &[String]) -> bool {
     false
 }
 
+/// [`lock_has_member_importers`] of a whole `pnpm-lock.yaml` text: its
+/// project document alone ([`super::grammar::main_document`]), so importers
+/// of a pnpm 11+ env document never count.
+pub(crate) fn lock_text_has_member_importers(text: &str) -> bool {
+    let doc = super::grammar::main_document(strip_bom(text));
+    lock_has_member_importers(&doc.split('\n').collect::<Vec<_>>())
+}
+
 /// `text` with the contiguous run of lines `block` removed, when that
 /// leaves a [`is_keyless`] document: the inverse of a splice that gave a
 /// keyless file its first keys. `None` when the run is absent or the rest
@@ -506,6 +520,36 @@ mod tests {
         assert!(lock_has_member_importers(&crlf));
     }
 
+    /// #1952: a commented-out importer is not a member, a column-0 comment
+    /// does not end `importers:`, and the env document's importers never
+    /// count; the line and text forms agree.
+    #[test]
+    fn member_importers_skip_comments_and_the_env_document() {
+        let head = "lockfileVersion: '9.0'\n\nimporters:\n\n  .:\n    dependencies: {}\n";
+        for (body, members) in [
+            ("  # sub:\n  #   dependencies: {}\n", false),
+            ("# a note\n  sub:\n    dependencies: {}\n", true),
+            ("    # nested: comment\n", false),
+        ] {
+            let text = format!("{head}{body}\npackages: {{}}\n");
+            assert_eq!(lock_text_has_member_importers(&text), members, "{text:?}");
+            assert_eq!(
+                lock_has_member_importers(&lines(&text)),
+                members,
+                "{text:?}"
+            );
+        }
+        let env = "---\nlockfileVersion: '9.0'\n\nimporters:\n\n  .: {}\n\n  tooling:\n    configDependencies: {}\n\n";
+        let project = format!("---\n{head}\npackages: {{}}\n");
+        assert!(!lock_text_has_member_importers(&format!("{env}{project}")));
+        assert!(!lock_text_has_member_importers(&format!(
+            "\u{feff}{env}{project}"
+        )));
+        assert!(lock_text_has_member_importers(&format!(
+            "{env}---\n{head}  sub:\n    dependencies: {{}}\n"
+        )));
+    }
+
     #[test]
     fn keyless_documents_and_their_splices() {
         for text in [

crates/socket-patch-core/src/hosted/engine.rs

@@ -2991,13 +2991,8 @@ fn read_advisory_text(view: &ProjectView<'_>, rel: &str) -> Option<String> {
 /// root: with a keyless pnpm-workspace.yaml that marks a pnpm <= 10.4
 /// install (see [`crate::formats::pnpm::workspace::lock_has_member_importers`]).
 fn lock_lists_members(view: &ProjectView<'_>) -> bool {
-    read_advisory_text(view, "pnpm-lock.yaml").is_some_and(|text| {
-        let doc =
-            crate::formats::pnpm::grammar::main_document(crate::formats::text::strip_bom(&text));
-        crate::formats::pnpm::workspace::lock_has_member_importers(
-            &crate::formats::pnpm::lines::split_lines(doc),
-        )
-    })
+    read_advisory_text(view, "pnpm-lock.yaml")
+        .is_some_and(|text| crate::formats::pnpm::workspace::lock_text_has_member_importers(&text))
 }
 
 /// The ancestor `pnpm-workspace.yaml` governing a disk project's pnpm

crates/socket-patch-core/src/utils/pnpm_workspace.rs

@@ -262,37 +262,6 @@ pub fn shared_lockfile_disabled(workspace: &str, npmrc: Option<&str>) -> bool {
     .is_some_and(|setting| setting.as_bool() == Some(false))
 }
 
-/// Whether a root lock is a SHARED workspace lock: its `importers:` names a
-/// project other than the root (`.`). Such a lock is what pnpm installs
-/// every member from, whatever the settings say now, so member locks
-/// beside it are stale leftovers.
-pub fn root_lock_lists_members(lock: &str) -> bool {
-    let mut in_importers = false;
-    for line in lock.lines() {
-        let line = line.strip_suffix('\r').unwrap_or(line);
-        if !line.starts_with([' ', '\t']) && !line.trim().is_empty() {
-            in_importers = line.trim_end() == "importers:";
-            continue;
-        }
-        if !in_importers {
-            continue;
-        }
-        // An importer key sits at exactly two spaces.
-        let Some(key) = line
-            .strip_prefix("  ")
-            .filter(|k| !k.starts_with([' ', '#']))
-        else {
-            continue;
-        };
-        let key = key.split(':').next().unwrap_or("").trim();
-        let key = key.trim_matches(['\'', '"']);
-        if !key.is_empty() && key != "." {
-            return true;
-        }
-    }
-    false
-}
-
 /// Whether pnpm's project finder lists the directory at `rel` (relative
 /// to the workspace root, one entry per component) under `globs`
 /// (`pnpm-workspace.yaml` `packages:`, as [`read_package_globs`] reads them), as
@@ -473,7 +442,13 @@ async fn workspace_members(view: &ProjectView<'_>) -> Members {
         return Members::Shared;
     }
     let root_lock = view.read_text(PNPM_LOCK).await.ok();
-    if root_lock.as_deref().is_some_and(root_lock_lists_members) {
+    // A root lock whose importers name a project other than `.` is a
+    // SHARED workspace lock: pnpm installs every member from it, whatever
+    // the settings say now, so member locks beside it are stale leftovers.
+    if root_lock
+        .as_deref()
+        .is_some_and(crate::formats::pnpm::workspace::lock_text_has_member_importers)
+    {
         return Members::Shared;
     }
     let globs = match read_package_globs(&workspace) {
@@ -1217,18 +1192,19 @@ mod tests {
 
     #[test]
     fn a_root_lock_listing_member_importers_is_shared() {
+        use crate::formats::pnpm::workspace::lock_text_has_member_importers;
         let root_only = "lockfileVersion: '9.0'\n\nimporters:\n\n  .:\n    dependencies:\n      a:\n        specifier: 1.0.0\n        version: 1.0.0\n\npackages:\n\n  a@1.0.0:\n    resolution: {integrity: sha512-x}\n";
-        assert!(!root_lock_lists_members(root_only));
-        assert!(!root_lock_lists_members(
+        assert!(!lock_text_has_member_importers(root_only));
+        assert!(!lock_text_has_member_importers(
             "lockfileVersion: '9.0'\n\nimporters:\n\n  .: {}\n"
         ));
-        assert!(!root_lock_lists_members(
+        assert!(!lock_text_has_member_importers(
             "lockfileVersion: 5.4\n\ndependencies:\n  a: 1.0.0\n"
         ));
-        assert!(root_lock_lists_members(
+        assert!(lock_text_has_member_importers(
             "lockfileVersion: '9.0'\r\n\r\nimporters:\r\n\r\n  .: {}\r\n\r\n  packages/a:\r\n    dependencies: {}\r\n"
         ));
-        assert!(root_lock_lists_members(
+        assert!(lock_text_has_member_importers(
             "lockfileVersion: '6.0'\nimporters:\n  '.': {}\n  'packages/a': {}\n"
         ));
     }