Fix yarn classic check of dangling descriptors (#1379)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -1464,7 +1464,7 @@ Every `--json` invocation emits a single JSON object that follows the **unified
 | `vendor_vlt_reinstall_required` | `skipped` (advisory; human: `Warning: …`) | vendor / scan / get `--mode vendored` (vlt), wet and dry runs, and in-sync reruns: (a) the run rewires an optional dependency, or an importer's `node_modules/<name>` of an optional dependency still resolves into `node_modules/.vlt/`: from vlt 0.0.0-30 a plain `vlt install` (1.2.0: also `--force`) keeps that installed upstream copy linked; the detail says to run `vlt ci` (or delete `node_modules` and run `vlt install`) to link the vendored copy, and that vlt 0.0.0-30 … 1.0.4 install no optional dependency from the lock of a project that declares only optional dependencies (upgrade to 1.0.5 or later first); (b) otherwise, an importer's link of the dependency still resolves into `node_modules/.vlt/`: the detail names the links (`node_modules/<name>`, `<member dir>/node_modules/<name>`) and says `vlt install` (or `vlt ci`) links the vendored copy — on a warm tree after a plain `vlt install` that is true of every vendored direct dependency; (c) an importer's link resolves into the vendored dir of the patch this run replaces (a new patch uuid), which the run removes: the detail names the links and says `vlt install` (or `vlt ci`) links the new vendored copy; (d) a redownload of the payload (vendor, or `repair` after a corrupt or missing payload) could not keep vlt's links to the package's own dependencies (its old `node_modules/` held more than links): the detail says to run `vlt ci` (or delete `node_modules` and run `vlt install`), since a plain `vlt install` does not re-link them. `repair` moves those links back into the downloaded payload when they are only links. The package is vendored either way; a run whose patch fails to apply emits neither. A wet `vendor --revert` (and the revert a vendored → hosted takeover runs, whose advisory joins `warnings[]`): (a) the revert moves an `optionalDependencies` spec back from the `file:` dir, or an optional importer's `node_modules/<name>` still resolves into the vendored uuid dir: from vlt 0.0.0-30 a plain `vlt install` keeps that link (dangling once the dir is removed), so the detail says to run `vlt ci` (or delete `node_modules` and run `vlt install`) to link the restored copy, with the same vlt 1.0.5 note; (b) otherwise, an importer's link still resolves into the vendored uuid dir: the detail names the links and says `vlt install` (or `vlt ci`) links the restored copy. A dry-run revert emits neither. |
 | `vendor_bun_reinstall_required` | `skipped` (advisory; human: `Warning: …`); rollback/remove `warnings[]`; `scan --prune` top-level `warnings[]` (human: `GC: …`) | a wet Bun revert (`vendor --revert`, rollback / remove of a vendored entry, `--preserve-state` included) that restored the lock entry while `node_modules/<name>` is a real directory, or the tree has no `node_modules/.bun/` (a hoisted install): Bun's hoisted linker does not re-extract a package whose lock entry moves from the vendored tarball back to the registry record of the same `name@version`, so a plain `bun install` (also `--frozen-lockfile`) reports no changes and keeps the vendored bytes (measured on 1.1.45 … 1.4.2). The detail names `name@version` and says to run `bun install --force` (or delete `node_modules` and run `bun install`); the human revert hint names `bun install --force` too. An isolated install (a link into `node_modules/.bun/`) relinks and a project without `node_modules/` has nothing installed: neither warns, and neither does a dry run, a drift-kept revert, or a revert that restored nothing (`vendor_lockfile_missing`, or `vendor_lock_entry_removed` after `bun remove`, whose copy a plain `bun install` prunes). |
 | `vendor_flavor_changed` | `failed` | vendor (npm): the purl's vendor ledger entry was written for another lockfile `flavor` than the one the router now detects (for example `npm` → `vlt` after switching package managers). Remedy: `socket-patch vendor --revert` it first, then re-vendor. Refused before any write. |
-| `vendor_dep_manifest_unlocked` | refused | vendor (yarn classic): the patch rewrites the package's own `package.json` to depend on a descriptor (`name@range`) no `yarn.lock` block is keyed by — an added dependency, or an existing one moved to a new range. yarn 1 builds its install graph from the lock, so the rewired block would name a dependency it never resolves: online frozen installs fetch it unpinned, `--offline` installs fail and every plain `yarn install` re-saves the lock (#591). Refused after staging and before any wiring is written (the staged uuid dir is removed); the detail names the descriptors. Remedy: lock them first (for example `yarn add <descriptor>`), then re-run. A dry run, which stages nothing, does not foresee it. |
+| `vendor_dep_manifest_unlocked` | refused | vendor (yarn classic): the patch rewrites the package's own `package.json` to depend on a descriptor (`name@range`) no `yarn.lock` block is keyed by — an added dependency, or an existing one moved to a new range. yarn 1 builds its install graph from the lock, so the rewired block would name a dependency it never resolves: online frozen installs fetch it unpinned, `--offline` installs fail and every plain `yarn install` re-saves the lock (#591). Refused after staging and before any wiring is written (the staged uuid dir is removed); the detail names the descriptors. Remedy: lock them first (for example `yarn add <descriptor>`), then re-run. A dry run, which stages nothing, does not foresee it. Also `failed` on `repair` (and `vendor_check_failed` on `vendor --check`) for a lock an older release already vendored that way, with a block wired to the entry naming a descriptor no block locks and the recorded pre-vendor block did not (#1379); the lock is left as it is. Remedy: `yarn install` (without `--frozen-lockfile`) to lock them, then commit `yarn.lock`. |
 | `redirect_yarn_classic_dep_manifest_unlocked` | `warnings[]` (warning) | scan/get `--mode hosted` (yarn classic): the served tarball's own `package.json` depends on a descriptor no `yarn.lock` block is keyed by. yarn 1 installs only what the lock names, so a pin would install the patched package without that dependency (#591). The dep is not pinned and never confirmed; the lock is left as it was. Same remedy as `vendor_dep_manifest_unlocked`. |
 | `redirect_yarn_classic_dep_manifest_rewritten` | `warnings[]` (warning) | scan/get `--mode hosted` (yarn classic): the served tarball's own `package.json` declares other dependencies than the pinned block's sub-maps, every descriptor already locked; the block's `dependencies:` / `optionalDependencies:` sub-maps are rewritten to match (#591). |
 | `vendor_artifact_gitignored` | `failed` | vendor (vlt, the npm-family tarball flavors — npm, pnpm, bun, yarn classic, yarn berry — NuGet, and the JVM trees of Maven, Gradle, sbt and scala-cli): inside a git work tree, `git check-ignore --no-index` reports the new artifact's uuid directory (JVM: its tree root, such as `.socket/vendor/maven2`) as ignored by a rule its own `.gitignore` cannot override (such as a root `.socket/` or `vendor/` rule; the detail names the rule). Remedy: drop that rule for `.socket/vendor/`. Refused before any write, dry run included. A file rule such as `*.tgz`, `*.nupkg` or `*.jar` is overridden by the `!*` `.gitignore` vendoring writes into the uuid dir or tree root; if a written tarball, directory payload or nupkg still reads as ignored, the run refuses and removes the uuid dir it created. |
@@ -1994,7 +1994,11 @@ from that package's own `npm-shrinkwrap.json` whatever the lock says (#753); the
 reason names the entry and its shrinkwrapping dependency, which must be updated
 since re-vendoring cannot reach that copy. A package-lock entry also fails when
 npm's effective `allow-file` setting refuses its vendored `file:` tarball (#969,
-see the npm vendored `allow-file` contract). Missing ledger entries fail with
+see the npm vendored `allow-file` contract). A yarn classic entry fails when a
+`yarn.lock` block wired to it names a dependency descriptor that no block locks
+and that the recorded pre-vendor block did not already name (#1379: a release
+before #591 could vendor a patch that way); the reason names the descriptors
+and the remedy, `yarn install` without `--frozen-lockfile`. Missing ledger entries fail with
 `vendor_ledger_missing`: a manifest patch with no ledger entry, and a lockfile
 reference to `.socket/vendor/<eco>/<uuid>/` that no ledger entry owns (the
 artifact-level event repair emits: `uuid` plus `details.{ecosystem,path}`, no

crates/socket-patch-cli/src/commands/vendored_backend/repair.rs

@@ -474,6 +474,24 @@ impl VendoredBackend<'_> {
             }
             match health {
                 ArtifactHealth::Healthy => {
+                    // A yarn classic block a pre-v5 vendor left naming a
+                    // dependency with no lock block (#1379): the artifact
+                    // is fine, but repair can't resolve the descriptor.
+                    if entry.ecosystem == "npm" {
+                        if let Some(detail) =
+                            vendor::npm_flavor::dangling_lock_dependencies(&entry, &common.cwd)
+                                .await
+                        {
+                            fail(
+                                env,
+                                common.json,
+                                purl,
+                                "vendor_dep_manifest_unlocked",
+                                detail,
+                            );
+                            continue;
+                        }
+                    }
                     // vlt's `<uuid>/.gitignore` and `.gitattributes` are not
                     // part of the artifact: a missing or edited one is
                     // simply rewritten.

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

@@ -1240,6 +1240,84 @@ fn yarn_classic_workspaces_vendor_warns_about_member_dir_installs() {
     }
 }
 
+/// #1379: a yarn classic lock a pre-v5 release vendored with the
+/// patched manifest's new dependency in the block's sub-map but no lock
+/// block for it. Releases since #591 refuse to write that, but on such a
+/// lock `vendor --check` reported `vendor_check_ok` and `repair` left it
+/// alone, while yarn 1 installs the dependency unpinned. Both now fail the
+/// entry and name the descriptor; locking it turns the check green.
+#[test]
+fn yarn_classic_check_and_repair_flag_a_dangling_dependency_descriptor() {
+    let fx = npm_fixture();
+    std::fs::remove_file(fx.lock_path()).unwrap();
+    std::fs::write(
+        fx.root().join("package.json"),
+        r#"{"name":"fixture","version":"1.0.0","private":true,"dependencies":{"left-pad":"^1.3.0"}}"#,
+    )
+    .unwrap();
+    let yarn_lock = fx.root().join("yarn.lock");
+    std::fs::write(
+        &yarn_lock,
+        format!(
+            "# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.\n\
+             # yarn lockfile v1\n\n\n\
+             left-pad@^1.3.0:\n  version \"1.3.0\"\n  resolved \"{REG_RESOLVED}\"\n  \
+             integrity {REG_INTEGRITY}\n"
+        ),
+    )
+    .unwrap();
+    let (code, env) = vendor_cli(fx.root(), &[]);
+    assert_eq!(code, 0, "{env:#}");
+    let (code, env) = vendor_cli(fx.root(), &["--check"]);
+    assert_eq!(code, 0, "the fresh vendor is healthy: {env:#}");
+
+    // What a pre-v5 vendor of a patch adding `is-number` left behind.
+    let wired = std::fs::read_to_string(&yarn_lock).unwrap();
+    assert!(
+        wired.contains("resolved \"file:./.socket/vendor/"),
+        "{wired}"
+    );
+    assert!(
+        wired.ends_with("==\n"),
+        "left-pad is the last block: {wired}"
+    );
+    let dangling = format!("{wired}  dependencies:\n    is-number \"^7.0.0\"\n");
+    std::fs::write(&yarn_lock, &dangling).unwrap();
+
+    let (code, env) = vendor_cli(fx.root(), &["--check"]);
+    assert_eq!(code, 1, "{env:#}");
+    let failed = find_event(&env, "failed", None).to_string();
+    assert!(
+        failed.contains("vendor_check_failed") && failed.contains("is-number@^7.0.0"),
+        "{env:#}"
+    );
+
+    let cwd = fx.root().to_str().unwrap();
+    let (code, stdout, stderr) = run_cli(
+        fx.root(),
+        &["repair", "--json", "--offline", "--cwd", cwd],
+        &[],
+    );
+    assert_ne!(code, 0, "{stdout}\n{stderr}");
+    let env: Value = serde_json::from_str(&stdout).unwrap();
+    let failed = find_event(&env, "failed", Some("vendor_dep_manifest_unlocked")).to_string();
+    assert!(failed.contains("is-number@^7.0.0"), "{env:#}");
+    assert_eq!(
+        std::fs::read_to_string(&yarn_lock).unwrap(),
+        dangling,
+        "repair leaves the lock as it was"
+    );
+
+    // Once the descriptor is locked, the check is green again.
+    std::fs::write(
+        &yarn_lock,
+        format!("{dangling}\nis-number@^7.0.0:\n  version \"7.0.0\"\n"),
+    )
+    .unwrap();
+    let (code, env) = vendor_cli(fx.root(), &["--check"]);
+    assert_eq!(code, 0, "{env:#}");
+}
+
 /// #627: a symlinked lockfile (a lock shared with another checkout) is
 /// refused like hosted mode refuses it — `redirect_symlinked_file_unsupported`,
 /// exit 1 — instead of being renamed over: the link survives, its target

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

@@ -53,21 +53,89 @@ pub(crate) fn with_manifest_dep_maps(lines: &[String], pkg: &Value) -> Vec<Strin
 /// block of `blocks` is keyed by, sorted and deduplicated: the ones a lock
 /// rewritten to the patched manifest would leave unresolved.
 pub(crate) fn unlocked_descriptors(blocks: &[LockBlock], pkg: &Value) -> Vec<String> {
+    unlocked_among(
+        blocks,
+        manifest_dep_maps(pkg)
+            .into_iter()
+            .flat_map(|(_, deps)| deps)
+            .map(|(name, range)| format!("{name}@{range}")),
+    )
+}
+
+/// Every descriptor of `descriptors` no block of `blocks` is keyed by,
+/// sorted and deduplicated.
+pub(crate) fn unlocked_among(
+    blocks: &[LockBlock],
+    descriptors: impl Iterator<Item = String>,
+) -> Vec<String> {
     let locked: std::collections::BTreeSet<String> = blocks
         .iter()
         .flat_map(|b| split_key_patterns(&b.key))
         .collect();
-    let mut missing: Vec<String> = manifest_dep_maps(pkg)
-        .into_iter()
-        .flat_map(|(_, deps)| deps)
-        .map(|(name, range)| format!("{name}@{range}"))
+    let mut missing: Vec<String> = descriptors
         .filter(|descriptor| !locked.contains(descriptor))
         .collect();
     missing.sort();
     missing.dedup();
     missing
 }
 
+/// The descriptors (`name@range`) the dependency sub-maps of `lines` (one
+/// block) name: what yarn 1 resolves through other blocks of the lock.
+pub(crate) fn block_dep_descriptors(lines: &[String]) -> Vec<String> {
+    block_dep_entries(lines)
+        .into_iter()
+        .map(|(name, range)| format!("{name}@{range}"))
+        .collect()
+}
+
+/// The `(name, range)` entries of a block's `dependencies:` /
+/// `optionalDependencies:` sub-maps, read back from yarn 1's writing:
+/// a 4-space-indented `name range` pair, either side quoted when yarn
+/// had to (`"@scope/x" "^1.0.0"`, `left-pad latest`).
+fn block_dep_entries(lines: &[String]) -> Vec<(String, String)> {
+    let mut out = Vec::new();
+    let mut in_map = false;
+    for line in lines.iter().skip(1) {
+        if let Some(rest) = body_field_line(line) {
+            in_map = is_dep_map_header(rest);
+            continue;
+        }
+        if !in_map {
+            continue;
+        }
+        let (name, rest) = yarn_token(line.trim_start());
+        let (range, _) = yarn_token(rest.trim_start());
+        if !name.is_empty() && !range.is_empty() {
+            out.push((name, range));
+        }
+    }
+    out
+}
+
+/// One classic-grammar token from the front of `s` — a `"`-quoted string
+/// or a bare run up to whitespace — and the rest of `s`. Quotes are
+/// dropped and a `\\`-escape is kept verbatim (only skipped over when
+/// finding the closing quote), as [`split_key_patterns`] reads a block
+/// key, so the two compare alike.
+fn yarn_token(s: &str) -> (String, &str) {
+    let Some(quoted) = s.strip_prefix('"') else {
+        let end = s.find(char::is_whitespace).unwrap_or(s.len());
+        return (s[..end].to_string(), &s[end..]);
+    };
+    let mut chars = quoted.char_indices();
+    while let Some((i, c)) = chars.next() {
+        match c {
+            '"' => return (quoted[..i].to_string(), &quoted[i + 1..]),
+            '\\' => {
+                chars.next();
+            }
+            _ => {}
+        }
+    }
+    (quoted.to_string(), "")
+}
+
 /// Whether the sub-maps `lines` carries already are exactly the ones `pkg`
 /// declares (so a writer leaves them alone).
 pub(crate) fn dep_maps_match(lines: &[String], pkg: &Value) -> bool {
@@ -145,6 +213,40 @@ is-odd@3.0.1:\n  version \"3.0.1\"\n  dependencies:\n    is-number \"^6.0.0\"\n"
         assert!(unlocked_descriptors(&blocks, &serde_json::json!({})).is_empty());
     }
 
+    #[test]
+    fn block_dep_descriptors_reads_the_blocks_own_sub_maps() {
+        // #1379: a block a pre-v5 vendor rewrote to a patched manifest
+        // without locking the new range.
+        let lock = format!(
+            "{LOCK}\nleft-pad@^1.3.0:\n  version \"1.3.0\"\n  dependencies:\n    \
+             is-number \"^7.0.0\"\n    tag latest\n  optionalDependencies:\n    \
+             \"@scope/opt\" \"^2.0.0\"\n  integrity sha512-X==\n"
+        );
+        let blocks = scan_blocks(&lock);
+        let pad = blocks.iter().find(|b| b.key == "left-pad@^1.3.0").unwrap();
+        assert_eq!(
+            block_dep_descriptors(&pad.lines),
+            vec!["is-number@^7.0.0", "tag@latest", "@scope/opt@^2.0.0"]
+        );
+        assert_eq!(
+            unlocked_among(&blocks, block_dep_descriptors(&pad.lines).into_iter()),
+            vec!["is-number@^7.0.0", "tag@latest"]
+        );
+        let odd = blocks.iter().find(|b| b.key == "is-odd@3.0.1").unwrap();
+        assert!(unlocked_among(&blocks, block_dep_descriptors(&odd.lines).into_iter()).is_empty());
+    }
+
+    #[test]
+    fn yarn_token_reads_quoted_and_bare_tokens() {
+        assert_eq!(
+            yarn_token("left-pad \"^1\""),
+            ("left-pad".into(), " \"^1\"")
+        );
+        assert_eq!(yarn_token("\"@s/x\" y"), ("@s/x".into(), " y"));
+        assert_eq!(yarn_token("\"a\\\"b\" c"), ("a\\\"b".into(), " c"));
+        assert_eq!(yarn_token(""), (String::new(), ""));
+    }
+
     #[test]
     fn sub_maps_are_rebuilt_in_yarns_order() {
         let block = lines("is-odd@3.0.1:\n  version \"3.0.1\"\n  dependencies:\n    is-number \"^6.0.0\"\n  integrity sha512-X==");