Fix vendored berry revert restoring stale lock (#759)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -1534,6 +1534,7 @@ command's integration tests check its output.
 | `pypi_pipenv_installer_unknown` | `skipped` (warning) | vendor (pipenv): no `pipenv` answered on PATH; the vendored references assume Pipenv 2018 or later (7–11 cannot consume them — use hosted mode there); `SOCKET_PIPENV_MAJOR` pins the release. |
 | `vendor_lock_entry_relocked` | revert `warnings[]` | vendor `--revert` / rollback (pipenv): a relock regenerated the wired entry to a registry reference, or removed it; the record is retired (artifact removed, ledger entry dropped) instead of drift-kept. |
 | `vendor_lock_entry_pruned_kept` | revert `warnings[]` | vendor `--revert` / rollback (yarn berry, #737): vendoring dropped a lock entry only the patched package's implicit `node-gyp` dependency reached (or one descriptor of a shared entry), and the lock has re-added or changed that entry since; it is left as it is (not drift: the artifact is still removed). Run `yarn install` if the lock is incomplete. |
+| `vendor_lock_descriptor_unlocked` | revert `warnings[]` | vendor `--revert` / rollback / remove (yarn berry, #759): a descriptor of the vendored package was added while it was vendored, and the restored registry version does not satisfy it (or its range is one socket-patch cannot read, such as a dist-tag), so it has no `yarn.lock` entry after the revert. The revert still succeeds and the artifact is removed. Run `yarn install` and commit `yarn.lock`. |
 | `pypi_{poetry,pdm,pipenv}_no_lockfile` | `failed` | vendor (pypi): a lock-less tool marker with no `requirements.txt` fallback — run `<tool> lock`. |
 | `pypi_poetry_integrity_unverified` | `skipped` (warning) | vendor (pypi / poetry): the lock was written by Poetry < 1.4 (0.12 `[metadata.hashes]`, lock 1.0/1.1, or a 2.0 lock without a `@generated by Poetry X.Y.Z` header — 1.3 wrote those). That installer does not verify local wheel hashes (the committed wheel bytes are the protection) and does not replace an already-installed package at the same version; recreate the virtualenv or `pip uninstall` the package before `poetry install`, or upgrade Poetry. |
 | `pypi_poetry_source_constraint` | `failed` / `skipped` (warning) | vendor (pypi / poetry): the pyproject pins the package to a named source (`source = "pypi"`), which Poetry records nothing for in the lock. Over a lock stamped by Poetry 1.4 – 2.2 (whose installer refuses that constraint on a file-sourced package, so every `poetry install` would fail) the package is refused before any write, `failed`; an already-wired lock is refused the same way instead of reading as in sync. Over any other lock the wiring stands and this is a warning naming the 1.4 – 2.2 range. The detail names the remedy: drop the `source` key or upgrade to Poetry >= 2.3, re-lock and re-run; when the lock is already wired it first says to run `socket-patch vendor --revert` (which reverts EVERY vendored package), since re-locking discards the wiring. |

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

@@ -881,3 +881,178 @@ fn yarn_berry_vendor_refuses_a_gitignored_socket_dir() {
     );
     assert!(!proj.join(format!(".socket/vendor/npm/{UUID}")).exists());
 }
+
+// ── #759: revert after the lock moved on ──────────────────────────────
+
+/// What changes in the project while left-pad is vendored, before the
+/// revert.
+#[derive(Clone, Copy, Debug)]
+enum LockChange {
+    /// Member `b` starts depending on `left-pad@1.3.0` beside the root's
+    /// `^1.3.0`.
+    MemberAddsDescriptor,
+    /// Member `b` stops depending on its `left-pad@1.3.0`; the root keeps
+    /// `^1.3.0`.
+    MemberRemovesDescriptor,
+    /// Vendored under yarn 4.0.2 (bare checksums), then installed with a
+    /// 4.1+ yarn, which respells every checksum `10c0/<hex>`.
+    YarnUpgrade,
+}
+
+/// #759: `vendor --revert` after the lock changed since vendoring leaves a
+/// lock a fresh checkout installs with `yarn install --immutable`. The
+/// revert used to write the recorded pre-vendor entry back verbatim (its
+/// old key, its old checksum spelling), and yarn rejected it (YN0028).
+#[test]
+fn issue_759_revert_after_a_member_adds_a_descriptor() {
+    run_revert_after_lock_change(LockChange::MemberAddsDescriptor);
+}
+
+#[test]
+fn issue_759_revert_after_a_member_removes_a_descriptor() {
+    run_revert_after_lock_change(LockChange::MemberRemovesDescriptor);
+}
+
+#[test]
+fn issue_759_revert_after_a_yarn_upgrade_respells_checksums() {
+    run_revert_after_lock_change(LockChange::YarnUpgrade);
+}
+
+fn run_revert_after_lock_change(change: LockChange) {
+    // The upgrade case pins its own pair of releases: the last bare-hex
+    // writer, then the one under test (or the first prefixed writer when
+    // that is a bare-hex one itself).
+    let (vendor_yarn, after_yarn) = match change {
+        LockChange::YarnUpgrade => (
+            "yarn@4.0.2",
+            match yarn_berry() {
+                "yarn@4.0.0" | "yarn@4.0.1" | "yarn@4.0.2" => "yarn@4.1.0",
+                current => current,
+            },
+        ),
+        _ => (yarn_berry(), yarn_berry()),
+    };
+    for pm in [vendor_yarn, after_yarn] {
+        if !has_corepack_pm(pm) {
+            skip!(
+                "SKIP e2e_vendor_yarn_berry_build (#759 {change:?}): `corepack {pm}` unavailable"
+            );
+            return;
+        }
+    }
+    let tmp = tempfile::tempdir().unwrap();
+    let proj = tmp.path().join("proj");
+    let global = tmp.path().join("yarn-global");
+    let yarn = |pm: &str, dir: &Path, args: &[&str]| {
+        let out = corepack(
+            dir,
+            pm,
+            args,
+            &[("YARN_GLOBAL_FOLDER", global.to_str().unwrap())],
+        );
+        (out.status.success(), yarn_berry_common::yarn_output(&out))
+    };
+    let member = |dep: Option<&str>| {
+        let deps = dep
+            .map(|r| format!(r#","dependencies":{{"{DEP}":"{r}"}}"#))
+            .unwrap_or_default();
+        std::fs::write(
+            proj.join("packages/b/package.json"),
+            format!(r#"{{"name":"b","version":"1.0.0"{deps}}}"#),
+        )
+        .unwrap();
+    };
+    std::fs::create_dir_all(proj.join("packages/b")).unwrap();
+    std::fs::write(
+        proj.join("package.json"),
+        format!(
+            r#"{{"name":"root","version":"0.0.0","private":true,"workspaces":["packages/*"],"dependencies":{{"{DEP}":"^{DEP_VERSION}"}}}}"#
+        ),
+    )
+    .unwrap();
+    std::fs::write(
+        proj.join(".yarnrc.yml"),
+        "nodeLinker: node-modules\nenableGlobalCache: false\n",
+    )
+    .unwrap();
+    member(match change {
+        LockChange::MemberRemovesDescriptor => Some(DEP_VERSION),
+        _ => None,
+    });
+    let (ok, out) = yarn(vendor_yarn, &proj, &["install"]);
+    if !ok {
+        skip!("SKIP e2e_vendor_yarn_berry_build (#759 {change:?}): fixture install failed:\n{out}");
+        return;
+    }
+
+    let orig = std::fs::read(proj.join("node_modules").join(DEP).join("index.js")).unwrap();
+    let patched: Vec<u8> = [MARKER.as_bytes(), orig.as_slice()].concat();
+    stage_patch(
+        &proj,
+        &format!("pkg:npm/{DEP}@{DEP_VERSION}"),
+        "package/index.js",
+        &orig,
+        &patched,
+    );
+    let cwd = proj.to_str().unwrap();
+    let (code, stdout, stderr) =
+        run_socket(&proj, &["vendor", "--json", "--offline", "--cwd", cwd]);
+    assert_eq!(
+        code, 0,
+        "vendor failed.\nstdout:\n{stdout}\nstderr:\n{stderr}"
+    );
+
+    match change {
+        LockChange::MemberAddsDescriptor => member(Some(DEP_VERSION)),
+        LockChange::MemberRemovesDescriptor => member(None),
+        LockChange::YarnUpgrade => {}
+    }
+    let (ok, out) = yarn(after_yarn, &proj, &["install"]);
+    assert!(ok, "{change:?}: install while vendored failed:\n{out}");
+    let vendored = std::fs::read_to_string(proj.join("yarn.lock")).unwrap();
+    if let LockChange::YarnUpgrade = change {
+        assert!(
+            vendored.contains("checksum: 10c0/"),
+            "{after_yarn} respells the vendored lock's checksums:\n{vendored}"
+        );
+    }
+
+    let (code, stdout, stderr) = run_socket(&proj, &["vendor", "--revert", "--json", "--cwd", cwd]);
+    assert_eq!(
+        code, 0,
+        "revert failed.\nstdout:\n{stdout}\nstderr:\n{stderr}"
+    );
+    let reverted = std::fs::read_to_string(proj.join("yarn.lock")).unwrap();
+
+    // Fresh checkout of the committable files, empty global folder.
+    let fresh = tmp.path().join("fresh");
+    std::fs::create_dir_all(fresh.join("packages/b")).unwrap();
+    for file in [
+        "package.json",
+        "yarn.lock",
+        ".yarnrc.yml",
+        "packages/b/package.json",
+    ] {
+        std::fs::copy(proj.join(file), fresh.join(file)).unwrap();
+    }
+    let out = corepack(
+        &fresh,
+        after_yarn,
+        &["install", "--immutable"],
+        &[(
+            "YARN_GLOBAL_FOLDER",
+            tmp.path().join("fresh-global").to_str().unwrap(),
+        )],
+    );
+    assert!(
+        out.status.success(),
+        "{change:?}: the reverted lock must pass `{after_yarn} install --immutable`:\n{}\n\
+         reverted yarn.lock:\n{reverted}",
+        yarn_berry_common::yarn_output(&out)
+    );
+    assert_eq!(
+        std::fs::read(fresh.join("node_modules").join(DEP).join("index.js")).unwrap(),
+        orig,
+        "the reverted project installs the registry bytes"
+    );
+}

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

@@ -25,6 +25,47 @@ pub(crate) struct Pruned {
     pub after: Option<String>,
 }
 
+/// 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
+/// [`descriptor_declared`].
+pub(crate) fn declared_descriptors(stanzas: &[String]) -> BTreeSet<String> {
+    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) = patch_inner(&descriptor) {
+                out.insert(inner);
+            }
+            out.insert(descriptor);
+        }
+        out.extend(keys.iter().filter_map(|k| patch_inner(k)));
+    }
+    out
+}
+
+/// The descriptor a `name@patch:<inner>#<patch>` descriptor wraps, with
+/// yarn's percent-encoding of the inner `:` undone.
+fn 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`
+/// ([`declared_descriptors`]), in yarn 4's spelling or yarn 2/3's
+/// bare `name@range`.
+pub(crate) fn descriptor_declared(declared: &BTreeSet<String>, descriptor: &str) -> bool {
+    declared.contains(descriptor)
+        || split_pattern(descriptor)
+            .and_then(|(name, range)| Some(format!("{name}@{}", range.strip_prefix("npm:")?)))
+            .is_some_and(|bare| declared.contains(&bare))
+}
+
 /// The descriptors (`name@range`) an entry's `dependencies:` map names.
 pub(crate) fn dependency_descriptors(stanza: &str) -> Vec<String> {
     let mut out = Vec::new();

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

@@ -794,12 +794,16 @@ async fn restore_berry(
             // 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));
+            let declared = declared.get_or_insert_with(|| {
+                crate::formats::yarn::berry_prune::declared_descriptors(&blocks)
+            });
             if descriptors
                 .iter()
-                .any(|d| berry_descriptor_declared(declared, d))
+                .any(|d| crate::formats::yarn::berry_prune::descriptor_declared(declared, d))
             {
-                descriptors.retain(|d| berry_descriptor_declared(declared, d));
+                descriptors.retain(|d| {
+                    crate::formats::yarn::berry_prune::descriptor_declared(declared, d)
+                });
             }
             if descriptors.is_empty() {
                 result.refuse(
@@ -1027,52 +1031,6 @@ 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
@@ -2788,7 +2746,10 @@ mod tests {
     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)
+            crate::formats::yarn::berry_prune::descriptor_declared(
+                &crate::formats::yarn::berry_prune::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\"";