Fix npm v4 lock refusal advice looping (#1576)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -1556,7 +1556,7 @@ command's integration tests check its output.
 | `redirect_bun_patched_dependency_skipped` | `warnings[]` (warning) | scan/get `--mode hosted` (bun, `bun.lock` and `bun.lockb`): the project patches the granted `name@version` itself with `bun patch` (a `patchedDependencies` key for `name@version`, or the bare name, in the root `package.json` or mirrored in `bun.lock`). Bun applies that patch only to the registry resolution, so the entry is left on its registry tuple instead of silently losing the user's patch (#367). Per-dep; the detail names the key and the remedy (fold the Socket fix into the user's patch, or drop the `patchedDependencies` entry and re-run); the in-run VEX never assumes the uuid applied. Vendored mode refuses the same package `vendor_lock_entry_unsupported` before any write or download. Exit 0. |
 | `redirect_bun_default_trust_lost` / `vendor_bun_default_trust_lost` | `warnings[]` / vendor `warnings[]` (warning) | scan/get `--mode hosted` and vendored mode (bun, `bun.lock` and `bun.lockb`): the rewired package is on Bun's built-in default trusted list (better-sqlite3, esbuild, sharp, …) and the project declares no `trustedDependencies` (root `package.json`, or the copy mirrored in `bun.lock`). Bun 1.3.5 and later apply that list only to packages resolved from the npm registry, so on a hosted URL or a local tarball the package's install scripts are skipped with exit 0 (#371). The package is still rewired; the detail tells the user to add it to `trustedDependencies` (which replaces Bun's default list, so other default-trusted dependencies whose scripts matter must be listed too). Repeated on every run while the pin stays and no list is declared. Exit 0. |
 | `redirect_bun_non_registry_entry_skipped` / `vendor_non_registry_entry_skipped` | `warnings[]` / vendor warnings (warning) | scan/get `--mode hosted` and vendored mode (bun, `bun.lock` and `bun.lockb`): the lock also installs the granted `name@version` from a user URL or `file:` tarball (its `<name>-<ver>.tgz` leaf names that version). Bun installs it from that spec, so the copy stays unpatched beside any rewired registry copy; it is left untouched, the in-run VEX does not assume the patch, and `vex` attests nothing for that `name@version` (#497). With no registry copy left, vendoring refuses with `vendor_lock_entry_not_rewritable`. A same-name URL / `file:` tarball whose leaf names no version, or a git, folder or `link:` copy, has no version in the lock: these codes do not fire for it, but `vex` attests no ref of that package from that lock (`patched_ref_unattributable`), since the copy may be the wired version. |
-| `redirect_npm_patched_dependency_skipped` | `warnings[]` (warning) | scan/get `--mode hosted` (`package-lock.json` / `npm-shrinkwrap.json`): the project patches the granted `name@version` itself with npm ≥ 12.1's native `npm patch` (a root `package.json` `patchedDependencies` key for `name@version` or the bare name, or a `packages` entry carrying npm's `patched` record, which npm writes in a lockfileVersion 4 lock). npm applies the user's diff on top of whatever tarball the lock names and fails the install `EPATCHFAILED` when it no longer applies, so a hosted pin would break every later `npm ci` / `npm install` (or install bytes VEX can never attest); the dep is left on its registry entry in every present npm lock instead (#711). Per-dep; the detail names the key or lock entry and the remedy (fold the Socket fix into the user's patch, or drop the `patchedDependencies` entry and re-run); when an earlier hosted run already pinned the entry, the detail says so and names `socket-patch rollback <uuid>` to restore the registry entry instead of claiming it is unchanged; the in-run VEX never assumes the uuid applied, and no sibling lock confirms it. Vendored mode refuses the lockfileVersion 4 lock `vendor_lockfile_version_unsupported`, its detail naming `npm patch` / `patchedDependencies`. Exit 0. |
+| `redirect_npm_patched_dependency_skipped` | `warnings[]` (warning) | scan/get `--mode hosted` (`package-lock.json` / `npm-shrinkwrap.json`): the project patches the granted `name@version` itself with npm ≥ 12.1's native `npm patch` (a root `package.json` `patchedDependencies` key for `name@version` or the bare name, or a `packages` entry carrying npm's `patched` record, which npm writes in a lockfileVersion 4 lock). npm applies the user's diff on top of whatever tarball the lock names and fails the install `EPATCHFAILED` when it no longer applies, so a hosted pin would break every later `npm ci` / `npm install` (or install bytes VEX can never attest); the dep is left on its registry entry in every present npm lock instead (#711). Per-dep; the detail names the key or lock entry and the remedy (fold the Socket fix into the user's patch, or drop the `patchedDependencies` entry and re-run); when an earlier hosted run already pinned the entry, the detail says so and names `socket-patch rollback <uuid>` to restore the registry entry instead of claiming it is unchanged; the in-run VEX never assumes the uuid applied, and no sibling lock confirms it. Vendored mode refuses the lockfileVersion 4 lock `vendor_lockfile_version_unsupported`, its detail naming `npm patch` / `patchedDependencies` and `npm install --lockfile-version 3` (npm keeps a v4 lock at v4 on a plain `npm install`); a v4 lock with no native patch left (no `patchedDependencies` key, no non-null `patched` record) gets a detail naming only that downgrade (#1576). Exit 0. |
 | `nested_project_skipped` | `warnings[]` (warning) | scan `--mode hosted` / `--mode vendored` (npm family): the crawl found a patched package only in the `node_modules` of a nested, independent npm project below the scan root (a directory with its own npm-family lock that is not a workspace member whose package manager ignores that lock), the root's lockfiles do not list it, and the root's own install holds no copy. A hosted or vendored run rewires only the root's lockfiles, so the package is left out of the run (no `packages[]` row, no event, no VEX statement) instead of failing `vendor_lock_entry_not_found` or skipping `redirect_unconfirmed`. One warning per nested project; the detail names the purls, the project directory and the PATH to pass (`socket-patch scan --mode <mode> <dir>`). Exit and status unchanged (#1534). |
 | `redirect_vlt_lock_unsupported` | `warnings[]` (warning) | scan/get `--mode hosted` (vlt): `vlt-lock.json` has a `lockfileVersion` other than absent, `0` or `1` (decided on the raw JSON token), is not a JSON object, starts with a UTF-8 BOM, or its `nodes` section is not vlt's one-node-per-line layout. Nothing rewritten; also refuses a vendored → hosted takeover of a `flavor: "vlt"` entry before its revert (`skipped` (hosted event)). Exit 0. |
 | `redirect_takeover_kept_vendored` | `warnings[]` (warning) | scan/get `--mode hosted`: a vendored → hosted takeover the hosted rewrite would not pin was retracted (see **Staged takeover**): the package keeps its vendored wiring, ledger entry and artifact byte-identical and stays patched. The detail names the cause code, which is also the purl's `skipped` (hosted event). Exit 0. Replaces v5.0-pre `redirect_takeover_unpatched`, which reported a package left unpatched in both modes and is no longer emitted. |

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

@@ -182,7 +182,7 @@ impl NpmLockBackend for PackageLockBackend {
                 )));
             }
         };
-        let lock_version = lock_version_gate(&lock, &lock_name)?;
+        let lock_version = lock_version_gate(&lock, &lock_name, project_root)?;
 
         // The root manifest's `overrides` (#490): which git / url / `file:`
         // dependent specs npm really installs from.
@@ -423,7 +423,7 @@ pub async fn npm_lock_vendor_preflight(project_root: &Path) -> Option<(&'static
     }
     let (lock_name, lock_bytes, _) = select_lockfile(project_root).await.ok()??;
     let gate = match LOCK_MEMO.parse(&lock_bytes, || parse_json_manifest(&lock_bytes)) {
-        Ok(lock) => lock_version_gate(&lock, &lock_name).err(),
+        Ok(lock) => lock_version_gate(&lock, &lock_name, project_root).err(),
         Err(e) => Some(Box::new(refused(
             "vendor_lockfile_version_unsupported",
             format!("{lock_name} is not parseable JSON: {e}"),
@@ -437,23 +437,35 @@ pub async fn npm_lock_vendor_preflight(project_root: &Path) -> Option<(&'static
 
 /// The lock version gate of [`vendor_npm`]'s step 2: only v2/v3 locks with
 /// a `packages` object are rewritten. `Ok` is the parsed `lockfileVersion`.
-fn lock_version_gate(lock: &Value, lock_name: &str) -> Result<Option<u64>, Box<VendorOutcome>> {
+fn lock_version_gate(
+    lock: &Value,
+    lock_name: &str,
+    project_root: &Path,
+) -> Result<Option<u64>, Box<VendorOutcome>> {
     let lock_version = lock.get("lockfileVersion").and_then(Value::as_u64);
     if !matches!(lock_version, Some(2) | Some(3))
         || !lock.get("packages").is_some_and(Value::is_object)
     {
         // npm >= 12.1 writes lockfileVersion 4 for its native `npm patch`
         // (root `patchedDependencies` + a `patched` record on the entry),
         // and re-running `npm install` keeps it at 4: the npm >= 7 upgrade
-        // advice would be wrong (#711).
-        let detail = if lock_version == Some(4) {
+        // advice would be wrong (#711), and so would a plain `npm install`
+        // remedy, which loops (#1576).
+        let detail = if lock_version == Some(4) && has_native_patch(lock, project_root) {
             format!(
                 "{lock_name} has lockfileVersion 4, which npm >= 12.1 writes for its native \
                  `npm patch` (`patchedDependencies` in package.json); vendored mode supports \
                  only v2/v3 locks and would drop or break that patch. Fold the Socket fix into \
-                 your own patch, or remove the `patchedDependencies` entries and \
-                 regenerate the lock with `npm install`, or use hosted mode, which leaves the \
-                 patched package alone"
+                 your own patch, or remove the `patchedDependencies` entries and rewrite the \
+                 lock as v3 with `npm install --lockfile-version 3`, or use hosted mode, which \
+                 leaves the patched package alone"
+            )
+        } else if lock_version == Some(4) {
+            format!(
+                "{lock_name} has lockfileVersion 4, which npm >= 12.1 keeps once it has \
+                 written it, even after the last `npm patch` is removed; vendored mode \
+                 supports only v2/v3 locks. Rewrite the lock as v3 with \
+                 `npm install --lockfile-version 3` and re-run"
             )
         } else {
             format!(
@@ -470,6 +482,26 @@ fn lock_version_gate(lock: &Value, lock_name: &str) -> Result<Option<u64>, Box<V
     Ok(lock_version)
 }
 
+/// Whether the project still carries an npm >= 12.1 native patch, read as
+/// hosted mode's #711 gate reads it: a root `package.json`
+/// `patchedDependencies` key, or a non-null `patched` record on a lock
+/// entry. Only the refusal path calls it, so the blocking read is rare; it
+/// goes through the regular-file reader, so a FIFO there can't wedge it.
+fn has_native_patch(lock: &Value, project_root: &Path) -> bool {
+    let manifest =
+        crate::utils::fs::read_regular_to_string_sync(&project_root.join("package.json")).ok();
+    if !super::bun_lock_text::patched_dependency_keys(manifest.as_deref(), None).is_empty() {
+        return true;
+    }
+    lock.get("packages")
+        .and_then(Value::as_object)
+        .is_some_and(|packages| {
+            packages
+                .values()
+                .any(|entry| entry.get("patched").is_some_and(|p| !p.is_null()))
+        })
+}
+
 /// [`vendor_npm`]'s step 3: the rewritable `packages` instances of
 /// `name@version`, refusing a workspace member, an entry whose every
 /// instance is bundled or linked, and an absent entry. Nothing here reads
@@ -564,7 +596,8 @@ pub(super) async fn read_project(project_root: &Path) -> Result<NpmLockProject,
     let lock = LOCK_MEMO
         .parse(&lock_bytes, || parse_json_manifest(&lock_bytes))
         .map_err(|_| "vendor_lockfile_version_unsupported")?;
-    lock_version_gate(&lock, &lock_name).map_err(|o| super::npm_common::refusal_code(&o))?;
+    lock_version_gate(&lock, &lock_name, project_root)
+        .map_err(|o| super::npm_common::refusal_code(&o))?;
     let overrides = NpmOverrides::read(project_root).await;
     Ok(NpmLockProject {
         lock_name,
@@ -2917,11 +2950,105 @@ mod tests {
             detail.contains("lockfileVersion 4")
                 && detail.contains("npm patch")
                 && detail.contains("patchedDependencies")
+                && detail.contains("npm install --lockfile-version 3")
                 && !detail.contains("npm >= 7"),
             "{detail}"
         );
     }
 
+    /// REGRESSION (#1576): a `patchedDependencies` key in package.json
+    /// still counts as a native patch when no lock entry records it, and
+    /// the remedy names the flag that writes v3, never a plain install.
+    #[tokio::test]
+    async fn lockfile_v4_manifest_patch_remedy_downgrades_the_lock() {
+        let lock = json!({
+            "name": "fixture",
+            "version": "1.0.0",
+            "lockfileVersion": 4,
+            "packages": {
+                "": { "name": "fixture", "version": "1.0.0" },
+                "node_modules/left-pad": {
+                    "version": "1.3.0",
+                    "resolved": REG_RESOLVED,
+                    "integrity": "sha512-orig==",
+                    "patched": null
+                }
+            }
+        });
+        let fx = fixture_with("left-pad", "1.3.0", lock).await;
+        std::fs::write(
+            fx.root().join("package.json"),
+            r#"{"name":"fixture","patchedDependencies":{"left-pad@1.3.0":"patches/left-pad@1.3.0.patch"}}"#,
+        )
+        .unwrap();
+        let detail = expect_refused(
+            fx.vendor(false).await,
+            "vendor_lockfile_version_unsupported",
+        );
+        assert!(
+            detail.contains("would drop or break")
+                && detail.contains("npm install --lockfile-version 3")
+                && !detail.contains("regenerate the lock with `npm install`"),
+            "{detail}"
+        );
+    }
+
+    /// REGRESSION (#1576): a v4 lock whose native patches are all gone
+    /// is refused without claiming a patch it no longer has, and names
+    /// the downgrade that clears the refusal.
+    #[tokio::test]
+    async fn lockfile_v4_without_native_patch_names_only_the_downgrade() {
+        let lock = json!({
+            "name": "fixture",
+            "version": "1.0.0",
+            "lockfileVersion": 4,
+            "packages": {
+                "": { "name": "fixture", "version": "1.0.0" },
+                "node_modules/left-pad": {
+                    "version": "1.3.0",
+                    "resolved": REG_RESOLVED,
+                    "integrity": "sha512-orig=="
+                }
+            }
+        });
+        let fx = fixture_with("left-pad", "1.3.0", lock).await;
+        let detail = expect_refused(
+            fx.vendor(false).await,
+            "vendor_lockfile_version_unsupported",
+        );
+        assert!(
+            detail.contains("lockfileVersion 4")
+                && detail.contains("npm install --lockfile-version 3")
+                && !detail.contains("would drop or break"),
+            "{detail}"
+        );
+    }
+
+    /// A FIFO at package.json is not a manifest: the native-patch probe
+    /// reads past it instead of blocking in open(2).
+    #[cfg(unix)]
+    #[test]
+    fn native_patch_probe_does_not_block_on_a_fifo_manifest() {
+        use std::os::unix::ffi::OsStrExt as _;
+        let tmp = tempfile::tempdir().unwrap();
+        let fifo = tmp.path().join("package.json");
+        let c_path = std::ffi::CString::new(fifo.as_os_str().as_bytes()).unwrap();
+        assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o644) }, 0);
+        let (tx, rx) = std::sync::mpsc::channel();
+        let root = tmp.path().to_path_buf();
+        std::thread::spawn(move || {
+            let _ = tx.send(has_native_patch(&json!({ "packages": {} }), &root));
+        });
+        match rx.recv_timeout(std::time::Duration::from_secs(5)) {
+            Ok(found) => assert!(!found),
+            Err(_) => {
+                // Release the wedged opener so the suite can fail cleanly.
+                let _ = std::fs::OpenOptions::new().write(true).open(&fifo);
+                panic!("the native-patch probe wedged in open(2) on a FIFO");
+            }
+        }
+    }
+
     /// The takeover preflight raises exactly the backend's own version
     /// refusal on a v1 lock (#659), before any write.
     #[tokio::test]

docs/testing/npm-compatibility.md

@@ -29,7 +29,11 @@ npm 12 notes:
   mode therefore leaves such a package on its registry entry, in every npm
   lock, and warns `redirect_npm_patched_dependency_skipped`; other packages in
   the lock are still pinned. Vendored mode refuses the v4 lock
-  (`vendor_lockfile_version_unsupported`, naming `npm patch`) (#711).
+  (`vendor_lockfile_version_unsupported`, naming `npm patch`) (#711). npm
+  keeps a v4 lock at v4 on a plain `npm install`, even after `npm patch rm`,
+  so the refusal names `npm install --lockfile-version 3` as the step that
+  clears it; a v4 lock with no native patch left is refused without naming
+  one (#1576).
 - `allow-remote` defaults to `none`: any tarball whose `resolved` origin is not
   the configured registry is refused. `allow-remote=root` admits only direct
   dependencies. `allow-remote=all` is the setting a hosted redirect needs; it