Fix yarn berry missing portal:/link: copies (#1561)

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

@@ -1880,6 +1880,56 @@ async fn issue_939_berry_renamed_copy_is_named() {
     }
 }
 
+/// Run the hosted engine over a berry project that depends on left-pad
+/// from the registry and on `lp2` through the `portal:` / `link:` `spec`,
+/// plus `extra` files, and return its `redirect_copy_unpatched` details
+/// (after checking the registry entry is still pinned).
+async fn berry_linked_copy_details(uuid: &str, spec: &str, extra: &[(&str, &str)]) -> Vec<String> {
+    let ws = "app%40workspace%3A.";
+    let key = format!("lp2@{spec}::locator={ws}");
+    let server = MockServer::start().await;
+    mount_left_pad_grant(&server, uuid).await;
+    let mut files = BTreeMap::new();
+    files.insert(
+        "package.json".to_string(),
+        format!(
+            r#"{{"name":"app","version":"1.0.0","dependencies":{{"left-pad":"^1.3.0","lp2":"{spec}"}}}}"#
+        )
+        .into_bytes(),
+    );
+    files.insert(
+        "yarn.lock".to_string(),
+        format!(
+            "__metadata:\n  version: 8\n  cacheKey: 10c0\n\n\
+             \"app@workspace:.\":\n  version: 0.0.0-use.local\n  \
+             resolution: \"app@workspace:.\"\n  dependencies:\n    \
+             left-pad: \"npm:^1.3.0\"\n    lp2: \"{spec}\"\n  \
+             languageName: unknown\n  linkType: soft\n\n\
+             \"left-pad@npm:^1.3.0\":\n  version: 1.3.0\n  \
+             resolution: \"left-pad@npm:1.3.0\"\n  checksum: 10c0/{}\n  \
+             languageName: node\n  linkType: hard\n\n\
+             \"{key}\":\n  version: 0.0.0-use.local\n  resolution: \"{key}\"\n  \
+             languageName: node\n  linkType: soft\n",
+            "1".repeat(128)
+        )
+        .into_bytes(),
+    );
+    for (rel, text) in extra {
+        files.insert(rel.to_string(), text.as_bytes().to_vec());
+    }
+    let output = run_engine(&server, build_input(&files, &[], options(false))).await;
+    let project = &output.projects[0];
+    assert!(project.error.is_none(), "{spec}: {:?}", project.error);
+    assert_eq!(
+        project.redirected.len(),
+        1,
+        "{spec}: {:?} {:#}",
+        project.skipped,
+        project.redirect
+    );
+    copy_unpatched_details(project)
+}
+
 /// #1540: a berry `portal:` / `link:` copy with an ABSOLUTE path is never
 /// read (#1516), and its lock version is the placeholder
 /// `0.0.0-use.local`, so it may be any version of any package. The hosted
@@ -1888,56 +1938,49 @@ async fn issue_939_berry_renamed_copy_is_named() {
 /// report a clean success while `node_modules/lp2` held unpatched bytes.
 #[tokio::test]
 async fn issue_1540_berry_absolute_portal_and_link_copies_are_named() {
-    const UUID: &str = "15401540-1540-4540-8540-154015401540";
-    let ws = "app%40workspace%3A.";
     for protocol in ["portal", "link"] {
         let spec = format!("{protocol}:/home/me/shared/left-pad");
-        let key = format!("lp2@{spec}::locator={ws}");
-        let server = MockServer::start().await;
-        mount_left_pad_grant(&server, UUID).await;
-        let mut files = BTreeMap::new();
-        files.insert(
-            "package.json".to_string(),
-            format!(
-                r#"{{"name":"app","version":"1.0.0","dependencies":{{"left-pad":"^1.3.0","lp2":"{spec}"}}}}"#
-            )
-            .into_bytes(),
-        );
-        files.insert(
-            "yarn.lock".to_string(),
-            format!(
-                "__metadata:\n  version: 8\n  cacheKey: 10c0\n\n\
-                 \"app@workspace:.\":\n  version: 0.0.0-use.local\n  \
-                 resolution: \"app@workspace:.\"\n  dependencies:\n    \
-                 left-pad: \"npm:^1.3.0\"\n    lp2: \"{spec}\"\n  \
-                 languageName: unknown\n  linkType: soft\n\n\
-                 \"left-pad@npm:^1.3.0\":\n  version: 1.3.0\n  \
-                 resolution: \"left-pad@npm:1.3.0\"\n  checksum: 10c0/{}\n  \
-                 languageName: node\n  linkType: hard\n\n\
-                 \"{key}\":\n  version: 0.0.0-use.local\n  resolution: \"{key}\"\n  \
-                 languageName: node\n  linkType: soft\n",
-                "1".repeat(128)
-            )
-            .into_bytes(),
-        );
-        let output = run_engine(&server, build_input(&files, &[], options(false))).await;
-        let project = &output.projects[0];
-        assert!(project.error.is_none(), "{protocol}: {:?}", project.error);
-        assert_eq!(
-            project.redirected.len(),
-            1,
-            "{protocol}: {:?} {:#}",
-            project.skipped,
-            project.redirect
+        let details =
+            berry_linked_copy_details("15401540-1540-4540-8540-154015401540", &spec, &[]).await;
+        assert_eq!(details.len(), 1, "{protocol}: {details:?}");
+        assert!(
+            details[0].contains(&format!("lp2@{spec}::"))
+                && details[0].contains("can't be read")
+                && details[0].contains("UNPATCHED"),
+            "{protocol}: {}",
+            details[0]
         );
-        let details = copy_unpatched_details(project);
-        assert_eq!(details.len(), 1, "{protocol}: {:#}", project.redirect);
+    }
+}
+
+/// #1561: a berry `portal:` / `link:` copy with a RELATIVE path whose
+/// target is not in the snapshot (a git submodule CI never checked out)
+/// can't be read, so like an absolute one (#1540) it may be any version of
+/// any package. The hosted scan still pins the registry entry, and names
+/// that copy as possibly unpatched (`redirect_copy_unpatched`); it used to
+/// report a clean success while the install with the submodule linked
+/// unpatched bytes as `node_modules/lp2`. Control: a target whose
+/// `package.json` the snapshot holds is read, so one naming another
+/// package is no copy of left-pad.
+#[tokio::test]
+async fn issue_1561_berry_missing_relative_portal_and_link_copies_are_named() {
+    const UUID: &str = "15611561-1561-4561-8561-156115611561";
+    for protocol in ["portal", "link"] {
+        let spec = format!("{protocol}:./vend/lp");
+        let details = berry_linked_copy_details(UUID, &spec, &[]).await;
+        assert_eq!(details.len(), 1, "{protocol}: {details:?}");
         assert!(
-            details[0].contains(key.as_str())
+            details[0].contains(&format!("lp2@{spec}::"))
                 && details[0].contains("can't be read")
                 && details[0].contains("UNPATCHED"),
             "{protocol}: {}",
             details[0]
         );
+        let other = [(
+            "vend/lp/package.json",
+            r#"{"name":"other-pkg","version":"1.3.0"}"#,
+        )];
+        let details = berry_linked_copy_details(UUID, &spec, &other).await;
+        assert!(details.is_empty(), "{protocol}: {details:?}");
     }
 }

crates/socket-patch-core/src/formats/yarn/copy.rs

@@ -58,13 +58,6 @@ impl CopyRef {
         }
     }
 
-    /// A `portal:` / `link:` copy that is never read ([`CopyRef::read_path`]
-    /// is `None`: an absolute path). Its lock version is the placeholder
-    /// `0.0.0-use.local`, so it may be any version of any package (#1540).
-    pub(crate) fn is_unread_linked(&self) -> bool {
-        matches!(self, CopyRef::Linked(_)) && self.read_path().is_none()
-    }
-
     /// The `(name, version)` this copy installs, given the lock entry's
     /// `version` and the bytes read at [`CopyRef::read_path`]. `None` when
     /// the copy does not say (an unreadable or nameless manifest, a
@@ -219,6 +212,13 @@ fn copy_path(base: &str, path: &str) -> Option<String> {
     ))
 }
 
+/// Whether `bytes` parse as a JSON `package.json` (any content): a copy
+/// whose manifest doesn't (a Git LFS pointer, an encrypted blob) can't be
+/// read for its package (#1561).
+pub(crate) fn is_json_manifest(bytes: &[u8]) -> bool {
+    parse_manifest(bytes).is_some()
+}
+
 fn parse_manifest(bytes: &[u8]) -> Option<serde_json::Value> {
     let bytes = crate::formats::text::strip_bom_bytes(bytes);
     serde_json::from_slice::<serde_json::Value>(bytes).ok()
@@ -346,11 +346,11 @@ mod tests {
                 "{reference}"
             );
         }
-        let unix = CopyRef::Linked("/home/me/lp".into());
-        assert!(unix.read_path().is_none() && unix.is_unread_linked());
-        let relative = CopyRef::Linked("vend/lp".into());
-        assert!(!relative.is_unread_linked());
-        assert!(!CopyRef::Directory("/home/me/lp".into()).is_unread_linked());
+        assert!(CopyRef::Linked("/home/me/lp".into()).read_path().is_none());
+        assert_eq!(
+            CopyRef::Linked("vend/lp".into()).read_path().as_deref(),
+            Some("vend/lp/package.json")
+        );
     }
 
     #[test]

crates/socket-patch-core/src/vendor/yarn_berry_lock.rs

@@ -276,13 +276,14 @@ impl NpmLockBackend for YarnBerryBackend {
                          vendoring writes never reaches it, so that copy stays UNPATCHED"
                     )
                 } else {
-                    // An unread `portal:` / `link:` copy has no version
-                    // to name (#1540).
-                    let what = if copy.source.is_unread_linked() {
-                        "a package".to_string()
-                    } else {
-                        format!("version {version}")
-                    };
+                    // An unidentified `portal:` / `link:` copy has no
+                    // version to name (#1540, #1561).
+                    let what =
+                        if matches!(copy.source, crate::formats::yarn::copy::CopyRef::Linked(_)) {
+                            "a package".to_string()
+                        } else {
+                            format!("version {version}")
+                        };
                     format!(
                         "{YARN_LOCK} entry `{key}` installs {what} under another \
                          dependency name, from {origin}, whose package can't be read, so it \
@@ -2561,6 +2562,70 @@ __metadata:
         }
     }
 
+    /// #1561: a RELATIVE `portal:` / `link:` copy whose target is missing,
+    /// empty (the placeholder git leaves for a submodule CI never checked
+    /// out) or holds a `package.json` that is no JSON (a Git LFS pointer) is
+    /// as unknown as an
+    /// absolute one (#1540): named as a possible unpatched copy, while the
+    /// registry entry still vendors. It was dropped silently. Control: a
+    /// `link:` to an existing plain directory with no `package.json` is no
+    /// package copy and stays unnamed.
+    #[tokio::test]
+    async fn issue_1561_missing_relative_portal_and_link_targets_are_named() {
+        for protocol in ["portal", "link"] {
+            for target in ["missing", "empty", "lfs-pointer", "plain-dir"] {
+                let plain_dir = target == "plain-dir";
+                let key = format!("lp2@{protocol}:./vend/lp::locator=vendor-spike%40workspace%3A.");
+                let block = format!(
+                    "\n\"{key}\":\n  version: 0.0.0-use.local\n  resolution: \"{key}\"\n  \
+                     languageName: node\n  linkType: soft\n"
+                );
+                let lock = format!("{B3_BEFORE_LOCK}{block}");
+                let fx = fixture_with(B3_BEFORE_PKG, &lock).await;
+                let dir = fx.tmp.path().join("vend/lp");
+                match target {
+                    "empty" => tokio::fs::create_dir_all(&dir).await.unwrap(),
+                    "lfs-pointer" => {
+                        tokio::fs::create_dir_all(&dir).await.unwrap();
+                        tokio::fs::write(
+                            dir.join("package.json"),
+                            "version https://git-lfs.github.com/spec/v1\noid sha256:00\nsize 1\n",
+                        )
+                        .await
+                        .unwrap();
+                    }
+                    "plain-dir" => {
+                        tokio::fs::create_dir_all(&dir).await.unwrap();
+                        tokio::fs::write(dir.join("logo.svg"), "<svg/>")
+                            .await
+                            .unwrap();
+                    }
+                    _ => {}
+                }
+
+                let (result, entry, warnings) = expect_done(fx.vendor(false).await);
+                assert!(result.success, "{protocol}/{target}: {:?}", result.error);
+                assert!(entry.is_some(), "{protocol}/{target}: registry vendors");
+                let copies: Vec<&VendorWarning> = warnings
+                    .iter()
+                    .filter(|w| w.code == "vendor_copy_unpatched")
+                    .collect();
+                if plain_dir {
+                    assert!(copies.is_empty(), "{protocol}: {warnings:?}");
+                    continue;
+                }
+                assert_eq!(copies.len(), 1, "{protocol}/{target}: {warnings:?}");
+                assert!(
+                    copies[0].detail.contains(&format!("`\"{key}\"`"))
+                        && copies[0].detail.contains("installs a package under")
+                        && copies[0].detail.contains("can't be read"),
+                    "{protocol}/{target}: {}",
+                    copies[0].detail
+                );
+            }
+        }
+    }
+
     /// #939 (review): when left-pad@1.3.0 is locked ONLY under another
     /// dependency name, vendoring refuses `vendor_lock_entry_not_rewritable`
     /// naming that copy, in the loop and in the download plan alike, not

crates/socket-patch-core/src/vendor/yarn_copies.rs

@@ -13,13 +13,14 @@
 use std::path::Path;
 
 use crate::formats::yarn::blocks::{berry_field, classic_field};
-use crate::formats::yarn::copy::{berry_copy_ref, classic_copy_ref, CopyRef};
+use crate::formats::yarn::copy::{berry_copy_ref, classic_copy_ref, is_json_manifest, CopyRef};
 use crate::formats::yarn::is_berry_lock;
 use crate::formats::yarn::patterns::classic_key_real_name;
 use crate::formats::yarn::source::{classic_copy_source, CopySource};
 use crate::utils::fs::read_regular_to_bytes;
 
 use super::lock_inventory::yarn::{berry_entries, classic_entries};
+use super::lock_inventory::ProjectView;
 
 /// A lock entry under another dependency name that installs, or may
 /// install, `name@version` ([`other_name_copies`]).
@@ -42,9 +43,10 @@ pub(crate) struct OtherNameCopy {
 /// that version whose package cannot be read is kept, unidentified: it
 /// may hold `name`, and leaving it out would let the scan and `vex` vouch
 /// for a lock that installs it unpatched (#1516). So is a `portal:` /
-/// `link:` copy that is never read (an absolute path): its version is its
-/// manifest's, so it may be `version` too (#1540). One read and found to
-/// hold no versioned package (a `link:` to a plain directory) is left out.
+/// `link:` copy whose package is unknown ([`linked_copy_unknown`]): its
+/// version is its manifest's, so it may be `version` too (#1540, #1561).
+/// One that holds no versioned package (a `link:` to a plain directory) is
+/// left out.
 pub(crate) async fn other_name_copies(
     project_root: &Path,
     lock_text: &str,
@@ -107,10 +109,15 @@ pub(crate) async fn other_name_copies(
             Some(rel) => read_regular_to_bytes(&project_root.join(rel)).await.ok(),
             None => None,
         };
+        let parsed = bytes.as_deref().is_some_and(is_json_manifest);
         let identified = match copy_identity(&source, &lock_version, bytes).await {
             Some((n, v)) if n == name && v == version => true,
             Some(_) => continue,
-            None if matches!(source, CopyRef::Linked(_)) && !source.is_unread_linked() => continue,
+            None if matches!(source, CopyRef::Linked(_))
+                && !linked_copy_unknown(ProjectView::Disk(project_root), &source, parsed).await =>
+            {
+                continue
+            }
             None => false,
         };
         out.push(OtherNameCopy {
@@ -122,6 +129,39 @@ pub(crate) async fn other_name_copies(
     out
 }
 
+/// Whether a `portal:` / `link:` copy whose manifest named no versioned
+/// package may be any version of any package. Its lock version is the
+/// placeholder `0.0.0-use.local`, so only its `package.json` says what it
+/// is; `manifest_parsed` says that file was read and parsed as JSON. A copy
+/// is unknown when it is never read (an absolute path, #1540) or its
+/// target can't be read (#1561): a missing directory, an EMPTY one (the
+/// placeholder git leaves for a submodule CI never checked out), or a
+/// `package.json` that is no readable regular file or no JSON (a Git LFS
+/// pointer, an encrypted blob). Only a non-empty directory with no
+/// `package.json` at all (a `link:` to plain assets), or a manifest parsed
+/// and found to name no versioned package, is no package copy.
+pub(crate) async fn linked_copy_unknown(
+    view: ProjectView<'_>,
+    source: &CopyRef,
+    manifest_parsed: bool,
+) -> bool {
+    let CopyRef::Linked(dir) = source else {
+        return false;
+    };
+    if manifest_parsed {
+        return false;
+    }
+    let Some(manifest) = source.read_path() else {
+        return true;
+    };
+    if !view.is_dir(dir) || view.exists_no_follow(&manifest).await {
+        return true;
+    }
+    view.list_dir(dir)
+        .await
+        .map_or(true, |entries| entries.is_empty())
+}
+
 /// [`CopyRef::identity`] off the async runtime when it unpacks a tarball (a
 /// user's `file:` tarball can be large); lockfile discovery reads copies
 /// through it too.