Repository navigation
Conversation
History::save wrote each entry verbatim followed by '\n', so an entry that was typed over several lines occupied several lines of ~/.bun_repl_history, and History::load (which splits on '\n') brought it back as one entry per fragment in the next session. Store the newlines inside an entry as '\r' on save and turn them back on load, the encoding node's repl history uses. The line editor reads a typed '\r' as Enter, so no entry contains one and the decode cannot alter a real entry. load strips a CRLF line ending before decoding, so a file rewritten with CRLF endings does not gain a trailing newline per entry. In-memory entries are unchanged, so .history, recall and .save behave as before. Co-authored-by: Samuel <samuelpatro@users.noreply.github.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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 |
|
Updated 10:58 AM PT - Oct 21st, 2026
✅ @robobun, your commit 77b5eb966138d52bb07b96851149dc6381b0fbe1 passed in 🧪 To try this PR locally: bunx bun-pr 38023That installs a local version of the PR into your bun-38023 --bun |
|
Status: reproduced on a build of main (2d6d445) and on bun 1.4.3-canary.1. Pipe This PR fixes it. The 4 tests in the Two notes for the merge order:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, self-contained fix that swaps \n↔\r in history entries so multi-line inputs survive a restart as one entry.
What was reviewed
append_swapping_line_breaks+ theload/savechanges: verified the split/join is a plain byte replace andtrim_suffixstrips exactly one trailing\r.- Checked the invariants the encoding rests on:
Key::from_bytemaps 13 to Enter so no entry contains\r, andhandle_entertrims surrounding\nbeforehistory.addso no encoded entry ends in\r(CRLF strip is unambiguous). - Confirmed
.save(cmd_save) is intentionally unchanged — it writes runnable source, not the history-file encoding. - Tests:
HOME/USERPROFILEboth set (env_var::HOMEreadsUSERPROFILEon Windows), exact file-content and snapshot assertions cover round-trip, blank lines inside an entry, and CRLF re-save.
Extended reasoning...
Overview
Two files: src/runtime/cli/repl.rs (adds a 10-line append_swapping_line_breaks helper and rewires History::load/History::save through it — ~25 changed lines) and test/js/bun/repl/repl.test.ts (a new describe.concurrent("REPL history file") block with three tests). The fix stores each newline inside a multi-line history entry as \r on disk and decodes it back on load, so ~/.bun_repl_history keeps one entry per physical line and a restarted session recalls the whole multi-line input instead of fragments. This matches Node's .node_repl_history encoding.
Security risks
None. This is developer-tool persistence of the REPL's own input history to a file the REPL itself writes; no untrusted input is parsed differently, no auth/crypto/permissions code is touched, and the file mode (0600) path is unchanged.
Level of scrutiny
Low-to-moderate. The change is a byte-level string transform in a pair of symmetric functions with a small, easy-to-audit helper. I verified the two invariants the scheme depends on directly in the surrounding code: (1) \r cannot appear in an in-memory entry because Key::from_byte maps byte 13 to Key::Enter, and multi-byte UTF-8 never encodes \r; (2) entries are trimmed of surrounding \n before history.add, so an encoded entry never ends in \r and the CRLF trim_suffix cannot eat a real trailing newline. strings::split/strings::trim_suffix were checked in bun_core. The .save REPL command is deliberately left writing real newlines (it emits runnable JS, not the history encoding), which the PR description calls out.
Other factors
- Backward compatible: existing history files contain no
\r(other than CRLF endings, which are now stripped), so they load unchanged. - Tests are strong per the repo guidelines: exact
toEqualon the saved file's physical lines, inline snapshots of the second session's output (arrow-up recall actually definesfandf()returns 1), an explicitnot.toContain("\r")guard because snapshots tolerate CRLF, and a CRLF-file re-save assertion.HOMEandUSERPROFILEare both set andenv_var::HOMEis platform-specific, so the block is not POSIX-only. - The PR notes a one-line overlap with #36510 in
load; that is a merge-order concern, not a correctness issue here. - No prior reviews or outstanding comments on this PR.
The local split-and-copy helper duplicated strings::replace_owned; both history sites now call that directly. The encoding and the invariants it relies on are documented once, at the save site, and the CRLF comments no longer attribute CRLF files to Windows (bun itself never writes CRLF to the history file; only an external editor does).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Both history sites now read the byte from HISTORY_STORED_NEWLINE, whose doc comment is the one place the file encoding is described; the inline comments at the save and load sites are gone, except a one-liner on the CRLF trim saying why it cannot eat entry content.
There was a problem hiding this comment.
LGTM — small, well-reasoned fix that mirrors Node's .node_repl_history encoding.
What was reviewed:
History::save/loadround-trip: verifiedreplace_ownedandtrim_suffixsemantics; entries never end in\n(trimmed inhandle_enter), so the CRLF strip is unambiguous.- Confirmed
\rcan't otherwise reach an entry (Key::from_bytemaps 13 → Enter; editor mode and.loaddon't callhistory.add). cmd_saveintentionally left writing real newlines (it emits source, not the history-file encoding).- Tests cover the on-disk format, arrow-up recall + duplicate suppression, blank lines inside an entry, entry order, and CRLF load/re-save.
Extended reasoning...
Overview
Two files: ~10 changed lines in src/runtime/cli/repl.rs (History::load/save plus a new HISTORY_STORED_NEWLINE constant) and a three-test describe.concurrent block in test/js/bun/repl/repl.test.ts. The fix encodes newlines inside a history entry as \r on save and decodes them on load, so a multi-line REPL input survives a restart as one entry instead of fragmenting into one entry per physical line.
Security risks
None. This is encoding of a per-user history file the REPL already reads and writes; no new paths, no external input beyond what load() already handled, and the file mode (0600) is unchanged.
Level of scrutiny
Low-to-medium. The change is mechanical (two strings::replace_owned calls and a trim_suffix), scoped to the REPL's history file, and follows Node's internal/repl/history.js encoding exactly. The one subtlety — whether stripping a trailing \r per line can ever eat entry content — is closed by the invariant that handle_enter trims surrounding newlines before history.add(), so a stored entry never ends in \r. I traced every path that reaches history.add() (only handle_enter; editor mode and .load evaluate without adding) and confirmed Key::from_byte maps CR to Enter, so \r cannot appear as entry content.
Other factors
The comment-cop bot flagged over-long comments on earlier commits; those were resolved in aa5d213 (the encoding is now a one-line doc comment on the constant), and all inline threads are marked resolved. cmd_save (the .save REPL command) is deliberately left writing real newlines since it emits runnable source, not the history-file format — the PR description calls this out. The new tests are hermetic (tempDir, per-test HOME/USERPROFILE), concurrent, and assert the exact on-disk bytes plus a full-session snapshot including arrow-up recall, duplicate suppression, blank-line preservation, entry order, and CRLF→LF re-save. The PR notes it supersedes #36510 (CRLF-only) and documents the trivial rebase if that lands first.
There was a problem hiding this comment.
LGTM — small, well-scoped fix that mirrors Node's .node_repl_history encoding.
What was reviewed
History::save/loadround-trip:\rcannot enter an entry via the line editor (Key::from_bytemaps 13 → Enter) andadd()gets\n-trimmed input, so the trailing-\rstrip is unambiguous vs. CRLF endings.- Confirmed
strings::replace_owned/strings::trim_suffixsignatures match usage;env_var::HOMEreadsUSERPROFILEon Windows so the new tests (which set both) are cross-platform. cmd_saveintentionally left writing raw newlines (it emits source code, not the history-file format); comment-cop feedback on long comments was addressed in aa5d213.
Extended reasoning...
Overview
This PR fixes multi-line REPL history persistence in src/runtime/cli/repl.rs. History::save now replaces embedded \n with \r (the new HISTORY_STORED_NEWLINE constant) so each entry occupies exactly one line of ~/.bun_repl_history; History::load strips a trailing \r per line (CRLF tolerance) and decodes remaining \r back to \n. Three new tests in test/js/bun/repl/repl.test.ts cover the on-disk encoding, round-trip with blank interior lines and entry ordering, and loading a CRLF-saved file. Net Rust change is ~10 lines.
Security risks
None. The history file is user-local and already written with mode 0600; this change only alters the byte-level encoding of newlines within entries. No new syscalls, no untrusted-input parsing beyond what load already did (splitting on \n).
Level of scrutiny
Low-to-medium. The change is a targeted serialization fix in a non-hot-path CLI feature, using existing in-tree helpers (strings::replace_owned, strings::trim_suffix) and the exact encoding Node's internal/repl/history.js uses. The correctness argument (why \r is a safe sentinel and why the trailing-\r strip cannot eat entry content) checks out against the surrounding code: Key::from_byte treats byte 13 as Enter so the line editor never inserts it, and handle_enter calls history.add(strings::trim(&code, b"\n")) so a stored entry never ends in \r. Existing history files contain no interior \r, so they load unchanged.
Other factors
- The comment-cop bot flagged long explanatory comments; the author addressed this by collapsing to a one-line doc comment on
HISTORY_STORED_NEWLINEplus a short inline note inload(all threads resolved). - CI failures on the latest build (
setInterval.test.js,html-rewriter-leak.test.ts, Windows aarch64 agent creation) are unrelated flakes touching none of the changed code. - Tests are hermetic (
tempDir+HOME/USERPROFILEoverride), concurrent, and assert exact file contents plus inline snapshots of the reload session;env_var::HOMEisUSERPROFILEon Windows so the non-skipped block is portable. cmd_save(the.save <file>REPL command) is correctly left untouched — it writes runnable source, not the history-file format.
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>
There was a problem hiding this comment.
LGTM — small, self-contained fix that encodes multi-line REPL history entries with \r (matching Node's .node_repl_history format) so they survive a restart as one entry.
What was reviewed:
- Verified the invariant that entries never contain
\r:Key::from_bytemaps CR→Enter, andhandle_entertrims\nfrom both ends beforeadd(), so encoded entries never end in\rand the singletrim_suffixfor CRLF is unambiguous. - Confirmed
strings::trim_suffixstrips exactly one occurrence andreplace_ownedhandles the byte-for-byte swap;cmd_save(.save) intentionally left un-encoded since it writes runnable source. - Checked cross-platform: test sets both
HOMEandUSERPROFILE, andenv_var::HOMEresolves toUSERPROFILEon Windows; the arrow-up escape sequence is read byte-wise from the pipe so works with non-TTY stdin. - Tests cover round-trip, embedded blank lines (
\r\r), CRLF file with a stored multi-line entry, and the plain CRLF single-line case from #36510.
Extended reasoning...
Overview
Two files touched: ~15 lines in src/runtime/cli/repl.rs (History::load/History::save plus a named constant) and ~140 lines of new tests in test/js/bun/repl/repl.test.ts. The Rust change replaces newlines inside a history entry with \r when writing the one-entry-per-line file, and reverses that on load after stripping a single trailing \r (CRLF line ending). This is exactly the encoding Node uses for .node_repl_history.
Security risks
None. The change only affects how the REPL's own history file is serialized/deserialized; no new syscalls, no untrusted-input parsing beyond what load already did, and the file-permission logic (0o600) is unchanged.
Level of scrutiny
Low-to-medium. The REPL history file is a convenience feature, not a hot path or security boundary. The change is mechanical (two replace_owned calls plus a trim_suffix), and the correctness argument hinges on one invariant — entries never contain \r — which I verified by tracing every path into History::add: only handle_enter calls it, with input built from the line editor (where CR maps to Key::Enter) and trimmed of \n at both ends. .load and .editor don't add to history, so they can't introduce \r either. Because entries never end in \n, encoded lines never end in \r, making the single-\r CRLF strip unambiguous even for entries whose last internal char decodes from \r.
Other factors
- The comment-cop bot flagged verbose inline comments three times; the author reduced them to a one-line doc comment on
HISTORY_STORED_NEWLINEand all threads are resolved. - Four new tests exercise the on-disk format directly (exact
savedLinesassertions), the reload behavior via.historyand arrow-up recall, blank-line preservation (\r\rsequences), and CRLF interop in both the mixed and single-line-only forms — the latter subsumes #36510's test case. - Tests follow harness conventions:
tempDir,bunEnv/bunExeviarunRepl,describe.concurrent, inline snapshots, and set bothHOME/USERPROFILEsoenv_var::HOMEresolves on Windows. replace_ownedalways allocates even when no replacement is needed, but this runs once per session on ≤1000 short entries, so it's immaterial.- No CODEOWNERS on this path, no outstanding human review comments, and CI is building the latest commit.
runRepl sets HOME and USERPROFILE after it spreads options.env, so a HOME passed in env does not reach the REPL. The history file tests passed the directory in env, then read a directory that the REPL never used. The tests now pass the directory with the home option. They compare outputs with toEqual and check stderr, as the other tests in the file do.
Problem
bun replkeeps a multi-line input as one history entry.History::save(src/runtime/cli/repl.rs:247) writes it with its newlines, so it takes several lines of the history file.History::load(repl.rs:216) splits the file on\n. The next session has three entries:function f() {,return 1,}.\rof a CRLF line ending..historyprintsold_one\r.Fix
savewrites each newline inside an entry as\r.loaddrops one\rat the end of each line and turns the others back into\n.\ras Enter, and no entry ends in a newline.\r, so it loads as before.REPL history fileintest/js/bun/repl/repl.test.ts(4 tests, all fail on a build of main). Also ran the whole file.Background
$HOME/.bun_repl_historyat startup and rewrites it at exit. Each line is one entry.\r. A backslash escape changes how an existing entry such as"a\nb"loads.Downsides
.historyand on Up. Up then Enter still runs it.savemakes one temporary buffer per entry, at most 1000 per exit.Notes
Checks
--profile=release) of thesrc/of main (2d6d445) and of this branch (77b5eb9). The build of main fails the 4 tests. The build of this branch passes them and the whole file (133 tests). A debug build (ASAN) of this branch also passes the whole file.function f() {,return 1,}on three lines. Tests 3 and 4:Expected to not contain: "\r".sizeon the two release binaries: text 107349864 and 107350120 (+256). Data, bss and the file size (107521760 bytes) are equal.saveruns only at the exit of a session that added an entry. The allocation count is from the code, not from a tool:strace,perf,valgrindandbloatyare not on the build machine. Insave,replace_ownedreturns a newVecfor each entry. Inload, it takes the place of theBoxthat each entry already had.function f() {\r return 1\r}as one entry, runs the entry on recall and writes it back unchanged. On a pty of 60 columns,.historydraws its row as} return 1nction f() {and Up draws} return 1 f() {..savewrites the\rbytes, and the saved file still runs. A template literal keeps its value: the older bun evaluates a recalled template literal with a raw CR betweenaandbtoa\nb.\r\x1b[J> function f() {\r\n return 1\r\n}\r\x1b[27C. The 1.4.3 canary writes the same bytes for an entry of the current session. The cursor is in a wrong column, and each later key leaves one more copy of the two upper lines. Up then Enter runs the entry and leaves a clean screen.printf 'function f() {\n return 1\n}\n.exit\n' | HOME=/tmp/h bun repl. Then.historyin a secondHOME=/tmp/h bun repllists 3 entries. For CRLF:printf 'old_one\r\nold_two\r\n' > /tmp/h/.bun_repl_history, then.historyprints1 old_one\r.Details
.history, recall, the check for a duplicate of the last entry and.savedo not change..savewrites source code, so it keeps the newlines.handle_enteradds it with its outer newlines trimmed, so a stored entry never ends in\r. A\rat the end of a line is then always part of a CRLF line ending. Without the strip, that\rwould decode into a newline at the end of each entry.src/js/internal/repl/history.js:146stores\nas\r, and line 321 splits the file on/\r?\n+/.\rinto an entry on main.Key::from_bytemaps byte 13 to Enter.Key::Textis a multi-byte UTF-8 sequence. Completions insert identifiers only (repl: inline ghost-text suggestions for symbol autocomplete #30413, in 1.4.0).\r. Such an entry now loads with\nin its place. JS reads both as a line terminator, so the entry evaluates the same.\rbytes, and the next save writes LF. That is the intended change from repl: strip trailing CR when loading history (Windows CRLF) #30293.\ras a line ending: the entry then loads as it does on main.$HOME(USERPROFILEon Windows). The tests pass that directory with thehomeoption ofrunRepl.runReplsetsHOMEandUSERPROFILEafter it spreadsenv, so aHOMEinenvdoes not reach the REPL.Related
\rinload. repl: strip trailing CR when loading history (Windows CRLF) #36510 applied it torepl.rsand was closed in favour of this PR. The commits carry theCo-authored-bytrailer, and the last test is the case from repl: strip trailing CR when loading history (Windows CRLF) #36510.refresh_line. This PR does not changerefresh_line. It makes entries from a previous session reach that bug. repl: redraw a recalled multi-line history entry in place #46095 is the change torefresh_line.History::savewithHistory::write. If it merges first, the encode moves into the loop ofwrite. The decode stays in the parse loop ofload.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/repl/repl.test.ts