repl: strip trailing CR when loading history (Windows CRLF) - #30293
repl: strip trailing CR when loading history (Windows CRLF)#30293samuelpatro wants to merge 3 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughThe REPL history loader now trims trailing ChangesCRLF Normalization in REPL History
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/repl/repl.test.ts`:
- Around line 936-944: Move the process exit assertion so it runs after the
saved-history assertions: keep the existing saved = await Bun.file(...).text()
and the three expect(saved)... checks first (ensuring no "\r" and presence of
"old_one\n", "old_two\n", "new_three\n"), then await proc.exited and assert
expect(exitCode).toBe(0); this changes only the order of checking the exitCode/
proc.exited usage (variable exitCode from proc.exited) so test failures show
saved-history diffs before the process exit assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c381254-c9c8-4ddc-a262-ca5fc6cda754
📒 Files selected for processing (2)
src/repl.zigtest/js/bun/repl/repl.test.ts
History files written with CRLF line endings (e.g. on Windows, or after editing in an editor that converts EOLs) carried the trailing '\r' into each entry. The CR then leaked into recall, .history output, and was re-saved verbatim, compounding the corruption on every session. Mirror the pattern used in ini.zig: strip '\r' before treating the line as an entry. I could not run `bun bd test` in this environment (missing clang-21), but verified with the system bun that the new test fails before the fix and the change is a one-line copy of the established pattern.
Port the prior Zig fix to the new Rust REPL. History files written with CRLF line endings (Windows, or after editing in a CRLF editor) carried the trailing '\r' into each entry, leaking into recall, .history output, and the next save.
5dc0065 to
aabc696
Compare
|
Picked this up in #36510 on top of current main (the |
History.load split the history file on '\n' only, so a file written with CRLF line endings (Windows, or after editing in a CRLF editor) kept a trailing '\r' on every loaded entry. The stray CR then leaked into arrow-key recall, .history output, and the next save, compounding on every session. Strip a trailing '\r' from each split line before storing the entry. Adopts the fix from #30293 for the current Rust REPL (the repl.zig hunk in that PR no longer applies). Co-authored-by: Samuel <samuelpatro@users.noreply.github.com>
|
Closing this one in favor of #36510, which carries the same change for the current Rust REPL. This branch still patches |
The trailing-CR strip in History::load is the fix from #30293 (carried by #36510 until this PR superseded it). Add that PR's case as its own test: a history file of ordinary entries with CRLF line endings loads without a '\r' on any entry and is written back with LF endings. Co-authored-by: Samuel <samuelpatro@users.noreply.github.com>
|
One more pointer, since #36510 has now been closed too: your CRLF fix is part of #38023 (which stores multi-line history entries on one line and needs the same trailing-CR strip when loading). Its commits keep your |
3 participants
History files written with CRLF line endings (e.g. on Windows, or after editing in an editor that converts EOLs) carried the trailing '\r' into each entry. The CR then leaked into recall, .history output, and was re-saved verbatim, compounding the corruption on every session.
Mirror the pattern used in ini.zig: strip '\r' before treating the line as an entry.
I could not run
bun bd testin this environment (missing clang-21), but verified with the system bun that the new test fails before the fix and the change is a one-line copy of the established pattern.What does this PR do?
Fixes a CRLF handling bug in the Bun REPL's history loader.
History.load in src/repl.zig split the history file on '\n' only, so when the file used CRLF line endings (common on Windows, or after editing in a CRLF-defaulting editor), every loaded entry kept a trailing '\r'. The stray CR then leaked into:
arrow-key recall (the cursor jumped to column 0 after the recalled text),
.history output,
the next .save — which wrote the same CRs back, compounding the corruption on every subsequent session.
The fix mirrors the existing pattern in src/ini.zig:60: strip a trailing '\r' before treating the line as an entry.
How did you verify your code works?
Added a regression test in test/js/bun/repl/repl.test.ts ("strips CRLF when loading existing history") that:
Seeds a temp $HOME with a .bun_repl_history containing old_one\r\nold_two\r\n,
Spawns bun repl, evaluates one new line, then .exit (forcing load → modify → save),
Re-reads the saved history and asserts there are no \r bytes and all three entries are present.
Verified the test fails against an unpatched Bun (system bun reproduced old_one\r\nold_two\r\nnew_three\n in the saved file).
Could not run bun bd test to confirm the patched build passes — sandbox is missing clang-21. Please run locally before merging:
bun bd test test/js/bun/repl/repl.test.ts -t "strips CRLF"