Fix berry restore keeping removed descriptors (#1435)

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

@@ -5408,6 +5408,95 @@ async fn yarn_berry_unwind_merges_into_a_registry_entry_added_after_pinning() {
     }
 }
 
+/// #1435: a hosted berry pin routes each descriptor through its own
+/// `resolutions` selector. `yarn remove` in one workspace member, while
+/// another still uses the package, keeps the URL-keyed entry and both
+/// selectors. `rollback` and `remove` must rebuild the restored key from
+/// the descriptors the lock still declares, not from every selector, or
+/// every `yarn install --immutable` fails YN0028. A descriptor another
+/// entry still declares stays in the key.
+#[tokio::test]
+#[serial]
+async fn yarn_berry_unwind_drops_a_descriptor_no_entry_declares() {
+    let server = MockServer::start().await;
+    mock_discovery(&server).await;
+    let hosted_url = HOSTED_URL.replace("http://patch.test", &server.uri());
+    mock_reference_with_berry_url(&server, &hosted_url).await;
+    mock_view(&server).await;
+    mock_npm_registry_advertising(
+        &server,
+        &vlt_hosted_common::sha512_sri(&upstream_tarball()),
+        &format!(
+            "{}/npm-registry/{NAME}/-/{NAME}-{VERSION}.tgz",
+            server.uri()
+        ),
+    )
+    .await;
+    // A workspace member that declares the exact version: its entry is
+    // yarn's record that the second descriptor is still in use.
+    let member = format!(
+        "\n\"member@workspace:packages/member\":\n  version: 0.0.0-use.local\n  \
+         resolution: \"member@workspace:packages/member\"\n  dependencies:\n    \
+         {NAME}: \"npm:{VERSION}\"\n  languageName: unknown\n  linkType: soft\n"
+    );
+    for (declared, key) in [
+        (false, format!("\"{NAME}@npm:^{VERSION}\":")),
+        (
+            true,
+            format!("\"{NAME}@npm:{VERSION}, {NAME}@npm:^{VERSION}\":"),
+        ),
+    ] {
+        for command in ["rollback", "remove"] {
+            let tmp = tempfile::tempdir().unwrap();
+            write_berry_project(tmp.path());
+            let env = run_redirect_subprocess_with(
+                tmp.path(),
+                &server.uri(),
+                &["--patch-server-url", &server.uri()],
+            );
+            assert_eq!(vlt_hosted_common::redirected(&env), 1, "{env:#}");
+            // The selector a since-removed member's descriptor left behind.
+            let pkg_path = tmp.path().join("package.json");
+            let mut pkg: serde_json::Value =
+                serde_json::from_str(&std::fs::read_to_string(&pkg_path).unwrap()).unwrap();
+            pkg["resolutions"][format!("{NAME}@npm:{VERSION}")] =
+                serde_json::Value::String(hosted_url.clone());
+            let lock_path = tmp.path().join("yarn.lock");
+            if declared {
+                pkg["workspaces"] = serde_json::json!(["packages/*"]);
+                let dir = tmp.path().join("packages").join("member");
+                std::fs::create_dir_all(&dir).unwrap();
+                std::fs::write(
+                    dir.join("package.json"),
+                    format!(
+                        r#"{{ "name": "member", "version": "0.0.0", "dependencies": {{ "{NAME}": "{VERSION}" }} }}"#
+                    ),
+                )
+                .unwrap();
+                let pinned = std::fs::read_to_string(&lock_path).unwrap();
+                std::fs::write(&lock_path, format!("{pinned}{member}")).unwrap();
+            }
+            std::fs::write(&pkg_path, serde_json::to_string_pretty(&pkg).unwrap()).unwrap();
+
+            let args: &[&str] = if command == "rollback" {
+                &["rollback"]
+            } else {
+                &["remove", PURL]
+            };
+            let (code, env) = hosted_unwind_json(tmp.path(), &server, &server.uri(), args);
+            assert_eq!(code, Some(0), "{command} declared={declared}: {env:#}");
+            let restored = std::fs::read_to_string(&lock_path).unwrap();
+            assert!(
+                restored.contains(&format!("\n{key}\n")),
+                "{command} declared={declared}: restored key must be {key}:\n{restored}"
+            );
+            assert!(!restored.contains(&hosted_url), "{restored}");
+            let pkg = std::fs::read_to_string(&pkg_path).unwrap();
+            assert!(!pkg.contains("resolutions"), "{pkg}");
+        }
+    }
+}
+
 /// #908: the restore reads `dist.tarball` from the registry the project
 /// resolves against (`.yarnrc.yml` `npmRegistryServer`), not from the
 /// default registry. A mirror whose tarball URLs are off the conventional

crates/socket-patch-core/src/patch/redirect/upstream/npm.rs

@@ -757,6 +757,8 @@ async fn restore_berry(
         url_pin: bool,
     }
     let mut hits: Vec<Hit> = Vec::new();
+    // What the lock still declares, read once for the URL-keyed hits.
+    let mut declared: Option<BTreeSet<String>> = None;
     for (i, block) in blocks.iter().enumerate() {
         let lines = stanza_lines(block);
         let Some(resolution) = berry_field(&lines, "resolution").map(str::to_string) else {
@@ -825,6 +827,20 @@ async fn restore_berry(
                 })
                 .cloned()
                 .collect();
+            // Only descriptors some entry still declares key the restored
+            // entry: `yarn remove` in one workspace member leaves its
+            // selector behind while another member keeps the URL entry
+            // live, and yarn keys the entry by what is declared (#1435).
+            // The selectors are all dropped below either way. A lock that
+            // declares none of them (a spelling not read here) keeps every
+            // selector's descriptor, as before.
+            let declared = declared.get_or_insert_with(|| berry_declared_descriptors(&blocks));
+            if descriptors
+                .iter()
+                .any(|d| berry_descriptor_declared(declared, d))
+            {
+                descriptors.retain(|d| berry_descriptor_declared(declared, d));
+            }
             if descriptors.is_empty() {
                 result.refuse(
                     &uuid,
@@ -1051,6 +1067,52 @@ async fn restore_berry(
     view.write(rel, doc.render(&moved));
 }
 
+/// The descriptors a berry lock's `stanzas` still declare: every
+/// entry's `dependencies:` map, plus the descriptor a `patch:` descriptor
+/// wraps (`name@patch:name@npm%3A1.3.0#…` wraps `name@npm:1.3.0`), in
+/// either a dependency map or a key. Yarn 4 spells a registry range
+/// `npm:range`, yarn 2 and 3 a bare `range`; see
+/// [`berry_descriptor_declared`].
+fn berry_declared_descriptors(stanzas: &[String]) -> BTreeSet<String> {
+    use crate::formats::yarn::berry_prune::dependency_descriptors;
+    use crate::formats::yarn::patterns::split_berry_key_patterns;
+    use crate::formats::yarn::stanzas::stanza_key;
+
+    let mut out = BTreeSet::new();
+    for stanza in stanzas {
+        let keys = stanza_key(stanza)
+            .map(split_berry_key_patterns)
+            .unwrap_or_default();
+        for descriptor in dependency_descriptors(stanza) {
+            if let Some(inner) = berry_patch_inner(&descriptor) {
+                out.insert(inner);
+            }
+            out.insert(descriptor);
+        }
+        out.extend(keys.iter().filter_map(|k| berry_patch_inner(k)));
+    }
+    out
+}
+
+/// The descriptor a `name@patch:<inner>#<patch>` descriptor wraps, with
+/// yarn's percent-encoding of the inner `:` undone.
+fn berry_patch_inner(descriptor: &str) -> Option<String> {
+    let (_, rest) = descriptor.split_once("@patch:")?;
+    let inner = rest.split('#').next().unwrap_or(rest);
+    Some(inner.replace("%3A", ":"))
+}
+
+/// Whether the `name@npm:range` `descriptor` is in `declared`
+/// ([`berry_declared_descriptors`]), in yarn 4's spelling or yarn 2/3's
+/// bare `name@range`.
+fn berry_descriptor_declared(declared: &BTreeSet<String>, descriptor: &str) -> bool {
+    use crate::formats::yarn::patterns::split_pattern;
+    declared.contains(descriptor)
+        || split_pattern(descriptor)
+            .and_then(|(name, range)| Some(format!("{name}@{}", range.strip_prefix("npm:")?)))
+            .is_some_and(|bare| declared.contains(&bare))
+}
+
 /// Retire the leftover hosted `resolutions` selectors of a yarn berry root
 /// `package.json` (#1203): `yarn remove` / `yarn up` deleted the lock entry
 /// a hosted pin keyed by its tarball URL but left its selector, which now
@@ -2723,6 +2785,55 @@ mod tests {
     };
     use crate::patch::redirect::upstream::client::NpmDist;
 
+    /// #1435: what still declares a descriptor the hosted restore keys
+    /// the registry entry by.
+    #[test]
+    fn berry_descriptor_declared_reads_what_the_lock_still_uses() {
+        let declared = |stanzas: &[&str], d: &str| {
+            let stanzas: Vec<String> = stanzas.iter().map(|s| s.to_string()).collect();
+            super::berry_descriptor_declared(&super::berry_declared_descriptors(&stanzas), d)
+        };
+        let url_entry = "\"left-pad@http://patch.test/left-pad-1.3.0.tgz\":\n  version: 1.3.0\n  \
+                         resolution: \"left-pad@http://patch.test/left-pad-1.3.0.tgz\"";
+        let member = |dep: &str| {
+            format!(
+                "\"b@workspace:packages/b\":\n  version: 0.0.0-use.local\n  dependencies:\n    \
+                 {dep}\n  linkType: soft"
+            )
+        };
+        // Yarn 4 spells the range `npm:range`; yarn 2 and 3 a bare one.
+        let yarn4 = member("left-pad: \"npm:^1.3.0\"");
+        let yarn3 = member("left-pad: ^1.3.0");
+        assert!(declared(&[url_entry, &yarn4], "left-pad@npm:^1.3.0"));
+        assert!(declared(&[url_entry, &yarn3], "left-pad@npm:^1.3.0"));
+        // The member a `yarn remove` emptied declares nothing.
+        assert!(!declared(&[url_entry, &yarn4], "left-pad@npm:1.3.0"));
+        assert!(!declared(&[url_entry, &yarn3], "left-pad@npm:1.3.0"));
+        // A `yarn patch` of the descriptor still uses it, in a dependency
+        // map or a key.
+        let patch = "left-pad@patch:left-pad@npm%3A1.3.0#~/.yarn/patches/x.patch";
+        let patched = format!("\"{patch}\":\n  version: 1.3.0");
+        assert!(declared(&[url_entry, &patched], "left-pad@npm:1.3.0"));
+        let patch_dep = member(&format!("left-pad: \"{}\"", &patch["left-pad@".len()..]));
+        assert!(declared(&[url_entry, &patch_dep], "left-pad@npm:1.3.0"));
+        // An alias, a scoped namesake and a longer range declare other
+        // descriptors.
+        for other in [
+            member("lp: \"npm:[email protected]\""),
+            member("\"@acme/left-pad\": \"npm:1.3.0\""),
+            member("left-pad: \"npm:1.3.0-rc.1\""),
+            "\"@acme/left-pad@patch:@acme/left-pad@npm%3A1.3.0#x.patch\":\n  version: 1.3.0"
+                .to_string(),
+            "\"left-pad@patch:left-pad@npm%3A1.3.0-rc.1#x.patch\":\n  version: 1.3.0-rc.1"
+                .to_string(),
+        ] {
+            assert!(
+                !declared(&[url_entry, &other], "left-pad@npm:1.3.0"),
+                "{other}"
+            );
+        }
+    }
+
     fn files(npmrc: &[(&str, bool)], bunfig: &[(&str, bool)]) -> super::BunConfigFiles {
         let layer = |list: &[(&str, bool)]| {
             list.iter()