[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1804

Summary

Since #1792, apply and rollback treated every file a package lists as a bin as one npm had rewritten to an LF shebang at install. npm only does that (fixBin) for bins it links. With bin-links=false (--no-bin-links), or for a directories.bin file of a package installed from a tarball/folder (npm expands that field only in registry packuments), the installed bin keeps its CRLF shebang. So apply wrote bytes matching neither the patch's afterHash nor a fresh install, and rollback was no longer byte-exact.

Root cause

npm_bin::form_for_write and npm_bin::needs_fix took the package.json listing (is_listed_bin) as proof that npm rewrote the shebang, ignoring whether npm linked the bin at all.

Fix

is_listed_bin is now is_linked_bin: a listed bin counts as npm's form only when npm linked it:

• a .bin/<name> entry in the node_modules the package sits in (where npm links it; a symlink must resolve to this file, and a cmd/shell shim must name this file's path below node_modules), under the names npm links it as (bin keys, unscoped package name, file names for arrays and directories.bin);
• a global prefix's bin/<name> (Unix) or <name>.cmd (Windows);
• for a package inside a virtual store (pnpm .pnpm or a relocated virtualStoreDir, npm linked .store, vlt, Bun, or a copy outside any node_modules such as Yarn's pnpmStoreFolder), whose bins are linked into each dependent's .bin, the listing alone still counts, as before.

The #1791 repair (a linked bin an older release left with a CRLF shebang) still works; the on-disk LF-shebang evidence path (#1756) is unchanged. Wrappers under npm/, pypi/, gem/ don't touch this logic.

Tests (red → green)

Issue shape	Test	Without fix	With fix
#1804 --no-bin-links	patch::apply::tests::apply_writes_recorded_bytes_for_bin_npm_did_not_link (apply, re-apply, needs_npm_bin_rewrite, rollback byte-exact)	FAIL (LF shebang written)	pass
#1804 directories.bin from file:	same test, second case	FAIL (old listing)	pass
#1804 real npm, both shapes	e2e_redirect_npm_build::npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link (npm 10.9.4)	FAIL at apply	pass
link detection	npm_bin::tests::{is_linked_bin_wants_npms_link, is_linked_bin_follows_a_symlink_to_this_file, is_linked_bin_finds_global_and_pnpm_links, is_linked_bin_looks_only_where_npm_links_the_copy, is_linked_bin_wants_the_shim_to_run_this_file, is_linked_bin_takes_the_listing_in_relocated_stores, bin_names_are_npms_link_names}	new	pass

Existing #1791 unit tests now put the package under node_modules with a .bin link (the state npm leaves); the #1791/#1756 real-npm e2e tests pass unchanged.

Local runs:

• cargo fmt --all -- --check: clean
• cargo clippy --workspace --all-features -- -D warnings: clean
• cargo test -p socket-patch-cli --all-features --test e2e_redirect_npm_build -- --ignored: 9 passed
• cargo test --workspace --all-features --no-fail-fast: all pass except tests that can't pass in this sandbox: e2e_bun_lockb (2, need network to patches-api.socket.dev), and 9 permission-based tests (chmod-based write/remove failures; the sandbox runs as root). The 4 core ones fail identically on main here.

/code-review high

Fixed: nested copies were matched against the top-level .bin (now nearest node_modules only); virtual-store layouts (pnpm workspace members, npm linked, vlt, Bun) missed the link (now the listing counts there, with no store-wide scan); cheap disk probe now runs before the link lookup; trailing-slash bin keys; fixture for apply_keeps_crlf_shebang_npm_does_not_fix now has a .bin link so it still checks the listing.
Not changed:

• "Decide from installed_source before the link heuristic": a CRLF file hashing to the installed source is exactly the ambiguous After the #1756 fix, a CRLF-shebang npm bin that v5.0.1 already broke stays broken: apply reports "already patched", and repair and rollback keep the unrunnable CRLF shebang #1791 state (an older rollback wrote the raw CRLF original over a linked bin), so on-disk bytes can't settle it; the link is what tells the two apart.
• "Migrate unlinked bins the previous release already rewrote to LF": Fix CRLF npm bins left broken by v5.0.1 (#1791) #1792 is in no release tag, so no installed tree has that state.

Bugbot

Two findings on 871630f, both fixed in d2387bf (threads answered and resolved): a plain file that only had the bin's name (a project's own <bin>.cmd, or a lib workspace's sibling bin/<name>) counted as npm's link; and relocated stores lost the listing fallback. Bugbot's re-review of d2387bf: no issues found.

https://claude.ai/code/session_011f9aeQu3zbzV1egVhwbLdK

---

Note

Medium Risk
Changes npm bin shebang logic used on every patch apply/rollback, with filesystem heuristics for .bin links and shims; regression risk is mitigated by broad new tests but edge-case layouts could still misclassify linked vs unlinked bins.

Overview
Fixes #1804: patch apply and rollback no longer assume every package.json bin was rewritten by npm’s fixBin. npm only normalizes shebangs on bins it links; with --no-bin-links or directories.bin from a file: tarball install, on-disk bytes stay CRLF and must match the patch blobs exactly.

is_listed_bin is replaced by is_linked_bin, which checks for a real link (nearest node_modules/.bin, global prefix shims, symlink/canonicalize, shim path matching) and still treats virtual-store layouts (pnpm, relocated stores) like before when links live on dependents.

Apply/rollback write recorded afterHash/beforeHash for unlinked bins; linked-bin #1791 repair and LF-shebang heuristics are unchanged. Tests add unit coverage for link detection, apply+rollback byte-exact paths, refactored CRLF e2e helpers, and a real-npm e2e for both failure shapes.

^{Reviewed by Cursor Bugbot for commit 5789c73. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Ready for review at d2387bf.

• CI: 50/50 checks green (31 success, 19 skipped) on d2387bf; mergeable, no conflicts.
• Bugbot: reviewed d2387bf — no new issues; both earlier threads resolved.
• Reviewer focus: is_linked_bin in crates/socket-patch-core/src/patch/npm_bin.rs decides when CRLF→LF shebang rewriting applies (only for bins npm actually links into .bin / global shims).

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does. Since #1792, apply/rollback treated every file a package lists as a bin as one npm had rewritten to an LF shebang. npm only does that for bins it actually links, so with --no-bin-links or a directories.bin file from a tarball/folder install, apply wrote bytes matching neither afterHash nor a fresh install. Now a listed bin counts as npm's form only when a .bin/global-prefix link (symlink to this file, or a shim naming it) exists; virtual stores (pnpm, npm linked, vlt, Bun, relocated stores) keep the old listing-only behaviour. Fixes #1804.

Risk: medium. It changes which bytes apply writes for npm bin files, and the link detection is heuristic over package-manager layouts. Every uncertain layout (no node_modules ancestor, any virtual store) falls back to the previous behaviour, so the change is limited to plain npm/yarn/bun trees, which the tests cover.

Look here

• npm_bin.rs:95: form_for_write now probes the on-disk LF shebang before the link lookup (same logic as before, cheaper order)
• npm_bin.rs:149: has_bin_link: nearest node_modules/.bin, Windows <prefix>/<name>.cmd, Unix <prefix>/bin/<name>
• npm_bin.rs:202: in_virtual_store: the fallback that keeps pnpm and other stores on the listing
• npm_bin.rs:223: shim_names is a substring match of the path below node_modules (see the note below)
• apply.rs:4296: regression test covering apply, re-apply and a byte-exact rollback, for both Since #1792, agent apply and rollback rewrite a CRLF shebang that npm left alone (bin-links=false, or a directories.bin-only non-registry package), so rollback is no longer byte-exact #1804 shapes

Verified

• Read the full diff (4 files, +591/−105). Code only; CHANGELOG.md untouched; no debug leftovers.
• cargo test -p socket-patch-core --lib -- npm_bin crlf bin on d2387bf: 188 passed.
• Put npm_bin.rs back to main and ran apply_writes_recorded_bytes_for_bin_npm_did_not_link: it fails (apply.rs:4343), so the test really covers the fix.
• CI on d2387bf: 31 success, 19 skipped, 0 failed (ci-ok and clippy green). Cursor Bugbot: success. Its 2 earlier findings were fixed and their threads resolved.

Changes I made: none.

Open questions (non-blocking)

• shim_names matches by substring, so a shim for a-crlfbin/cli.js would also match crlfbin/cli.js. It only matters when the shim also carries this package's bin name, which is far-fetched and errs toward the previous behaviour.

Auto-merge (squash) is armed: approving sends this straight to the merge queue.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

[agent] Addressed the final review's open question in 5789c73: shim_names now matches the path below node_modules only as whole segments (a / before it, no path character after it), so ../a-crlfbin/cli.js and ../crlfbin/cli.json no longer count. Covered in is_linked_bin_wants_the_shim_to_run_this_file. Unit tests, clippy and the crlf e2e tests pass locally.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 5789c73. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Burn-down agent: Ready for review confirmed at 5789c73 (the label was first applied at d2387bf).

• CI: 50/50 check runs on 5789c73 green (31 success, 19 skipped); mergeable, no conflicts.
• Bugbot: reviewed 5789c73, no new issues.
• Since the label: 5789c73 makes shim_names match the path below node_modules only as whole segments, answering the final review's open question.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief (updated for 5789c73)

What it does. apply/rollback only treat an npm bin as LF-shebang-rewritten when npm actually linked it (a .bin entry or global-prefix shim that points at this file), not merely because package.json lists it. Fixes CRLF shebangs being rewritten under --no-bin-links and for directories.bin from file: installs (#1804). Since my last brief (d2387bf), 5789c73 makes a shim count only when it names the bin path as whole segments, so ../a-crlfbin/cli.js or cli.json no longer match.

Risk: medium. It changes which installed bins apply rewrites, so a missed link shape would bring back a byte mismatch on a linked bin. Virtual stores and copies outside node_modules keep the old listing-only behavior, which limits that.

Look here

• npm_bin.rs:149: has_bin_link, the link-evidence rules
• npm_bin.rs:192: global-prefix shims (Windows .cmd, Unix lib→bin)
• npm_bin.rs:223: shim_names, the whole-segment match added in 5789c73

Verified. Re-read the 5789c73 delta against npm cmd-shim and shell-shim output (quoted paths end in ", which isn't a path character, so real shims still match) and its new negative tests. CI 50/50 on 5789c73 (31 success, 19 skipped), Cursor Bugbot green, mergeable, no open review threads, CHANGELOG.md untouched.

Changes I made. None.

Auto-merge is armed, so approving sends this straight to the merge queue.

---

Generated by Claude Code