Repository navigation
Conversation
`createInterface({ input })` stored `size`, `history` and
`removeHistoryDuplicates` on the input stream, then handed the stream to
`setupHistoryManager` as its options. The constructor now hands over a
null-prototype record of its own, as Node.js does since v26.7.0
(nodejs/node#64585). Without an options object the stream is still the
options, as in Node.js.
`$Node.ReadlineInput` no longer extends the history options, so a store
of a history option on an input stream is a type error.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Collect and pass history options src/js/internal/readline/interface.ts |
ReadlineHistoryOptions adds an optional __proto__?: null property, and ReadlineInput no longer extends it. The constructor collects history settings into a null-prototype options object and passes it to setupHistoryManager, falling back to the input stream when no options object is supplied. |
Exercise history option behavior test/js/node/readline/history-options-off-input-fixture.cjs, test/js/node/readline/readline.node.test.ts |
The fixture records readline and REPL behavior, stream properties, validation results, and errors. The test compares its output under Bun and, when available, Node.js 26.7.0 or later. |
Suggested reviewers: alii
Priority:
Merge Risk: 
74910
-
Merge Risk: ⚪ Minimal · up to
74910The change separates history settings from input streams while retaining positional-call compatibility. No actionable merge-blocking issue remains; merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: passing readline history options in a separate record instead of storing them on the input stream. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description follows the repository template. It has both required sections: "What does this PR do?" (as the Problem, Fix, and Background subsections) and "How did you verify your code works?" (as … Full details: Description check
-
Explanation
The description follows the repository template. It has both required sections: "What does this PR do?" (as the Problem, Fix, and Background subsections) and "How did you verify your code works?" (as the Verified, Self-reviewed, and per-binary run tables). It states the bug clearly, names the upstream Node.js change it ports, and gives specific reproduction steps, test commands, and measured costs. Its main weakness is length and density: much of the verification detail sits in a collapsed Notes section, and the heading "What does this PR do?" is not used verbatim. These gaps are minor and do not hide the required information.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
Updated 11:46 AM PT - Oct 29th, 2026
❌ @robobun, your commit 74910a9 has 2 failures in
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.0fe953eb-a2ce-4aa1-b51f-369bac6c8d2d
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.8aa98862-d2f6-484c-a0db-959332ff8aa7
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.a3eec87f-5fe4-4dc5-9840-840833530fbd
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.abccf956-fd39-4853-8d90-b65e6dce414d
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.5acb2cf5-a654-4626-a9ee-a8e8270ecc63
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.99646846-49af-497b-9820-83d1183b3629
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.79e337e8-0d84-4e21-8402-93298cd699aa
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.824a7ae1-5f27-4be4-9c2e-ecea641cd0a5
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.5aa4f02b-70e7-4552-8234-a78f0939f258
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.6970c093-80dd-4727-8e06-4fbd6032feef
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.7ca431df-75a7-4c91-b4e7-ca930c9b696f
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.af3aa904-4075-4baf-a401-9ed4abe125a8🧪 To try this PR locally: bunx bun-pr 47361That installs a local version of the PR into your bun-47361 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings on the test file, I also checked the other callers of setupHistoryManager: src/js/node/repl.ts:1134 already passes its own options record, and there is no ahead-of-time/ twin of internal/readline/interface.ts that would need the same hunk. The positional createInterface(stream) form still handing the stream itself to setupHistoryManager matches Node v26.12.1 interface.js#L226, so it is parity, not a regression.
Extended reasoning...
The runtime change is a four-line hunk in src/js/internal/readline/interface.ts that replaces three property stores on the user's input stream with a null-prototype historyOptions record, plus type declaration tweaks; it touches no security-sensitive surface. The two inline findings are both about the new test in test/js/node/readline/readline.node.test.ts (a per-test timeout override and a byte-exact stderr assertion against the system Node), not the runtime hunk, so a human should weigh them before merge.
|
Status: ready for review. The diff is green on every platform that CI has. What is red in CI is CI infrastructure and one test that this change does not touch. A retry of those jobs needs a maintainer. How I reproduced it. I ran the six-row script from the report and the new fixture (
To run the test: CI.
|
A warning that Node.js writes to stderr no longer fails the Node.js case. A debug build gets 90 s for the new case, the time limit that scripts/runner.node.ts gives each test.
There was a problem hiding this comment.
LGTM — small, focused compat fix that matches upstream Node, and the follow-up commit addressed both earlier inline comments (Node-run stderr is no longer compared on a clean exit; the debug timeout no longer shrinks the CI runner's budget).
What was reviewed:
- Traced the old path's three stream writes against
ReplHistory's constructor reads: the new null-prototype record carriessize/history/removeHistoryDuplicates;filePath/onHistoryFileLoadedare no longer picked up off the stream in the options form, and nothing else insrc/jsreads those keys back from the input. - Checked the legacy
createInterface(stream, ...)form still hands the stream tosetupHistoryManager, matching Node;repl.setupHistorypasses its own options object and is unaffected. - Checked the test harness use:
it.skipIfis valid on the node-harnessit,nodeExeAtLeastalready exists intest/harness.ts, pipes are drained concurrently and asserted as a combined object.
Extended reasoning...
The change touches one hunk of src/js/internal/readline/interface.ts (Bun's port of Node's readline Interface, shared by node:readline, readline/promises, repl and FileHandle.readLines), a new subprocess fixture, and one new test that compares the fixture's 18 rows between Bun and a system Node >= 26.7.0 when available. It touches no security-sensitive surface. The source diff is about ten lines with clear intent, the behavior is pinned empirically against real Node rather than only by description, both prior optional findings were addressed by the second commit, the bug hunt ran dry with no findings, and no CODEOWNERS entry covers these paths.
Problem
readline.createInterface({ input })storessize,historyandremoveHistoryDuplicateson the input stream. ssri'sintegrityStreamthen rejects a valid file (EBADSIZE ... Found: NaN). A stream-buffers input fails withTypeError: input.size is not a function.createInterface(input)hashistorySize3, not 30.src/js/internal/readline/interface.ts:269, ported from Node.js v26.3.0 in node:repl: replace the stub with Node v26.3.0's REPL — v26 readline stack, acorn recoverable-parse + top-level await, completion, history, --interactive (82 vendored upstream tests) #31827. Node.js removed the stores in v26.7.0 (readline: improve performance nodejs/node#64585).Fix
setupHistoryManagera null-prototype record of its own: thehistoryOptionshunk of readline: improve performance nodejs/node#64585.$Node.ReadlineInputno longer extends the history options type, so a store on the stream is a type error.test/js/node/readline/readline.node.test.tsruns one fixture on Bun and on Node.js v26.7.0 or later, and expects the same 18 rows. Bun 1.4.2 differs on 15. Also the 102 vendoredtest-readline-*andtest-repl-*files.Background
setupHistoryManager(options)creates theReplHistoryof anInterfaceand reads the history options fromoptions.readline,readline/promises,FileHandle.readLines()and the REPL construct throughInterfaceConstructor. One hunk covers them.createInterface(stream)too (Node.js reads the stream there).Downsides
createInterface(stream)still throws for a stream whose ownsizeis not a number, as on Node.js v26.12.1. Bun 1.3.14 accepted it.Notes
Which Bun differs from which Node.js. Bun 1.4.0 to 1.4.4 report Node.js v26.3.0 and behave as it does (it has the same three stores). Main reports v26.12.1 since #46533 and still has the stores. Bun 1.3.14 kept the options in locals (
src/js/node/readline.ts:1068-1087atbun-v1.3.14). I found no issue for this in oven-sh/bun. For a built-in stream the effect is three more own properties, and nothing reads them back unless the positional form runs on the same stream later.Runs, by binary. Node.js v26.12.1 is the official linux-x64 build. The six-row script is the one from the report.
input.size() = 2EBADSIZE ... Found: NaNinput.size is not a functionEBADSIZE ... Found: NaNinput.size is not a functioninput.size() = 2EBADSIZE ... Found: NaNinput.size is not a functionssri is 14.0.0 and stream-buffers is 3.0.3, both unmodified:
readline.node.test.tson the same binaries, with Node.js v26.12.1 first onPATH: Bun 1.4.2 and the release build of main fail the Bun case (15 rows) and pass the Node.js case. The debug and release builds of this change pass both (83 tests). The Node.js case is skipped whennodeis older than 26.7.0. After the rebase onto da867c6 the debug build passes both cases again, and so do the vendored files.Not taken, on purpose.
createInterface(stream, ...)still hands the stream tosetupHistoryManager, which readssize,history,removeHistoryDuplicates,filePathandonHistoryFileLoadedfrom it. Node.js does this since v24.2.0 (repl: extract and standardize history from both repl and interface nodejs/node#58225).readlineRegression since v24.2.0Cannot read properties of undefined (reading 'pause')nodejs/node#61526 reports it. That issue was closed as not planned without a comment from a Node.js collaborator, and the fix attempt readline: initialize input before history manager nodejs/node#61538 was closed by the stale bot. So Node.js has not ruled on it, and Bun keeps it only for parity. Two inputs that Bun 1.3.14 accepted still throw there: a stream-buffersReadableStreamBuffer(ERR_INVALID_ARG_TYPE, itssizeis a method) and a stream with anonHistoryFileLoadedproperty (TypeError). The last fixture row pins the parity.TERM=dumbcheck only in terminal mode, and the[kNormalWrite]rewrite (which overlaps readline: split newline-only chunks with indexOf instead of a RegExp match per line #45392). Each is a performance change measured on V8.colorizeoption (nodejs/node c18b0aa54e) is not ported, and the REPL loses lines that are evaluated while the history file loads (repl: keep entries added while history file is loading nodejs/node#64513).__proto__: nullin the record is upstream's text. It is the fifth such literal that runs per construction (the four accessor descriptors insetupHistoryManagerare the others).With a terminal.
terminal: truecallsemitKeypressEvents, which stores two symbol-keyed decoders on the input, as in Node.js. So a terminal interface still adds own symbols, and a non-extensible input still throws there. This change is only about the history options.Cost, measured. Release builds (
bun run build:release) of main 11c3671 with and without the change, same machine.perf,valgrindandbloatyare not installed on it. Instruction counts come from aperf_event_opencounter forPERF_COUNT_HW_INSTRUCTIONSin user mode, which is whatperf stat -e instructions:ureads. Sizes come fromsizeandwc -c.createInterface+close(BUN_JSC_useGC=0, 2000 calls,heapStats().objectTypeCounts): options form 51.175 → 52.175 (+1Objectof 48 bytes), positional form 50.175 → 50.175.InterfaceConstructorbytecode: 318 → 324 instructions.new_object0 → 1,call_ignore_result19 → 20 (the__proto__: nullkey is a call to the@setPrototypeDirecthost function),jnundefined_or_null1 → 2 (the??).put_by_idstays at 16: three now store into the record.createInterface({ input: new PassThrough() }).close(), as (I(n) - I(m)) / (n - m):{ __proto__: null, a, b, c }436 / 376 / 559 / 959, plain{ a, b, c }68 / 37 / 220 / 556,Object.create(null)and three stores 77 / 41 / 333 / 761. The__proto__: nullkey costs about 340 to 400 instructions in every tier.src/jshas 636 such literals.PassThroughinput afterclose: 113.6 → 81.1 (-32). Out-of-line property storage of the input, used/capacity aftercreateInterface({ input }):PassThrough6/8 → 3/4,Transform11/16 → 8/8,fs.ReadStream10/16 → 7/8,net.Socket18/32 → 15/16,process.stdin13/16 → 10/16. The storage is no longer reallocated.require("node:readline"): 3,114 cells kept on both builds. Instructions 15,295,110 against 15,284,999, inside the run-to-run spread.internal/readline/interface.js: 35,279 → 35,324 bytes. Release binary: +45 bytes of text (size), same file size.bun build --compile --aotof a program that makes 2,000 interfaces: 1,507,247 → 1,513,845 bytes of machine code (+6,598). The type change alone (against the same record with the oldextends ReadlineHistoryOptionsand no cast) resizes 0 of 2,341 functions, and the emitted built-in JS is byte-identical.bun src/codegen/check-types-of-modules.ts): 0 errors. With the three stores put back: 3 errors (TS2339). With the cast removed: 1 error (TS2345).Other suites. On the release build of the change:
readline_promises.node.test.ts(3),history-of-interface-and-repl.test.ts(1),getStringWidth.test.ts(3),pause_stdin_should_exit.test.ts(1),readline_never_unrefs.test.ts(1),stdin-pause-pty.test.ts(1),stdin_fell_asleep.test.ts(2),members-of-prototype-objects-for-any-receiver.test.ts(32),test/js/bun/repl/repl.test.ts(129),test/aot/methods-of-library-classes-nobody-calls.test.ts(8), andAOT_PROGRAMS=test/js/node/readlinewithtest/aot/buns-own-tests-as-compiled-code(7). The fixture compiled with--aotprints the same 18 rows (with a real path in place of__filename, which a compiled executable does not have on disk). On the debug build: the 20test-readline-*, 82test-repl-*andtest-tty-stdin-pipe.jsvendored files, run asscripts/runner.node.tsruns them.Self-review, not taken. Two notes ask for a check that lists the upstream commits that touched the ported files when the Node.js version is bumped. That is tooling for the bump and not part of this bug.
Where the test lives. The new case is in
readline.node.test.tsand not inhistory-of-interface-and-repl.test.ts: the existing test in that file needs 4.3 to 5.0 s under a debug build and timed out in 2 of 5 runs here.[human-review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 2
evidence per changed file