Skip to content

Fix CRLF bin e2e failing under npm 11 on macOS - #1896

Open
Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
mainfrom
ci-janitor/npm11-directories-bin-crlf
Open

Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
mainfrom
ci-janitor/npm11-directories-bin-crlf

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

e2e-macos (macos-latest, e2e_redirect_npm_build, 1) has failed on every run since #1805 (b73b080) landed on 2026-10-29 13:29 UTC:

Both failures are the same assertion:

npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link panicked at e2e_redirect_npm_build.rs:1388:
bin/cli.js: npm left the shebang alone
  left:  "#!/usr/bin/env node\n..."   (LF shebang)
  right: "#!/usr/bin/env node\r\n..." (CRLF shebang)

This leg doesn't run on PRs or in the merge queue, so it doesn't block merges. But it keeps the nightly and every workflow_dispatch run red (ci-ok (macos only)), which hides new macOS regressions.

Root cause

The test's second case installs a directories.bin-only package from a file: tarball and assumes npm won't link the bin, so the CRLF shebang survives. That holds for npm 10 but not npm 11, which links bin/cli.js into node_modules/.bin and rewrites its shebang the way fixBin does. The e2e_redirect_npm_build legs have no setup-node step, so they use the runner image's npm: npm 10 on the Ubuntu legs (which pass) and npm 11 on macos-latest (which fails).

I reproduced it on Linux:

npm node_modules/.bin installed bin/cli.js shebang test on main
10.9.4 empty \r\n pass
11.21.1 (SOCKET_PATCH_NPM_E2E_BIN) cli.js \n fails, same assertion as CI

Fix

Test-only change. The test now checks whether npm linked the bin (node_modules/.bin/<name> exists):

  • Not linked: expects the untouched CRLF bytes, as before.
  • Linked: expects npm's rewritten bytes (NPM_BIN_BEFORE/NPM_BIN_AFTER, the same constants npm_agent_patches_crlf_shebang_bin_npm_rewrote uses).

In both cases it still checks apply, apply --check and a byte-exact rollback. A new assertion fails if --no-bin-links ever links the bin, so the unlinked #1804 path stays covered on every npm version. If npm linked the bin without rewriting it, or rewrote it without linking it, the test still fails. No assertions were removed.

Proof

  • cargo test -p socket-patch-cli --test e2e_redirect_npm_build -- --ignored crlf (all 3 CRLF tests) passes with npm 10.9.4 and with npm 11.21.1.
  • Ran npm_agent_keeps_crlf… 10 times under each npm version: 0 failures out of 20.
  • cargo clippy -p socket-patch-cli --test e2e_redirect_npm_build -- -D warnings reports no new warnings. The only errors are two duplicate_mod lints that already fire on main for this test target; CI's clippy job doesn't build test targets.
  • rustfmt run on the touched file only.

The macOS leg only runs on nightly and workflow_dispatch, so the next nightly will confirm the fix in CI.

Where tests run

Nothing removed or moved. The test still runs on the e2e Ubuntu leg (npm 10) and the nightly e2e-macos leg (npm 11).


Generated by Claude Code


Note

Low Risk
Test-only change in an ignored e2e; no production code or CLI behavior is modified.

Overview
Fixes nightly macOS e2e failures where npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link assumed npm always leaves CRLF shebangs on directories.bin installs from a file: tarball. npm 11 links those bins into node_modules/.bin and rewrites the shebang like fixBin; npm 10 did not.

The test now detects whether npm linked the expected .bin entry and picks baseline bytes accordingly: untouched CRLF when unlinked (--no-bin-links or npm 10-style behavior), LF shebang (NPM_BIN_*) when linked. It still asserts byte-exact apply, apply --check, and rollback, and fails if --no-bin-links ever creates a link. Doc comments were updated to document npm 10 vs 11.

Reviewed by Cursor Bugbot for commit 9d7d4cb. Configure here.


Generated by Claude Code

npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link assumed npm never
links a directories.bin-only package installed from a file: tarball.
npm 10 doesn't, but npm 11 does: it links bin/cli.js and rewrites its
CRLF shebang to LF. The e2e_redirect_npm_build legs use the runner
image's npm, which is 11 on macos-latest, so the macOS nightly leg has
been red on main since #1805 landed.

The test now checks whether npm linked the bin and expects npm's
rewritten bytes when it did, and the untouched CRLF bytes when it
didn't. apply, apply --check and a byte-exact rollback are still
asserted in both cases, and --no-bin-links must still leave the bin
unlinked.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011f9aeRC4w7jsAshzLQM6VL
@mikolalysenko Mikola Lysenko (mikolalysenko) added the ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) label Oct 30, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

I dispatched a macOS-only run of this branch to check the fix in CI: https://github.com/SocketDev/socket-patch/actions/runs/41029711345

The PR run is green, including the required ci-ok and clippy checks.


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent) at 5e0418e.

  • CI: PR checks green; the macOS dispatch passes e2e-macos (e2e_redirect_npm_build), which is the leg this PR fixes.
  • Main-wide failure (not caused by this PR): test (macos-latest, 2) / ci-ok (macos only), from apply_never_writes_go_work_when_gowork_names_a_sibling_file, which also fails on main's nightly (run 41173651959). Fix Go GOWORK test failing on macOS nightly #1894 fixes it.
  • Bugbot: reviewed 5e0418e, no findings; no unresolved threads.
  • Reviewer note: test-only change.

Generated by Claude Code

@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator Author

Generated by Claude Code

The --no-bin-links guard looked for .bin/cli.js, but npm names a bin-map
link after its key (crlfbin), so the guard could never fire. Pass the link
name npm would create for each case: the bin key for the bin map, the file
name for directories.bin.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011f9aeTApLyQQnpuXiK39EX
@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review: the --no-bin-links guard probed .bin/cli.js, but npm names a bin-map link after its key (crlfbin), so the guard could never fire, contrary to what the description says. I pushed 9d7d4cb, which passes each case's real link name. I ran the test locally with npm 10.9.9 and it passes, and clippy is clean on that test target. I removed Ready for review until CI is green again. Everything else in the PR looks good.

bugbot run


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 9d7d4cb. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Labeled Ready for review again at 9d7d4cb, after the final reviewer pushed the link-name fix.

  • CI: all PR checks green on 9d7d4cb (31 success, 19 skipped); mergeable, no conflicts.
  • Bugbot: reviewed 9d7d4cb, no findings; no unresolved review threads.
  • Reviewer note: test-only change in tests/e2e_redirect_npm_build.rs. The --no-bin-links guard now probes the crlfbin link name npm actually creates. The macOS leg this fixes runs only in the full/dispatch tier, not in PR CI.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: Test-only fix for the macOS nightly red in npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link. npm 11 (macOS runner image) links a directories.bin file and rewrites its shebang, while npm 10 (Ubuntu) doesn't. The test now checks whether npm created the node_modules/.bin link and expects npm's rewritten bytes in that case, or the original CRLF bytes otherwise. It still fails if a bin gets linked under --no-bin-links.

Risk: low. One test file, no product code. The test still checks apply, apply --check and a byte-exact rollback for both shapes.

Look here:

  • crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs:1377:`` the new link_name column (bin key vs. file name)
  • crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs:1395:``` link detection and the --no-bin-links guard
  • crates/socket-patch-cli/tests/e2e_redirect_npm_build.rs:1432:```` rollback now compares against the bytes npm wrote

Verified: Read the full diff. NPM_BIN_BEFORE/NPM_BIN_AFTER are the existing constants that the linked-bin tests in this file already use. The PR's CI run is green (ci-ok and clippy pass, and Ubuntu e2e_redirect_npm_build passes). Bugbot found no issues on this head. Caveat: the macOS leg only runs nightly or on dispatch, so the npm 11 branch has the author's local npm 11.21.1 repro behind it but no CI run. CHANGELOG.md is untouched.

Changes I made: none.

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


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants