Fix CRLF npm bins left broken by v5.0.1 (#1791) (#1792)

* Start fix for #1791

Assisted-by: Claude Code:claude-opus-5-5

* Recover npm bins v5.0.1 left with CRLF shebangs

v5.0.1 could write a patch's raw bytes over an npm bin that npm had
installed with an LF shebang, leaving "#!/usr/bin/env node\r" that env
can't run (exit 127). After #1757, upgrading did not recover it: apply
and apply --check called it already patched, and rollback wrote the
CRLF original back, because the shebang form was chosen from whatever
was on disk.

Decide from the package's own package.json bin field (bin string or
object, or directories.bin) whether a file is a bin npm rewrites. Apply
now rewrites such a bin in npm's form, apply --check reports it as not
applied, and rollback restores npm's runnable form of the original.

Fixes #1791

Assisted-by: Claude Code:claude-opus-5-5

* Match npm's bin rules and fix old CRLF rollbacks

Read package.json bins the way npm does: array bins, a string bin only
when the package has a name, and directories.bin (skipping hidden
files) when bin is missing or empty. apply --check now flags a bin only
when its bytes are exactly the patched content, the same test apply
uses, so the two always agree.

Rollback also rewrites a bin that an older rollback left holding the
raw CRLF original, when the original's blob is available. Writes skip
reading package.json for content that has no CRLF shebang.

Refs #1791

Assisted-by: Claude Code:claude-opus-5-5

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: mikolalysenko

crates/socket-patch-cli/src/commands/apply.rs

@@ -600,6 +600,33 @@ async fn verify_installed_tree(
         .failed
         .retain(|f| !(f.reason == "package_not_found" && mismatched_keys.contains(&f.purl)));
     outcome.failed.extend(unmatched);
+
+    // An npm bin holding the patched bytes with a CRLF shebang `env`
+    // can't run (written by a release before #1756) is one `apply`
+    // rewrites, so it is not in sync (#1791).
+    let mut crlf_bins: Vec<String> = Vec::new();
+    for purl in &outcome.applied {
+        let Some(record) = tree.patches.get(purl) else {
+            continue;
+        };
+        'copies: for copy in copies.get(purl).into_iter().flatten() {
+            for (file, info) in &record.files {
+                if socket_patch_core::patch::apply::needs_npm_bin_rewrite(purl, copy, file, info)
+                    .await
+                {
+                    crlf_bins.push(purl.clone());
+                    break 'copies;
+                }
+            }
+        }
+    }
+    outcome.applied.retain(|purl| !crlf_bins.contains(purl));
+    outcome
+        .failed
+        .extend(crlf_bins.into_iter().map(|purl| FailedPatch {
+            purl,
+            reason: "not_applied".to_string(),
+        }));
     outcome
 }
 

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

@@ -1161,30 +1161,21 @@ async fn npm_get_ghsa_hosted_narrows_and_installs() {
     manifestless_tail(&fx, &fresh, installed, &[]);
 }
 
-/// Regression (#1756): on Linux and macOS npm's `bin-links` rewrites a
-/// `bin` file's CRLF shebang to LF at install. Agent-mode `apply` patches
-/// that install (it was refused as drift), keeps the bin runnable (a
-/// written-back `#!/usr/bin/env node\r` makes `env` exit 127), re-applies
-/// as a no-op, `vex` attests it, and `rollback` restores npm's form of the
-/// original. Offline: the package is a local tarball.
-#[cfg(unix)]
-#[tokio::test(flavor = "multi_thread")]
-#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"]
-async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
-    const SUITE: &str = "npm_agent_patches_crlf_shebang_bin_npm_rewrote";
-    if npm_e2e_common::npm_version().is_none() {
-        npm_e2e_common::skip(SUITE, "npm not on PATH");
-        return;
-    }
-    let before: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n";
-    let after: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n";
-    // What npm leaves on disk for each: the first line's `\r` dropped.
-    let installed_before: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n";
-    let installed_after: &[u8] =
-        b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n";
+/// The tarball bytes of a bin published from Windows (CRLF shebang), its
+/// patched twin, and what npm's `fixBin` leaves on disk for each: the
+/// first line's `\r` dropped (#1756).
+const CRLF_BIN_BEFORE: &[u8] = b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n";
+const CRLF_BIN_AFTER: &[u8] =
+    b"#!/usr/bin/env node\r\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n";
+const NPM_BIN_BEFORE: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n";
+const NPM_BIN_AFTER: &[u8] = b"#!/usr/bin/env node\nconsole.log(\"hi\");\r\n// SOCKET-PATCHED\r\n";
 
-    let tmp = tempfile::tempdir().unwrap();
-    let pkg = tmp.path().join("pkg");
+/// A project under `tmp` with the `crlfbin` tarball (a CRLF-shebang
+/// `bin`) npm-installed, and an agent manifest patching that bin with
+/// its blobs. Returns the project dir.
+fn crlf_bin_project(tmp: &Path) -> PathBuf {
+    let (before, after) = (CRLF_BIN_BEFORE, CRLF_BIN_AFTER);
+    let pkg = tmp.join("pkg");
     std::fs::create_dir_all(&pkg).unwrap();
     std::fs::write(
         pkg.join("package.json"),
@@ -1193,18 +1184,18 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
     .unwrap();
     std::fs::write(pkg.join("cli.js"), before).unwrap();
     std::fs::write(
-        tmp.path().join("crlfbin-1.0.0.tgz"),
+        tmp.join("crlfbin-1.0.0.tgz"),
         npm_e2e_common::npm_pack_like(&pkg),
     )
     .unwrap();
-    let proj = tmp.path().join("proj");
+    let proj = tmp.join("proj");
     std::fs::create_dir_all(&proj).unwrap();
     std::fs::write(
         proj.join("package.json"),
         r#"{"name":"p","version":"1.0.0","dependencies":{"crlfbin":"file:../crlfbin-1.0.0.tgz"}}"#,
     )
     .unwrap();
-    let cache = tmp.path().join("cache");
+    let cache = tmp.join("cache");
     let out = npm_e2e_common::npm(
         &proj,
         &[
@@ -1221,11 +1212,7 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
         npm_e2e_common::output_text(&out)
     );
     let cli = proj.join("node_modules/crlfbin/cli.js");
-    assert_eq!(
-        std::fs::read(&cli).unwrap(),
-        installed_before,
-        "npm's fixBin"
-    );
+    assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE, "npm's fixBin");
 
     let (b, a) = (git_sha256(before), git_sha256(after));
     let blobs = proj.join(".socket/blobs");
@@ -1243,12 +1230,42 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
     )
     .unwrap();
 
-    let run_bin = || {
-        std::process::Command::new(proj.join("node_modules/.bin/crlfbin"))
-            .current_dir(&proj)
-            .output()
-            .unwrap()
-    };
+    proj
+}
+
+/// Runs the project's `crlfbin` bin, asserting it prints `hi`.
+fn assert_crlf_bin_runs(proj: &Path, what: &str) {
+    let ran = std::process::Command::new(proj.join("node_modules/.bin/crlfbin"))
+        .current_dir(proj)
+        .output()
+        .unwrap();
+    assert!(
+        ran.status.success(),
+        "{what}: {}",
+        npm_e2e_common::output_text(&ran)
+    );
+    assert_eq!(String::from_utf8_lossy(&ran.stdout).trim(), "hi", "{what}");
+}
+
+/// Regression (#1756): on Linux and macOS npm's `bin-links` rewrites a
+/// `bin` file's CRLF shebang to LF at install. Agent-mode `apply` patches
+/// that install (it was refused as drift), keeps the bin runnable (a
+/// written-back `#!/usr/bin/env node\r` makes `env` exit 127), re-applies
+/// as a no-op, `vex` attests it, and `rollback` restores npm's form of the
+/// original. Offline: the package is a local tarball.
+#[cfg(unix)]
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"]
+async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
+    const SUITE: &str = "npm_agent_patches_crlf_shebang_bin_npm_rewrote";
+    if npm_e2e_common::npm_version().is_none() {
+        npm_e2e_common::skip(SUITE, "npm not on PATH");
+        return;
+    }
+    let (installed_before, installed_after) = (NPM_BIN_BEFORE, NPM_BIN_AFTER);
+    let tmp = tempfile::tempdir().unwrap();
+    let proj = crlf_bin_project(tmp.path());
+    let cli = proj.join("node_modules/crlfbin/cli.js");
 
     let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]);
     assert_eq!(code, 0, "apply\nstdout:\n{stdout}\nstderr:\n{stderr}");
@@ -1257,13 +1274,7 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
         "no drift warning: {stderr}"
     );
     assert_eq!(std::fs::read(&cli).unwrap(), installed_after);
-    let ran = run_bin();
-    assert!(
-        ran.status.success(),
-        "{}",
-        npm_e2e_common::output_text(&ran)
-    );
-    assert_eq!(String::from_utf8_lossy(&ran.stdout).trim(), "hi");
+    assert_crlf_bin_runs(&proj, "after apply");
 
     let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]);
     assert_eq!(code, 0, "re-apply\nstdout:\n{stdout}\nstderr:\n{stderr}");
@@ -1291,10 +1302,46 @@ async fn npm_agent_patches_crlf_shebang_bin_npm_rewrote() {
     let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--offline"]);
     assert_eq!(code, 0, "rollback\nstdout:\n{stdout}\nstderr:\n{stderr}");
     assert_eq!(std::fs::read(&cli).unwrap(), installed_before);
-    let ran = run_bin();
+    assert_crlf_bin_runs(&proj, "after rollback");
+}
+
+/// Regression (#1791): v5.0.1 wrote the raw CRLF patched bytes over the
+/// bin npm had fixed, so it exits 127. `apply --check` reports that drift,
+/// `apply` rewrites the bin in npm's form, and from the same broken state
+/// `rollback` restores npm's runnable form of the original.
+#[cfg(unix)]
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "wall-bound real-npm install; runs on the e2e CI matrix leg"]
+async fn npm_agent_recovers_crlf_bin_left_by_raw_write() {
+    const SUITE: &str = "npm_agent_recovers_crlf_bin_left_by_raw_write";
+    if npm_e2e_common::npm_version().is_none() {
+        npm_e2e_common::skip(SUITE, "npm not on PATH");
+        return;
+    }
+    let tmp = tempfile::tempdir().unwrap();
+    let proj = crlf_bin_project(tmp.path());
+    let cli = proj.join("node_modules/crlfbin/cli.js");
+    // What v5.0.1's apply left: the recorded bytes, CRLF shebang and all.
+    std::fs::write(&cli, CRLF_BIN_AFTER).unwrap();
+
+    let (code, stdout, stderr) = run_socket(&proj, &["apply", "--check", "--offline"]);
+    assert_eq!(code, 1, "check\nstdout:\n{stdout}\nstderr:\n{stderr}");
     assert!(
-        ran.status.success(),
-        "{}",
-        npm_e2e_common::output_text(&ran)
+        format!("{stdout}{stderr}").contains("pkg:npm/crlfbin@1.0.0"),
+        "names the drifted patch\nstdout:\n{stdout}\nstderr:\n{stderr}"
     );
+
+    let (code, stdout, stderr) = run_socket(&proj, &["apply", "--offline"]);
+    assert_eq!(code, 0, "apply\nstdout:\n{stdout}\nstderr:\n{stderr}");
+    assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_AFTER);
+    assert_crlf_bin_runs(&proj, "after apply");
+
+    let (code, stdout, stderr) = run_socket(&proj, &["apply", "--check", "--offline"]);
+    assert_eq!(code, 0, "re-check\nstdout:\n{stdout}\nstderr:\n{stderr}");
+
+    std::fs::write(&cli, CRLF_BIN_AFTER).unwrap();
+    let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--offline"]);
+    assert_eq!(code, 0, "rollback\nstdout:\n{stdout}\nstderr:\n{stderr}");
+    assert_eq!(std::fs::read(&cli).unwrap(), NPM_BIN_BEFORE);
+    assert_crlf_bin_runs(&proj, "after rollback");
 }