Fix yarn berry absolute portal:/link: copies (#1540)

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

@@ -1879,3 +1879,65 @@ async fn issue_939_berry_renamed_copy_is_named() {
         }
     }
 }
+
+/// #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
+/// scan still pins the registry entry, and names that copy as possibly
+/// unpatched (`redirect_copy_unpatched`) whatever it links to; it used to
+/// 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 = copy_unpatched_details(project);
+        assert_eq!(details.len(), 1, "{protocol}: {:#}", project.redirect);
+        assert!(
+            details[0].contains(key.as_str())
+                && details[0].contains("can't be read")
+                && details[0].contains("UNPATCHED"),
+            "{protocol}: {}",
+            details[0]
+        );
+    }
+}

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

@@ -58,6 +58,13 @@ 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
@@ -151,7 +158,10 @@ pub(crate) fn berry_copy_ref(reference: &str) -> Option<CopyRef> {
     } else {
         return None;
     };
-    let path = path.split(['#', ':']).next().unwrap_or_default();
+    // Cut the `::locator=…` bindings and any `#` selector; a single `:` is
+    // part of the path (a Windows drive, `portal:C:/Users/me/lp`).
+    let path = path.split("::").next().unwrap_or_default();
+    let path = path.split('#').next().unwrap_or_default();
     let rel = berry_copy_path(reference, path)?;
     Some(if linked {
         CopyRef::Linked(rel)
@@ -320,6 +330,29 @@ mod tests {
         assert_eq!(berry_copy_ref("workspace:packages/b"), None);
     }
 
+    /// #1540: an absolute `portal:` / `link:` path keeps its whole spelling,
+    /// a Windows drive's `:` included, and is never read.
+    #[test]
+    fn absolute_linked_refs_keep_the_path_and_are_unread() {
+        let ws = "::locator=app%40workspace%3A.";
+        for (reference, path) in [
+            (format!("portal:/home/me/lp{ws}"), "/home/me/lp"),
+            (format!("link:/home/me/lp{ws}"), "/home/me/lp"),
+            (format!("portal:C:/Users/me/lp{ws}"), "C:/Users/me/lp"),
+        ] {
+            assert_eq!(
+                berry_copy_ref(&reference),
+                Some(CopyRef::Linked(path.into())),
+                "{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());
+    }
+
     #[test]
     fn linked_identity_reads_name_and_version_from_the_manifest() {
         let linked = CopyRef::Linked("vend/lp".into());

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

@@ -275,8 +275,15 @@ 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}")
+                    };
                     format!(
-                        "{YARN_LOCK} entry `{key}` installs version {version} under another \
+                        "{YARN_LOCK} entry `{key}` installs {what} under another \
                          dependency name, from {origin}, whose package can't be read, so it \
                          may be {name}@{version}; the name-keyed resolutions entry vendoring \
                          writes never reaches it, so if it is, that copy stays UNPATCHED"
@@ -2581,6 +2588,51 @@ __metadata:
         }
     }
 
+    /// #1540: a `portal:` / `link:` copy with an ABSOLUTE path is never read
+    /// (#1516), so its package and version are unknown. It is named as a
+    /// possible unpatched copy (`vendor_copy_unpatched`, unidentified
+    /// wording) whatever its target holds, never taken as proof the package
+    /// is installed, and the registry entry still vendors. It was dropped
+    /// silently.
+    #[tokio::test]
+    async fn issue_1540_absolute_portal_and_link_copies_are_named() {
+        for protocol in ["portal", "link"] {
+            for fork in ["left-pad", "other-pkg"] {
+                let ext = tempfile::tempdir().unwrap();
+                tokio::fs::write(
+                    ext.path().join("package.json"),
+                    format!(r#"{{"name":"{fork}","version":"1.3.0"}}"#),
+                )
+                .await
+                .unwrap();
+                let abs = ext.path().to_string_lossy().replace('\\', "/");
+                let key = format!("lp2@{protocol}:{abs}::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 (result, entry, warnings) = expect_done(fx.vendor(false).await);
+                assert!(result.success, "{protocol}/{fork}: {:?}", result.error);
+                assert!(entry.is_some(), "{protocol}/{fork}: registry entry vendors");
+                let copies: Vec<&VendorWarning> = warnings
+                    .iter()
+                    .filter(|w| w.code == "vendor_copy_unpatched")
+                    .collect();
+                assert_eq!(copies.len(), 1, "{protocol}/{fork}: {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}/{fork}: {}",
+                    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

@@ -30,19 +30,21 @@ pub(crate) struct OtherNameCopy {
     pub(crate) source: CopyRef,
     /// The copy itself says it is `name@version`. `false` for a copy of
     /// that version whose package cannot be read (a missing directory, a
-    /// tarball with no readable `package.json`, a non-registry url): it may
-    /// be `name`, so it is named, but never taken as proof that `name` is
-    /// installed (#1516).
+    /// tarball with no readable `package.json`, a non-registry url, or an
+    /// unread `portal:` / `link:` directory of any version): it may be
+    /// `name`, so it is named, but never taken as proof that `name` is
+    /// installed (#1516, #1540).
     pub(crate) identified: bool,
 }
 
 /// Every live entry of `lock_text` (either grammar) that installs
 /// `name@version` under another dependency name, in lock order. A copy of
 /// 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). A `portal:` / `link:`
-/// copy whose manifest cannot be read is left out: its version is that
-/// manifest's, so nothing ties it to `version`.
+/// 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.
 pub(crate) async fn other_name_copies(
     project_root: &Path,
     lock_text: &str,
@@ -108,7 +110,7 @@ pub(crate) async fn other_name_copies(
         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(_)) => continue,
+            None if matches!(source, CopyRef::Linked(_)) && !source.is_unread_linked() => continue,
             None => false,
         };
         out.push(OtherNameCopy {

crates/socket-patch-core/src/vex/discover/mod.rs

@@ -435,15 +435,17 @@ pub struct UnpatchedCopy {
     pub how: String,
 }
 
-/// A lock entry that installs SOME package at `version` under another
-/// dependency name, from a copy whose package can't be read
-/// ([`Discovery::unidentified_copy`], #1516).
+/// A lock entry that installs SOME package (at `version`, when known) under
+/// another dependency name, from a copy whose package can't be read
+/// ([`Discovery::unidentified_copy`], #1516, #1540).
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct UnidentifiedCopy {
     /// Root-relative lock file.
     pub file: PathBuf,
-    /// The lock entry's version.
-    pub version: String,
+    /// The lock entry's version; `None` when the lock does not say (a yarn
+    /// berry `portal:` / `link:` copy, whose version is its unread
+    /// manifest's), so it may be any version (#1540).
+    pub version: Option<String>,
     /// The lock entry's key, as the lock spells it.
     pub key: String,
     /// How that entry installs, completing "lock entry `<key>` …".
@@ -892,39 +894,41 @@ impl Discovery {
     /// directory, a tarball with no readable `package.json`). It may be any
     /// package at that version, so [`Discovery::contest_within_locks`]
     /// takes it as an unpatched copy of every package `file` wires at
-    /// `version`: vouching for one would rest on a copy nobody read
-    /// (#1516). Matched there, once every extractor has pushed its refs.
-    pub(crate) fn unidentified_copy(&mut self, file: &str, version: &str, key: &str, how: &str) {
+    /// `version` (at any version when `version` is `None`, #1540): vouching
+    /// for one would rest on a copy nobody read (#1516). Matched there,
+    /// once every extractor has pushed its refs.
+    pub(crate) fn unidentified_copy(
+        &mut self,
+        file: &str,
+        version: Option<&str>,
+        key: &str,
+        how: &str,
+    ) {
         self.unidentified_copies.push(UnidentifiedCopy {
             file: PathBuf::from(file),
-            version: version.to_string(),
+            version: version.map(str::to_string),
             key: key.to_string(),
             how: how.to_string(),
         });
     }
 
     /// Fold [`Discovery::unidentified_copies`] into `unpatched_copies`: one
-    /// copy per ref of the same file at the same version.
+    /// copy per ref of the same file at the same version (at any version
+    /// for a copy with none).
     fn expand_unidentified_copies(&mut self) {
-        if self.unidentified_copies.is_empty() {
-            return;
-        }
-        let mut wired: BTreeMap<(&Path, &str), BTreeSet<&str>> = BTreeMap::new();
-        for r in &self.refs {
-            if let Some((_, version)) = r.purl.rsplit_once('@') {
-                wired
-                    .entry((r.source_file.as_path(), version))
-                    .or_default()
-                    .insert(r.purl.as_str());
-            }
-        }
         let mut expanded = Vec::new();
         for u in &self.unidentified_copies {
-            for purl in wired
-                .get(&(u.file.as_path(), u.version.as_str()))
-                .into_iter()
-                .flatten()
-            {
+            let purls: BTreeSet<&str> = self
+                .refs
+                .iter()
+                .filter(|r| r.source_file == u.file)
+                .filter(|r| match &u.version {
+                    Some(v) => r.purl.rsplit_once('@').is_some_and(|(_, rv)| rv == v),
+                    None => true,
+                })
+                .map(|r| r.purl.as_str())
+                .collect();
+            for purl in purls {
                 expanded.push(UnpatchedCopy {
                     purl: purl.to_string(),
                     file: u.file.clone(),