Repository navigation
fix(core): keep trailing blank lines and CRLF endings when applying a patch - #62477
kitlangton wants to merge 2 commits into
Conversation
derive() split the file into lines, dropped the terminating empty element, and then restored a final newline only when the last line was not already empty. A file ending in a blank line therefore lost that blank line on every update, and a hunk adding an empty line at the end of a file had it silently dropped. Always restoring the terminator preserves the file's own trailing newlines while still adding one to a file that lacked it.
parse() strips one carriage return from CRLF patch lines, so every hunk produces LF-only lines. On a file whose lines all end with CRLF, derive() therefore rewrote the edited lines, and any context lines it echoed back, with LF endings, leaving the file with mixed endings and changing lines the hunk only meant to anchor. Restore the file's own terminators on the lines a hunk wrote, and leave files with mixed endings untouched.
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix looks good. I reproduced both bugs on dev by having the simulated model patch an all-CRLF file and a file ending in a blank line, then printing both with cat -A. On dev, the lines the hunk wrote turned LF while the rest of the file stayed CRLF, and the trailing blank line was deleted. With this PR, the CRLF file stays CRLF throughout, the blank line stays, and the TUI diffs show only the changed lines. I also checked some edge cases directly in Patch.derive: a CRLF file with no final newline, a file that is only \n, deleting a whole CRLF file, and a CRLF file with a BOM. All behave sensibly. The ACP permission previews also use Patch.derive and pick up the same fix.
The patch and tool-patch tests pass (90). The typecheck script fails with "Script not found" on dev too, so that isn't caused by this PR; typecheck:tsc in packages/util passes.
Public API: unchanged. Only what the patch tool writes to files changes.
No blocking concerns.
Editing a file with the patch tool no longer deletes its trailing blank line or turns its Windows (CRLF) line endings into a mix of CRLF and LF.
Both bugs were in the step that applies a patch's hunks to the file's current text. Files that end in a single newline and use LF throughout are not affected.
❌ Before
line 1\nline 2\n\n); a patch changesline 2.line 1\nLINE 2\n: the blank line is gone. A hunk that adds a blank line at the end is dropped the same way.a\r\nb\r\nc\r\n); a patch changesb.✅ After
📊 Proof
Both new tests in
patch.test.tsfail ondevand pass here.👀 Read: the end of
deriveinpackages/util/src/patch.ts. The final newline is now added unconditionally; that's safe because the split at the top already removed the real terminator, and after the CRLF pass the last line is never empty. This differs on purpose from upstream apply_patch, which has the same conditional. The rest is routine.🔍 Found in a code audit · no user reports
Checks run
bun test test/patch.test.ts test/tool-patch.test.ts(packages/core): 90 passpatch.test.tswithdev'spatch.ts: 2 fail (the two new tests)Added by the opencode bot (AI-generated).
Fixes #45926
Related: #45912