Fix CRLF .npmrc append with no final newline (#1196)

[Mikola Lysenko (mikolalysenko)]

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

Fixes #1196

Summary

A hosted npm scan that appended allow-remote=all to a CRLF project .npmrc whose last line had no terminator (a common Windows editor save) wrote …audit=false\nallow-remote=all\r. The old last line lost its CR and the file ended in a lone CR, which the planner itself refuses (Unsupported, CR-only) on any later run. It now writes …audit=false\r\nallow-remote=all. LF files and files that end in a newline produce the same output as before.

Root cause

plan_npmrc_allow_remote_with (crates/socket-patch-core/src/patch/redirect/npmrc.rs) splits the file on \n, inserts allow-remote=all\r (CRLF files) after the last non-empty top-level line, and joins with \n. The baked-in \r assumes a \n from the join follows it. When the anchor is past an unterminated last line, the new line becomes the tail: nothing follows it, and the old last line gets the join's bare \n.

Fix

• When the anchor is past the last element, append the file's terminator (line_endings::terminator) and then the line, keeping the no-final-newline shape.
• The spliced line's CR is derived from that same terminator, so there is only one CRLF-or-LF decision.
• Pick the anchor with the module's js_trim (npm's whitespace, which includes U+FEFF) rather than str::trim, so a stray BOM-only line counts as blank, the way npm reads it.
• Updated the NpmrcPlan::Append and planner docs. The appended line has no unwind path: a user-owned .npmrc is never edited by rollback (npm_allow_remote_left), so terminating the old last line doesn't affect any revert.

Tests

Issue	Test	Without fix	With fix
#1196	npmrc::tests::append_after_an_unterminated_last_line_keeps_its_terminator (CRLF, BOM+CRLF, LF, one line, BOM-only tail; re-plan is AlreadyAll)	fail: "fund=false\r\naudit=false\nallow-remote=all\r"	pass
#1196	vendor suite: crlf_npmrc_without_final_newline_keeps_crlf_on_append (the issue's repro through scan --mode hosted, then a second scan shows no CR-only refusal and no rewrite)	fail (same bytes)	pass

Local runs:

• cargo fmt --all -- --check: clean
• cargo clippy --workspace --all-features -- -D warnings: clean
• cargo test -p socket-patch-core --all-features --lib: 6334 passed, 4 failed. All 4 failures are chmod-0555 permission tests (vlt_heal, copy_tree, pypi_poetry, pypi_requirements) that can't fail as root, and the sandbox runs as uid 0. None of them touch this code.
• cargo test -p socket-patch-cli --all-features --test vendor: 94 passed
• A full cargo test --workspace --all-features ran out of this sandbox's disk allowance while linking test binaries, so I couldn't finish it locally. CI runs the full set.
• No wrapper changes: npm/, pypi/ and gem/ have no .npmrc logic.

Review notes (/code-review high)

Fixed: stale docs, the duplicated terminator decision, trim vs js_trim, and the missing second-scan check. Not done: the reviewer suggested folding the end-of-file case into the general splice instead of an early return. The early return is the one place where the inserted line has nothing after it, and the comment explains why, so I kept it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MLrkKwvWmW2Zmi46c1dRCT

---

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 758f53a. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review (burn-down agent)

• Head: 758f53a
• CI: 35/35 non-skipped checks green, 1 neutral (50 skipped by path filters).
• Bugbot: reviewed 758f53a, no findings; no open review threads.
• Mergeable, no conflicts; no CHANGELOG change.
• Slack announcement: not sent this run (Slack send tool unavailable in this session); next run retries.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does. Fixes #1196. A hosted npm scan that appended allow-remote=all to a CRLF .npmrc with an unterminated last line wrote …audit=false\nallow-remote=all\r: a bare LF plus a lone CR, which the planner itself refuses on the next run. Now, when the new line goes after an unterminated last line, the planner adds the file's own terminator to that line and then appends, keeping the no-final-newline shape.

Risk: low. One planner function. The only output that changes is the unterminated-last-line case. For mixed files the CR decision is the same as before (terminator()), and the trim → js_trim switch adds only U+FEFF to the blank-line test (\r was already whitespace to both). Rollback never edits a user-owned .npmrc, so there is no unwind path to keep in sync.

Look here

• npmrc.rs:778: the new end-of-file branch
• npmrc.rs:787: the spliced line's CR now comes from the same terminator
• npmrc.rs:776: anchor uses js_trim, so a BOM-only line counts as blank
• npmrc.rs:985: unit table (CRLF, BOM+CRLF, LF, one line, BOM-only tail) with a re-plan check that each result is AlreadyAll
• redirect_npm_allow_remote.rs:387:`` the issue's repro through scan --mode hosted, run twice

Verified. Read the full diff and the helpers it uses (terminator, js_trim, top_level_end). Traced the empty file, whitespace-only tail, mixed-ending and [section] cases: all produce the same output as before. The new tests fail on the old code: the old splice yields the lone-CR bytes the tests reject. CHANGELOG.md is untouched. CI on 758f53a: ci-ok and clippy green, 0 failed checks. Bugbot: no issues.

Changes I made. None.

Open questions. None.

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

---

Generated by Claude Code