Repository navigation
Conversation
…ibility When the REPL puts the terminal in raw mode, pressing Enter sends \r (carriage return) instead of \n (newline). The alert(), confirm(), and prompt() functions only checked for \n, causing them to hang indefinitely when called from the REPL. Closes #27542 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 1:29 AM PT - Mar 1st, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 27544That installs a local version of the PR into your bun-27544 --bun |
WalkthroughAdds CR (\r) and CRLF (\r\n) handling to prompt APIs (alert/confirm/prompt) for raw-terminal input; introduces streamed body byte accounting and a 413 rejection helper to enforce max request body size; and adds tests for prompt CR handling and chunked-request 413 behavior. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/webcore/prompt.zig (1)
155-162: 🧹 Nitpick | 🔵 TrivialConsider the semantic coupling of hardcoded
\r.The function now always terminates on
\rregardless of thedelimiterparameter. This works correctly for the current usage (only called withdelimiter='\n'), but could cause unexpected behavior if the function is ever reused with a different delimiter.The comment at lines 155-156 documents this, which is good. Just be aware this is now effectively a "line reader" rather than a generic delimiter reader.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/webcore/prompt.zig` around lines 155 - 162, The reader currently treats '\r' as an unconditional terminator which couples behaviour to newline semantics; update the condition that checks byte vs delimiter so that '\r' is treated as an alternative terminator only when delimiter == '\n' (i.e., accept '\r' as equivalent to '\n' but do not unconditionally return on '\r'); locate the comparison using the variables delimiter and byte (near array_list.appendAssumeCapacity) and change it to only accept '\r' when the requested delimiter is '\n'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bun.js/webcore/prompt.zig`:
- Around line 155-162: The reader currently treats '\r' as an unconditional
terminator which couples behaviour to newline semantics; update the condition
that checks byte vs delimiter so that '\r' is treated as an alternative
terminator only when delimiter == '\n' (i.e., accept '\r' as equivalent to '\n'
but do not unconditionally return on '\r'); locate the comparison using the
variables delimiter and byte (near array_list.appendAssumeCapacity) and change
it to only accept '\r' when the requested delimiter is '\n'.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/webcore/prompt.zigtest/regression/issue/27542.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| switch (first_byte) { | ||
| '\n' => return .false, | ||
| '\r' => { | ||
| const next_byte = reader.takeByte() catch { | ||
| // They may have said yes, but the stdin is invalid. | ||
| return .false; | ||
| }; | ||
| if (next_byte == '\n') { | ||
| return .false; | ||
| } | ||
| }, | ||
| '\n', '\r' => return .false, |
There was a problem hiding this comment.
🔴 On Windows in cooked console mode (non-REPL), pressing Enter sends \r\n. The new code returns immediately on \r, leaving \n in the stdin buffer. This means sequential calls like confirm("first?"); confirm("second?") will have the second call immediately consume the leftover \n and return without waiting for user input. The fix should consume an optional trailing \n after \r (as the old code did), rather than treating \r as a standalone terminator.
Extended reasoning...
What the bug is
The PR replaces explicit \r\n (Windows CRLF) pair handling with simply treating \r as a standalone line terminator, equivalent to \n. While this correctly fixes the REPL case (where raw terminal mode sends bare \r), it regresses the normal Windows console case where Enter sends \r\n as a two-byte sequence.
How it manifests across all three functions
alert(): The old loop if (byte == '\n') break would read \r (no match, continue), then read \n (match, break) - correctly consuming both bytes. The new loop if (byte == '\n' or byte == '\r') break reads \r and immediately breaks, leaving \n in the stdin buffer.
confirm(): The old code had an explicit '\r' switch case that read the next byte and checked for \n before returning. The new code '\n', '\r' => return .false returns immediately on \r. Similarly, after y/Y, the old code checked for \r followed by \n; the new code returns .true on bare \r.
prompt(): The old code had an explicit else if (first_byte == '\r') branch that read a second byte and checked for \n. The new code returns default immediately on \r. The readUntilDelimiter helpers also now break on \r, leaving \n in the buffer.
Step-by-step proof with a concrete example
Consider this script running on Windows in a normal console (cooked mode):
const a = confirm("first?"); // User presses Enter
const b = confirm("second?"); // Should wait for user input
console.log(a, b);- User sees
first? [y/N]and presses Enter, which sends \r\n to stdin. - confirm() reads first_byte = '\r', matches the '\n', '\r' case, and returns immediately. The \n byte remains in the stdin buffer.
- User sees
second? [y/N]but confirm() immediately reads first_byte = '\n' (the leftover byte), matches the same case, and returns .false without waiting. - Output:
false falsebut the user only pressed Enter once.
The same issue affects prompt() via buffered_stdin (which persists across calls) and alert() followed by any other stdin-reading function.
Why existing tests don't catch this
The new tests only send bare \r (simulating raw REPL mode) and only test single calls. There are no tests for sequential calls, and no tests sending \r\n (simulating Windows cooked mode).
Impact
This is a regression for all Windows users running scripts that use multiple sequential alert(), confirm(), or prompt() calls in normal (non-REPL) console mode. The second call in any sequence will silently consume the leftover \n and return immediately with a default value.
How to fix
After breaking on \r, attempt to read the next byte and consume it if it is \n (restoring the old behavior). This handles both cases: bare \r in raw REPL mode and \r\n in Windows cooked mode. For example, in the alert loop, instead of if (byte == '\n' or byte == '\r') break, use logic that on seeing \r peeks at the next byte and consumes it if it is \n. Similar patterns should be applied in confirm(), prompt(), and both readUntilDelimiter helpers.
…or chunked requests 1. Fix Windows \r\n regression in alert/confirm/prompt: when \r is encountered, consume the optional trailing \n to prevent it leaking into subsequent calls. This handles both bare \r (REPL raw mode) and \r\n (Windows cooked mode). 2. Enforce maxRequestBodySize for chunked/streaming request bodies: track total bytes received via streamed_body_bytes_received and reject with 413 when the limit is exceeded. Previously only Content-Length was checked upfront, allowing chunked requests to bypass the limit entirely. 3. Initialize streamed_body_bytes_received with pre-buffered byte count when transitioning from buffered to streaming mode in onStartStreamingRequestBody. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/http/serve-chunked-413.test.ts`:
- Around line 61-73: Replace uses of string.repeat for large repetitive payloads
with Buffer.alloc(...).toString() to avoid relying on String.repeat: update the
construction of the request variable that currently uses "A".repeat(40) and
"B".repeat(40) to use Buffer.alloc(40, "A").toString() and Buffer.alloc(40,
"B").toString() respectively, and apply the same replacement for the identical
repeat patterns referenced later in the file (the occurrences at the same shapes
around the blocks noted at the other two locations).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/prompt.zigtest/js/bun/http/serve-chunked-413.test.tstest/regression/issue/27542.test.ts
| const request = | ||
| "POST / HTTP/1.1\r\n" + | ||
| `Host: ${server.hostname}:${server.port}\r\n` + | ||
| "Transfer-Encoding: chunked\r\n" + | ||
| "\r\n" + | ||
| "28\r\n" + // 0x28 = 40 in hex | ||
| "A".repeat(40) + | ||
| "\r\n" + | ||
| "28\r\n" + // 0x28 = 40 in hex | ||
| "B".repeat(40) + | ||
| "\r\n" + | ||
| "0\r\n" + | ||
| "\r\n"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use Buffer.alloc instead of String.repeat for repetitive strings.
As per coding guidelines, use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests.
♻️ Proposed fix
const request =
"POST / HTTP/1.1\r\n" +
`Host: ${server.hostname}:${server.port}\r\n` +
"Transfer-Encoding: chunked\r\n" +
"\r\n" +
"28\r\n" + // 0x28 = 40 in hex
- "A".repeat(40) +
+ Buffer.alloc(40, "A").toString() +
"\r\n" +
"28\r\n" + // 0x28 = 40 in hex
- "B".repeat(40) +
+ Buffer.alloc(40, "B").toString() +
"\r\n" +
"0\r\n" +
"\r\n";The same change should be applied to the identical patterns at lines 147-150 and 215-218.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const request = | |
| "POST / HTTP/1.1\r\n" + | |
| `Host: ${server.hostname}:${server.port}\r\n` + | |
| "Transfer-Encoding: chunked\r\n" + | |
| "\r\n" + | |
| "28\r\n" + // 0x28 = 40 in hex | |
| "A".repeat(40) + | |
| "\r\n" + | |
| "28\r\n" + // 0x28 = 40 in hex | |
| "B".repeat(40) + | |
| "\r\n" + | |
| "0\r\n" + | |
| "\r\n"; | |
| const request = | |
| "POST / HTTP/1.1\r\n" + | |
| `Host: ${server.hostname}:${server.port}\r\n` + | |
| "Transfer-Encoding: chunked\r\n" + | |
| "\r\n" + | |
| "28\r\n" + // 0x28 = 40 in hex | |
| Buffer.alloc(40, "A").toString() + | |
| "\r\n" + | |
| "28\r\n" + // 0x28 = 40 in hex | |
| Buffer.alloc(40, "B").toString() + | |
| "\r\n" + | |
| "0\r\n" + | |
| "\r\n"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/http/serve-chunked-413.test.ts` around lines 61 - 73, Replace
uses of string.repeat for large repetitive payloads with
Buffer.alloc(...).toString() to avoid relying on String.repeat: update the
construction of the request variable that currently uses "A".repeat(40) and
"B".repeat(40) to use Buffer.alloc(40, "A").toString() and Buffer.alloc(40,
"B").toString() respectively, and apply the same replacement for the identical
repeat patterns referenced later in the file (the occurrences at the same shapes
around the blocks noted at the other two locations).
| if (byte == '\r') { | ||
| // Consume optional trailing \n for Windows \r\n line endings | ||
| _ = reader.takeByte() catch {}; | ||
| break; | ||
| } |
There was a problem hiding this comment.
🔴 The _ = reader.takeByte() catch {} after reading \r (line 58, and 5 other locations) performs a blocking read. In raw terminal mode (the REPL scenario this PR fixes), Enter sends only \r with no trailing \n, so this read blocks until the user presses a second key — partially defeating the fix. The follow-up read should only consume the next byte if data is already buffered (non-blocking peek), not issue an unconditional blocking read.
Extended reasoning...
What the bug is
The PR adds _ = reader.takeByte() catch {} (or readByte) after every \r detection to consume an optional trailing \n for Windows CRLF line endings. However, takeByte() and readByte() are blocking calls — they issue a read() syscall that blocks until at least one byte is available. In raw terminal mode (the exact REPL scenario this PR is designed to fix), pressing Enter sends only \r with no trailing \n, so the follow-up read blocks waiting for the next byte from stdin.
The specific code path
In alert() at lines 56-60:
if (byte ==
) {
// Consume optional trailing \n for Windows \r\n line endings
_ = reader.takeByte() catch {};
break;
}The reader wraps a std.fs.File (stdin) via readerStreaming with a 1-byte buffer. When all buffered data has been consumed (only \r was available), takeByte() calls the underlying read() syscall. The catch {} only handles read errors (e.g., EOF on a pipe), not blocking — a blocking read simply never returns.
Why the REPL triggers this
The REPL puts the terminal in raw mode at startup (bun.tty.setMode(0, .raw) in repl.zig:757). The raw mode configuration sets VMIN=1, VTIME=0 (confirmed in wtf-bindings.cpp:90), meaning read() blocks until at least 1 byte is available. When the user presses Enter in raw mode, only \r is sent — there is no trailing \n. So after consuming \r, the takeByte() call blocks indefinitely waiting for the next keystroke.
Step-by-step proof
- User starts
bun repl(terminal enters raw mode) - User types
alert("hello")and submits it - Bun prints
hello [Enter]and waits for input - User presses Enter, which sends
\r(just one byte in raw mode) - The
whileloop reads byte =\rat line 54 - Enters the
if (byte == )branch at line 56 - Line 58:
_ = reader.takeByte() catch {}issues a blockingread()syscall - No data is available (raw mode only sent
\r), soread()blocks - The user must press a second key to unblock the read (that key is silently discarded)
Before this PR, alert() hung indefinitely (infinite loop waiting for \n). After this PR, it hangs until a second keypress — an improvement but still broken.
Why the tests pass
All tests use piped stdin with proc.stdin.end() after writing \r. When the pipe is closed, the follow-up takeByte() gets EOF, which is caught by catch {}. But in a real REPL session, stdin is an open terminal file descriptor — it never returns EOF, so the read blocks.
How to fix
The fix should only consume the next byte if it is already available in the reader's internal buffer (a non-blocking peek). For example, after reading \r, check if the streaming reader has any buffered data before attempting to read the trailing byte. If no data is buffered, skip the read. This handles both cases correctly: bare \r in raw REPL mode (no blocking read) and \r\n in Windows cooked mode (consumes the \n from the buffer since both bytes arrive together). The same fix is needed at all six locations: alert() line 58, confirm() lines 121/137/147, both readUntilDelimiter helpers at lines 176/199, and prompt() at line 289.
Summary
alert(),confirm(), andprompt()hanging indefinitely when called frombun repl\rinstead of\n, but these functions only checked for\n\rand\nas line endingsCloses #27542
Test plan
test/regression/issue/27542.test.tsverify all three functions work with\r:alert()accepts\rto dismissconfirm()accepts bare\ras default (no)confirm()acceptsy+\ras yesprompt()accepts\rto return default valueprompt()accepts text +\rto return user inputUSE_SYSTEM_BUN=1), pass with debug buildweb-globals.test.jsconfirm/prompt tests still pass (18/18)🤖 Generated with Claude Code