Fix repair leaving npm vendor dirs uncommittable (#1581)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -829,7 +829,7 @@ to **six flavors**.
 
 | eco / flavor | vendored artifact | committed wiring | consumption proof |
 |---|---|---|---|
-| npm (package-lock) | deterministic patched tarball `[@scope/]<name>-<version>.tgz`, plus `<uuid>/.gitignore` (re-includes the tarball against the project's ignores, such as Node.gitignore's `*.tgz`) and `<uuid>/.gitattributes` (`* -text -filter -ident -working-tree-encoding`, so neither EOL conversion nor a project filter such as a Git LFS `*.tgz` rule changes the committed bytes; a pre-#1541 `* -text` file is rewritten by the next vendored run); every tarball flavor below writes the same pair and refuses `vendor_artifact_gitignored` when git would still drop the tarball | `package-lock.json` only (`npm-shrinkwrap.json` wins when present): every entry matching name+version gets `resolved: "file:…"` + recomputed `integrity`. `package.json` untouched | `npm ci` (integrity-verified). Plain `npm install` preserves the entry; `npm update <pkg>` re-resolves and drops it |
+| npm (package-lock) | deterministic patched tarball `[@scope/]<name>-<version>.tgz`, plus `<uuid>/.gitignore` (re-includes the tarball against the project's ignores, such as Node.gitignore's `*.tgz`) and `<uuid>/.gitattributes` (`* -text -filter -ident -working-tree-encoding`, so neither EOL conversion nor a project filter such as a Git LFS `*.tgz` rule changes the committed bytes; a pre-#1541 `* -text` file is rewritten by the next vendored run or `repair`); every tarball flavor below writes the same pair (and `repair` restores it) and refuses `vendor_artifact_gitignored` when git would still drop the tarball | `package-lock.json` only (`npm-shrinkwrap.json` wins when present): every entry matching name+version gets `resolved: "file:…"` + recomputed `integrity`. `package.json` untouched | `npm ci` (integrity-verified). Plain `npm install` preserves the entry; `npm update <pkg>` re-resolves and drops it |
 | npm / yarn classic | (same tarball) | `yarn.lock` only: matching blocks get `resolved "file:./…#<sha1>"` + `integrity` (both checksums recomputed; merged-key & `npm:`-alias blocks covered); a patch that rewrites the package's `package.json` gets the blocks' `dependencies:` / `optionalDependencies:` sub-maps recomputed, and is refused `vendor_dep_manifest_unlocked` before any write when a dependency it adds or a range it changes to has no lock block of its own (#591); a descriptor the wired blocks' sub-maps already name is upstream's and needs no block of its own (#1578) | `yarn install --frozen-lockfile --offline` (sha1 fragment + sha512 SRI both enforced; byte-stable lock) |
 | npm / yarn berry (node-modules linker) | (same tarball) | root `package.json` `resolutions` + `yarn.lock` entry with `checksum: 10c0/<sha512>` of the berry cache-zip (reproduced from the tarball offline). **PnP is refused** (`.pnp.*` → different artifact pipeline) | `yarn install --immutable --check-cache`, cold cache. Refused if `__metadata.cacheKey ≠ 10c0` or a non-default `compressionLevel`. Both files keep their own layout — a CRLF lock (yarn's output on Windows) is spliced in CRLF, `package.json` is re-serialized with its BOM, indent, line ending and trailing-newline shape — so vendor + `--revert` round-trip byte-exactly; a lock or `package.json` MIXING CRLF and LF is refused before any write (`vendor_yarn_berry_mixed_line_endings`) |
 | npm / pnpm (lockfileVersion 9) | (same tarball) | root `package.json` `pnpm.overrides` (versioned selector) **+** `pnpm-lock.yaml` surgery (overrides / importer version / packages `resolution.integrity` / snapshots) **+** the same override in `pnpm-workspace.yaml` (pnpm >= 10.5 reads it there; created with a root-only `packages:` scaffold when absent, and given that scaffold too when the existing file has no keys at all, since pnpm 8.x–10.4 refuse a workspace file holding a key but no `packages` (#1096); revert removes it again). A project with no `pnpm-workspace.yaml` (or a keyless one) pinned to pnpm 9.0–10.4 (every pin, read as for the hosted trust config) gets no file: those read package.json, and a root-only workspace makes `pnpm add` fail there (#734); a later vendor on pnpm >= 10.5 adds it. Residual: an unpinned project with no install record still gets the scaffold, so on pnpm 9.0–10.4 it needs `pnpm add -w <pkg>` or a `packageManager` pin | `pnpm install --frozen-lockfile --offline`, cold store (integrity-verified; byte-stable on pnpm 9 & 10). Other lockfileVersions: 5.4/6.0 route to the legacy backend below; anything else refused |

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

@@ -419,6 +419,29 @@ impl VendoredBackend<'_> {
                 entry
             };
             let health = check_vendored_artifact(&common.cwd, &entry, &record).await;
+            // An npm-family `<uuid>/.gitignore` and `.gitattributes` are not
+            // part of the artifact: a missing, edited or pre-#1541 one is
+            // simply rewritten, and git must still commit the artifact with
+            // them (#1581). A broken artifact gets both after its redownload.
+            if !common.dry_run
+                && (health == ArtifactHealth::Healthy || workspace_copy_issue(&health))
+            {
+                if let Err(e) =
+                    vendor::redownload::restore_npm_uuid_metadata(&entry, &common.cwd).await
+                {
+                    fail(
+                        env,
+                        common.json,
+                        purl,
+                        "vendor_artifact_unrepairable",
+                        format!("cannot restore the vendored dir's git metadata: {e}"),
+                    );
+                    continue;
+                }
+                if !npm_artifact_committable(common, env, purl, &entry).await {
+                    continue;
+                }
+            }
             if health == ArtifactHealth::Healthy || workspace_copy_issue(&health) {
                 let mut healed = entry.clone();
                 match repair_workspace_copies(&common.cwd, &mut healed, common.dry_run).await {
@@ -492,26 +515,6 @@ impl VendoredBackend<'_> {
                             continue;
                         }
                     }
-                    // vlt's `<uuid>/.gitignore` and `.gitattributes` are not
-                    // part of the artifact: a missing or edited one is
-                    // simply rewritten.
-                    if entry.ecosystem == "npm"
-                        && entry.flavor.as_deref() == Some(vendor::vlt_lock::FLAVOR)
-                        && !common.dry_run
-                    {
-                        if let Err(e) =
-                            vendor::vlt_lock::restore_vlt_uuid_metadata(&entry, &common.cwd).await
-                        {
-                            fail(
-                                env,
-                                common.json,
-                                purl,
-                                "vendor_artifact_unrepairable",
-                                format!("cannot restore the vendored dir's .gitignore: {e}"),
-                            );
-                            continue;
-                        }
-                    }
                     // Likewise a JVM entry's derived metadata and owned
                     // `.gitattributes`: rewritten when missing, offline.
                     if vendor::jvm::apply::is_jvm_entry(&entry) && !common.dry_run {
@@ -734,6 +737,11 @@ impl VendoredBackend<'_> {
                             continue;
                         }
                     }
+                    if !npm_artifact_committable(common, env, &candidate.purl, &candidate.entry)
+                        .await
+                    {
+                        continue;
+                    }
                     if !quiet {
                         println!(
                             "Redownloaded {} ({})",
@@ -759,6 +767,29 @@ impl VendoredBackend<'_> {
     }
 }
 
+/// After repair rewrote an npm-family uuid dir: `false` (and a
+/// `vendor_artifact_gitignored` failure) when git would still leave the
+/// artifact out of the next commit, which `vendor` refuses up front. A git
+/// that cannot answer is only a warning.
+async fn npm_artifact_committable(
+    common: &GlobalArgs,
+    env: &mut Envelope,
+    purl: &str,
+    entry: &VendorEntry,
+) -> bool {
+    match vendor::redownload::npm_artifact_commit_check(entry, &common.cwd).await {
+        Ok(None) => true,
+        Ok(Some(warning)) => {
+            record_warning(env, purl, &warning, common);
+            true
+        }
+        Err((code, detail)) => {
+            fail(env, common.json, purl, code, detail);
+            false
+        }
+    }
+}
+
 fn workspace_copy_issue(health: &ArtifactHealth) -> bool {
     health.is_workspace_copy_issue()
 }

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

@@ -5130,3 +5130,157 @@ async fn in_process_vendor_state_attests_manifest_less() {
             .expect("manifest-less VEX tail panicked");
     });
 }
+
+// ─────────────────────────────────────────────────────────────────────
+// 13. repair restores the npm `<uuid>/` metadata (#1581)
+// ─────────────────────────────────────────────────────────────────────
+
+const UUID_GITIGNORE: &str =
+    "!*\n**/node_modules/*/node_modules/\n**/node_modules/@*/*/node_modules/\n";
+const UUID_GITATTRIBUTES: &str = "* -text -filter -ident -working-tree-encoding\n";
+
+fn uuid_dir(fx: &NpmFixture) -> PathBuf {
+    fx.root().join(format!(".socket/vendor/npm/{UUID}"))
+}
+
+/// `git init` the fixture; `false` when git is not installed.
+fn git_init(root: &Path) -> bool {
+    if socket_patch_core::utils::process::resolve_tool("git").is_none() {
+        return false;
+    }
+    assert!(std::process::Command::new("git")
+        .arg("-C")
+        .arg(root)
+        .args(["init", "-q"])
+        .status()
+        .unwrap()
+        .success());
+    true
+}
+
+fn git_stdout(root: &Path, args: &[&str]) -> String {
+    let out = std::process::Command::new("git")
+        .arg("-C")
+        .arg(root)
+        .args(args)
+        .output()
+        .unwrap();
+    String::from_utf8_lossy(&out.stdout).into_owned()
+}
+
+fn repair_cli(cwd: &Path) -> (i32, Value) {
+    let (code, stdout, stderr) = run_cli(
+        cwd,
+        &["repair", "--json", "--cwd", cwd.to_str().unwrap()],
+        &[],
+    );
+    let env: Value = serde_json::from_str(&stdout).unwrap_or_else(|e| {
+        panic!("repair --json must emit an envelope: {e}\nstdout:\n{stdout}\nstderr:\n{stderr}")
+    });
+    (code, env)
+}
+
+/// #1581: a project vendored before #1541 keeps the legacy `* -text`
+/// `<uuid>/.gitattributes`, so a repository `*.tgz filter=lfs` rule still
+/// applies to the tarball. `repair` must upgrade it, both when the
+/// artifact is healthy and when it redownloads an LFS pointer, or the
+/// repaired tarball goes back into LFS on the next commit.
+#[tokio::test]
+async fn repair_upgrades_a_legacy_uuid_gitattributes() {
+    let fx = npm_fixture();
+    assert_eq!(vendor_run(vendor_args(fx.root())).await, 0);
+    let has_git = git_init(fx.root());
+    std::fs::write(
+        fx.root().join(".gitattributes"),
+        "*.tgz filter=lfs diff=lfs merge=lfs -text\n",
+    )
+    .unwrap();
+    let attrs = uuid_dir(&fx).join(".gitattributes");
+    let tarball = std::fs::read(fx.tgz_path()).unwrap();
+
+    // Healthy artifact.
+    std::fs::write(&attrs, "* -text\n").unwrap();
+    let (code, env) = repair_cli(fx.root());
+    assert_eq!(code, 0, "{env:#}");
+    assert_eq!(std::fs::read_to_string(&attrs).unwrap(), UUID_GITATTRIBUTES);
+
+    // The tarball checked out as an LFS pointer (no smudge).
+    std::fs::write(&attrs, "* -text\n").unwrap();
+    std::fs::write(
+        fx.tgz_path(),
+        "version https://git-lfs.github.com/spec/v1\noid sha256:00\nsize 1\n",
+    )
+    .unwrap();
+    let (code, env) = repair_cli(fx.root());
+    assert_eq!(code, 0, "{env:#}");
+    find_event(&env, "rebuilt", None);
+    assert_eq!(std::fs::read(fx.tgz_path()).unwrap(), tarball);
+    assert_eq!(std::fs::read_to_string(&attrs).unwrap(), UUID_GITATTRIBUTES);
+    if has_git {
+        assert_eq!(
+            git_stdout(fx.root(), &["check-attr", "filter", "--", &rel_tgz()]),
+            format!("{}: filter: unset\n", rel_tgz()),
+            "the repository's LFS rule must not reach the vendored tarball"
+        );
+    }
+}
+
+/// #1581 (second symptom): when the whole `<uuid>/` dir is lost, `repair`
+/// redownloaded only the tarball. Without its `<uuid>/.gitignore` (`!*`) a
+/// stock Node.gitignore `*.tgz` rule ignores it, so "commit it" committed
+/// nothing and every fresh clone failed.
+#[tokio::test]
+async fn repair_of_a_lost_uuid_dir_restores_its_gitignore_and_gitattributes() {
+    let fx = npm_fixture();
+    assert_eq!(vendor_run(vendor_args(fx.root())).await, 0);
+    let has_git = git_init(fx.root());
+    std::fs::write(fx.root().join(".gitignore"), "node_modules/\n*.tgz\n").unwrap();
+    let tarball = std::fs::read(fx.tgz_path()).unwrap();
+    std::fs::remove_dir_all(uuid_dir(&fx)).unwrap();
+
+    let (code, env) = repair_cli(fx.root());
+    assert_eq!(code, 0, "{env:#}");
+    find_event(&env, "rebuilt", None);
+    assert_eq!(std::fs::read(fx.tgz_path()).unwrap(), tarball);
+    assert_eq!(
+        std::fs::read_to_string(uuid_dir(&fx).join(".gitignore")).unwrap(),
+        UUID_GITIGNORE
+    );
+    assert_eq!(
+        std::fs::read_to_string(uuid_dir(&fx).join(".gitattributes")).unwrap(),
+        UUID_GITATTRIBUTES
+    );
+    if has_git {
+        assert_eq!(
+            git_stdout(fx.root(), &["check-ignore", "--no-index", &rel_tgz()]),
+            "",
+            "git must commit the redownloaded tarball"
+        );
+    }
+}
+
+/// #1581: a rule the `<uuid>/.gitignore` cannot override (`.socket/`) still
+/// ignores the redownloaded tarball. `repair` must say so with the same
+/// `vendor_artifact_gitignored` code `vendor` refuses with, not report a
+/// repair that the next commit drops.
+#[tokio::test]
+async fn repair_of_a_lost_uuid_dir_under_an_ignored_socket_dir_fails() {
+    let fx = npm_fixture();
+    assert_eq!(vendor_run(vendor_args(fx.root())).await, 0);
+    if !git_init(fx.root()) {
+        return;
+    }
+    std::fs::write(fx.root().join(".gitignore"), "node_modules/\n.socket/\n").unwrap();
+    std::fs::remove_dir_all(uuid_dir(&fx)).unwrap();
+
+    let (code, env) = repair_cli(fx.root());
+    assert_eq!(code, 1, "{env:#}");
+    let failed = find_event(&env, "failed", Some("vendor_artifact_gitignored"));
+    assert_eq!(failed["purl"], PURL, "{env:#}");
+
+    // The artifact is now healthy on disk, but git still drops it: the
+    // next run must not go green with nothing fixed.
+    let (code, env) = repair_cli(fx.root());
+    assert_eq!(code, 1, "{env:#}");
+    find_event(&env, "failed", Some("vendor_artifact_gitignored"));
+}

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

@@ -381,21 +381,24 @@ pub(crate) async fn node_modules_holds_only_links(rel_abs: &Path) -> bool {
 }
 
 /// Rewrite `<uuid>/.gitignore` and `<uuid>/.gitattributes` when absent or
-/// different; neither is part of the artifact.
-pub(crate) async fn restore_uuid_metadata(uuid_dir: &Path) -> std::io::Result<()> {
+/// different; neither is part of the artifact. `true` when either was
+/// rewritten.
+pub(crate) async fn restore_uuid_metadata(uuid_dir: &Path) -> std::io::Result<bool> {
     write_uuid_metadata(uuid_dir, UUID_GITIGNORE).await
 }
 
 /// [`restore_uuid_metadata`] with the backend's own `.gitignore` text.
-pub(crate) async fn write_uuid_metadata(uuid_dir: &Path, gitignore: &str) -> std::io::Result<()> {
+pub(crate) async fn write_uuid_metadata(uuid_dir: &Path, gitignore: &str) -> std::io::Result<bool> {
+    let mut rewritten = false;
     for (name, want) in [(GITIGNORE, gitignore), (GITATTRIBUTES, UUID_GITATTRIBUTES)] {
         let path = uuid_dir.join(name);
         let current = crate::utils::fs::read_regular_to_bytes(&path).await.ok();
         if current.as_deref() != Some(want.as_bytes()) {
             atomic_write_bytes(&path, want.as_bytes()).await?;
+            rewritten = true;
         }
     }
-    Ok(())
+    Ok(rewritten)
 }
 
 // ── gitignore probe ──────────────────────────────────────────────────────

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

@@ -1722,7 +1722,9 @@ const UUID_GITIGNORE: &str = "!*\n";
 /// `<uuid>/.gitattributes` when absent or different; neither is part of
 /// the artifact.
 pub(crate) async fn restore_uuid_metadata(uuid_dir: &Path) -> std::io::Result<()> {
-    super::npm_dir::write_uuid_metadata(uuid_dir, UUID_GITIGNORE).await
+    super::npm_dir::write_uuid_metadata(uuid_dir, UUID_GITIGNORE)
+        .await
+        .map(|_| ())
 }
 
 /// Write `<uuid>/.gitignore` and `<uuid>/.gitattributes` next to the