repl: strip trailing CR when loading history (Windows CRLF)

[robobun]

Adopts #30293 by @samuelpatro onto current main. That PR also patches src/runtime/cli/repl.zig, which no longer exists; this applies the fix to repl.rs only.

Repro

printf 'old_one\r\nold_two\r\n' > ~/.bun_repl_history
bun repl
> .history
     1  old_one\r
     2  old_two\r

Arrow-up recall lands the cursor at column 0 after the recalled text, and the next save writes old_one\r\nold_two\r\n… back, so the stray CR survives every session.

Cause

History::load in src/runtime/cli/repl.rs splits the history file on '\n' only:

for line in strings::split(&content, b"\n") {
    if !line.is_empty() {
        self.entries.push(Box::<[u8]>::from(line));
    }
}

A history file with CRLF line endings (written on Windows, or edited in a CRLF-defaulting editor) therefore leaves a trailing '\r' on every loaded entry. That CR leaks into arrow-key recall, .history output, and the next save() (which writes entry + '\n'), so the corruption compounds.

Fix

Strip a trailing '\r' from each split line before storing it:

let line = line.strip_suffix(b"\r").unwrap_or(line);

save() already writes LF-only, so the file normalizes after one load/save cycle.

Why this is the right behavior:

• Matches node: its REPL history loader splits the file on /\r?\n+/ (see the vendored src/js/internal/repl/history.js used by node:repl), so a CRLF history file loads cleanly there too.
• Nothing legitimate is lost: save() emits entry + '\n', so an entry whose text ended in '\r' would be written as CRLF and be indistinguishable from a CRLF file anyway. The native format has no meaning for a trailing CR.
• A blank CRLF line ("\r") becomes empty and is skipped, the same as a blank LF line already is.

Verification

New test in test/js/bun/repl/repl.test.ts (REPL history file > strips CRLF when loading existing history) seeds .bun_repl_history with "old_one\r\nold_two\r\n", runs .history + a new entry + .exit, then asserts:

• .history output lines contain no \r
• the saved file contains no \r and splits to exactly ["old_one","old_two","new_three"]

# without fix (canary da3851e57)
(fail) REPL history file > strips CRLF when loading existing history
  Expected to not contain: "\r"
  Received: "     1  old_one\r"

# with fix (debug build of this branch, rebased on f426a8e173)
(pass) REPL history file > strips CRLF when loading existing history

# full suite
149 pass / 0 fail

Closes #30293.

---

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

[coderabbitai]

Walkthrough

The REPL history loader strips trailing carriage returns from loaded lines. A non-Windows test verifies normalized recall, .history output, saved history, empty stderr, and successful exit.

Changes

REPL history normalization

Layer / File(s)	Summary
Normalize loaded history entries src/runtime/cli/repl.rs	Loaded history lines remove a trailing \r before empty-line filtering and storage.
Verify CRLF history behavior test/js/bun/repl/repl.test.ts	The test verifies normalized history recall, display, saving, stderr, and exit status.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The Rust loader fix and regression test satisfy the CRLF handling, normalization, preservation, and verification objectives in issue #30293.
Out of Scope Changes check	✅ Passed	The one-line Rust fix and focused REPL regression test are directly related to issue #30293 and contain no unrelated changes.
Title check	✅ Passed	The title clearly and concisely describes the main change: stripping trailing CR characters when loading Windows CRLF REPL history.
Description check	✅ Passed	The description explains the cause, fix, verification steps, test coverage, and expected behavior in sufficient detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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 1287-1292: In the REPL subprocess test, move the stderr and
exitCode assertions before the Bun.file(...).text() call so subprocess failure
is reported first. Keep the saved-history content assertions unchanged and only
read the history file after confirming the subprocess succeeded.
- Around line 1280-1285: Add explicit presence assertions for both “old_one” and
“old_two” in historyLines before the carriage-return loop. Keep the existing
per-line CR checks, ensuring the test fails when either loaded history entry is
missing.

🪄 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: 379bf9b6-0601-4db1-9ba5-6516a5de747f

📥 Commits

Reviewing files that changed from the base of the PR and between 468dac3 and ad7877f.

📒 Files selected for processing (2)

• src/runtime/cli/repl.rs
• test/js/bun/repl/repl.test.ts

[github-actions]

This PR may be a duplicate of:

1. repl: strip trailing CR when loading history (Windows CRLF) #30293 - Original PR with the same CRLF history fix, targeting the old Zig codebase (now has merge conflicts); repl: strip trailing CR when loading history (Windows CRLF) #36510 explicitly adopts this PR

🤖 Generated with Claude Code

[claude]

LGTM — one-line strip_suffix(b"\r") in History::load is the obvious fix, and the saved-file toEqual assertion in the new test independently catches the regression, so the inline nit about the possibly-empty loop is non-blocking.

Extended reasoning...

Overview

The PR adds a single line to History::load in src/runtime/cli/repl.rs: let line = line.strip_suffix(b"\r").unwrap_or(line);, so a CRLF history file (written on Windows or a CRLF editor) no longer leaves a trailing \r on every loaded entry. It also adds a regression test in test/js/bun/repl/repl.test.ts that seeds .bun_repl_history with CRLF, runs .history + one new entry + .exit, and asserts both the .history output and the re-saved file contain no \r.

Security risks

None. This is byte-level trimming of a locally-read history file inside the REPL's own history loader; no untrusted network input, no path handling, no privilege changes.

Level of scrutiny

Low. The Rust change is a one-liner using the idiomatic strip_suffix().unwrap_or() pattern on a &[u8]; a bare-\r line correctly becomes empty and is skipped by the existing !line.is_empty() guard, and save() already writes LF-only so the file normalizes after one round-trip. The test follows harness conventions (tempDir, bunEnv/bunExe(), await using, concurrent drain of stdout/stderr/exited, HOME+USERPROFILE for cross-platform), and the PR description shows it fails on the unfixed build.

Other factors

The one finding is a nit: the for (const line of historyLines) loop over a filtered array has no non-emptiness guard. It's non-blocking because the saved-file toEqual(["old_one","old_two","new_three"]) assertion at line 1289 independently proves the CR was stripped — that assertion would see ["old_one\r","old_two\r","new_three"] on the unfixed build regardless of whether .history output rendering ever changes. No prior reviews or unresolved comments on the PR.

[claude]

LGTM — the vacuous-loop guard from the earlier review is now in place (expect(historyLines).toHaveLength(2)), and all threads are resolved.

What was reviewed:

• History::load: strip_suffix(b"\r") runs before the empty check, so a bare \r line is correctly dropped rather than stored as an empty entry.
• Test: pipes drained concurrently, HOME/USERPROFILE both set, new_three's ReferenceError goes to stdout (not stderr) so expect(stderr).toBe("") holds; history is added before evaluation so the entry persists regardless.
• Saved-file toEqual([...]) assertion independently catches the regression, verified fails-without-fix in the PR description.

Extended reasoning...

Overview

One-line change to src/runtime/cli/repl.rs (History::load): each history line is passed through .strip_suffix(b"\r").unwrap_or(line) before the emptiness check and storage. A new test in test/js/bun/repl/repl.test.ts seeds a CRLF history file, runs .history + a new entry + .exit, and asserts both the .history output and the re-saved file contain no \r.

Security risks

None. This is purely local REPL history-file parsing with no network, auth, or untrusted-boundary implications. The strip is applied to data the user already controls (their own ~/.bun_repl_history).

Level of scrutiny

Low. The Rust change is a single idiomatic slice operation with no allocation, no unsafe, and no control-flow change beyond making a previously non-empty "\r" line empty (which is then correctly filtered out by the existing !line.is_empty() guard). save() already writes LF-only, so the round-trip normalizes.

Other factors

• My earlier inline nit (guard the possibly-empty historyLines filter) was addressed in commit a15ec10 with expect(historyLines).toHaveLength(2); the thread is resolved.
• The other CodeRabbit suggestion (assertion ordering before file read) was reasonably declined by the author: the file is pre-seeded by tempDir, so the read cannot throw, and a REPL crash would surface as the seeded CRLF content failing not.toContain("\r") — matching the neighboring history-permissions tests and the repo's assert-exit-code-last convention.
• Verified the test's stderr === "" expectation holds despite new_three being an undefined identifier: interactive-REPL errors route through print_js_error → Output::writer() (stdout), and history.add() runs before evaluate_and_print(), so the entry is persisted regardless of the ReferenceError.
• PR description shows the test failing on the unfixed build and passing with the fix, satisfying the fails-for-the-right-reason requirement.

[robobun]

Status: rebased onto current main (f426a8e) as 8a4f8af. The only conflict was the loop header in History::load, which #37052 changed to strings::split; the strip_suffix(b"\r") line is unchanged. The original #30293 has been closed pointing here, and the commit keeps its Co-authored-by trailer.

Verified locally after the rebase:

• REPL history file > strips CRLF when loading existing history fails on canary da3851e (Received: " 1 old_one\r") and passes on a debug build of this branch.
• Full test/js/bun/repl/repl.test.ts: 149 pass, 0 fail.

CI on 8a4f8af (build 93728, finished): 179 of 181 jobs passed; test/js/bun/repl/repl.test.ts passed on every lane that ran. The build is marked failed only because the two darwin 26 aarch64 test jobs were canceled by the infrastructure before running; that lane ran the identical diff in the previous build (86019) and the repl suite passed there too. The handful of test failures in this build (child_process_ipc_handle.test.ts, vm/sourcetextmodule-leak.test.ts, bun/globals.test.js, grpc-js/test-tonic.test.ts) are all unrelated and all passed on retry or when rerun alone. Not retriggering for that; the diff is ready as is.

While re-reviewing this I noticed a separate pre-existing issue in the same loader: a multi-line entry is saved as several physical lines and reloads as several entries. That is independent of CRLF handling and is being tracked separately, not in this PR.

Previous status (a15ec10, before the rebase)

CI on a15ec10: the diff was green. test/js/bun/repl/repl.test.ts (the only test this PR touches) passed on every lane. Red lanes were unrelated: test/js/bun/s3/s3.test.ts (R2 returning 503), worker-transfer-terminate-stress.test.ts on debian 13 x64-asan (pre-existing JSC ExceptionScope::assertNoException abort), and tests marked flaky that passed on retry.

[claude]

LGTM — the vacuous-loop guard from my earlier note is now in place (expect(historyLines).toHaveLength(2)), and both other review threads are resolved.

What was reviewed:

• History::load: strip_suffix(b"\r") runs before the !line.is_empty() check, so a bare \r line becomes empty and is skipped; save() already writes LF-only.
• Test: non-TTY mode does not echo input, so the .history output yields exactly 2 filtered lines; new_three is added to history before eval so the saved-file toEqual holds even though it throws a ReferenceError (which goes to stdout, keeping stderr === "").
• Cross-platform: env_var::HOME reads USERPROFILE on Windows and the test sets both, so the new describe block is correctly ungated.

Extended reasoning...

Overview

One-line fix in src/runtime/cli/repl.rs (History::load): strip a trailing \r from each line after splitting on \n, so CRLF history files (written on Windows or by CRLF editors) don't leave stray CRs on loaded entries. Accompanied by a new test in test/js/bun/repl/repl.test.ts that seeds a CRLF history file, runs .history + a new entry + .exit, and asserts no \r in either the .history output or the re-saved file.

Security risks

None. The change is a byte-suffix strip on a locally-read history file; no new syscalls, no untrusted-input parsing surface, no permission changes.

Level of scrutiny

Low. This is a mechanical string-normalization fix in a non-critical CLI path (REPL history loading), with clear before/after behavior and a targeted regression test. The Rust change is a single idiomatic strip_suffix().unwrap_or() inserted before the existing empty-line filter, so bare-CR lines correctly become empty and are skipped.

Other factors

• My earlier inline nit (possibly-empty historyLines loop) was addressed in commit 8a4f8af with expect(historyLines).toHaveLength(2).
• CodeRabbit's assertion-ordering concern was withdrawn after robobun pointed out the file is pre-seeded by tempDir, so the read cannot mask a subprocess failure — matching neighboring history tests.
• Verified the test is cross-platform: env_var::HOME maps to USERPROFILE on Windows (src/bun_core/env_var.rs:148) and the test sets both env vars; the new describe is correctly not Windows-gated.
• Verified the toHaveLength(2) guard is exact: in non-TTY mode refresh_line does not echo input, so old_one/old_two appear only once each (from cmd_history output).
• CI: robobun reports repl.test.ts passed on every lane; red lanes are unrelated (R2 outage, pre-existing worker_threads flake).

[robobun]

^{Updated 8:55 PM PT - Aug 12th, 2026}

❌ @robobun, your commit 8a4f8af has some failures in Build #93728 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 36510

That installs a local version of the PR into your bun-36510 executable, so you can run:

bun-36510 --bun

[robobun]

Heads-up: #38023 (one line per history entry, so multi-line entries survive a restart) includes this same trailing-CR strip in History::load, since its decode step depends on it, and its CRLF test covers the single-line case this PR tests. If #38023 lands first, this PR can be closed; if this one lands first, #38023 gets a trivial rebase.

[robobun]

Closing in favour of #38023, which now owns this code path. Both PRs change the same loop in History::load; #38023 needs this trailing-CR strip for its own decode step, so it contains this fix as is, and leaving both open meant one of them would have to be rebased whichever landed first.

What was carried over to #38023 (commit 718d951):

• This PR's CRLF case as its own test: a history file of single-line entries with CRLF endings loads with no \r on any entry and is re-saved with LF endings. This PR's test also passes unchanged on a build of repl: keep multi-line history entries on one line of the history file #38023's branch, and the repro from the description (printf 'old_one\r\nold_two\r\n' > $HOME/.bun_repl_history, then .history) prints clean entries there.
• The Co-authored-by trailer for @samuelpatro from repl: strip trailing CR when loading history (Windows CRLF) #30293, so the credit for the fix stays with its author. repl: keep multi-line history entries on one line of the history file #38023's description points back to repl: strip trailing CR when loading history (Windows CRLF) #30293 as well.

No further work is needed on this branch.