Skip to content

node:stream: define the Readable operators with the class and load them on first use - #37395

Open
robobun wants to merge 20 commits into
mainfrom
farm/2927157e/readable-operators-without-stream-umbrella
Open

robobun wants to merge 20 commits into
mainfrom
farm/2927157e/readable-operators-without-stream-umbrella

Conversation

@robobun

@robobun robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A Readable has no map, toArray or the other nine operators until its realm loads node:stream. net.connect(port).toArray() throws TypeError: ... .toArray is not a function. Node has all eleven.
  • src/js/internal/stream.ts:24-63, the module behind node:stream, defined them. net, tls, crypto and child_process get the class from internal/streams/readable and never run that file.

Fix

  • internal/streams/readable.ts defines the eleven with the class. The wrappers are those of main, and internal/streams/operators loads when one is first called.
  • That module reads one replaceable global at load, TypeError.
  • What main's tests record of the stream modules stays, except the fault.
  • Verified: test/js/node/stream/node-stream.test.js (19 new cases fail on main), the 249 vendored test-stream* files and 232 ahead-of-time programs. Self-reviewed: 9 concerns raised, 8 addressed. Rejected: a comment in iter/classic.ts, because the install site already states the Node difference.

Background

  • A built-in module runs once per realm (the main thread or a Worker).
  • Node defines them in lib/stream.js, which its net and tls load. Bun's skip it to load less at startup.
  • Considered an eager require (as node builtins: defer cross-module requires and load-time work #35541): no name and length table, but 1.04M instructions per realm against 0.45M. A maintainer can choose. Considered require("node:stream") in each consumer: 2.9M.

Downsides

  • A realm that gets the class without node:stream runs 0.45M more user instructions (require("node:net"): 23.22M to 23.66M). One that loads node:stream saves 0.69M. The first operator call in a realm costs 0.63M more.
  • Still there: net.Socket.finished and the other statics of Stream need node:stream loaded.
  • Two contrived shapes change (Notes): a replaced Promise.prototype.then, a read-only Array.prototype index.
Notes

History. Opened on 2026-08-11 as a move of the two install loops. Reworked on 2026-10-25 to load the operators on first use. main is merged in since 2026-11-04, now at b1007f1. main's new tests record what a program sees of the stream modules, so the head keeps everything they record except the fault. Three commits on this branch (5ec3ccd, 06d03f0, 15c96ea) took the other way: they ran the promise operators in the call and recorded about 1,000 lines of expected output again. They are merged with this tree kept. All numbers below are from main e57d89b against this head.

Repro. Each line is a fresh process that never loads node:stream itself.

$ bun -e '{ const net = require("node:net"); console.log(typeof new net.Socket().map) }'
undefined        # node v26.3.0 and this PR: function
$ bun -e 'require("node:child_process").spawn("echo", ["hi"], { stdio: ["ignore", "pipe", "inherit"] }).stdout.toArray().then(a => console.log(String(a)))'
TypeError: ... .toArray is not a function        # node and this PR: hi

The same eleven are missing on net.Socket, tls.TLSSocket, the crypto Hash, Hmac, Cipheriv and Decipheriv streams, a child's stdout, stderr and stdio[3] when stdin is not a pipe, worker.stdout, worker.stderr, process.stdin inside a Worker, IncomingMessage.prototype from _http_incoming, and the four _stream_* modules. Found by a load-order sweep of the built-in modules. No user reported it.

Why require("node:stream") is not a dependable workaround.

  • It is per realm. With node:stream loaded on the main thread, process.stdin.toArray inside a Worker is still undefined on main.
  • bun build --target=bun removes a bare import "node:stream". A file that works under bun run throws after it is bundled (checked on main e57d89b).

What stays as on main.

  • The wrappers. A stream operator returns Stream.Readable.from(...) and reads Stream.Readable when it is called. Before node:stream loads it uses the class itself. A promise operator still runs from Promise.resolve().then(...).
  • The keys of node:stream: readable.ts does not set Stream.Readable.
  • The order in which Object lists its members. internal/stream.ts read Object.keys ahead of Object.defineProperty when it loaded, and it still does. A compiled executable replays these reads from a list (readMembersThatMakingStandardErrorReads in BunProcess.cpp), which stays right.
  • Every expected file and snapshot of test/js/node that records reads, Proxy traps and visible surfaces.

Tests of main that change.

  • stream-operators-arrive-with-first-pipe (the program under test/aot/parallel/library-modules and its expected file) recorded the fault: no operators with node:net alone. It is now stream-operators-come-with-the-readable-class, and every row has the eleven.
  • test/aot/a-built-in-module-that-threw-while-it-loaded-runs-again.test.ts and test/aot/the-first-read-of-a-standard-stream-calls-what-a-test-has-replaced.test.ts made the stream modules throw with an Object.keys that answers ["nonsense"]. Only the loops of internal/stream.ts threw on that answer. The answer is now undefined, which the other callers throw on. Both files pass with it on main too.

The choice for a maintainer. Release builds, user instructions, median of 11 interleaved runs. The eager column is the head of 2026-11-04 with internal/streams/operators required when readable.ts loads.

bun -e script main e57d89b this PR (lazy) eager
require("node:net") 23.215M +0.447M +1.038M
require("node:tls") 25.926M +0.414M +1.037M
require("node:crypto") 21.104M +0.407M +1.066M
require("node:_stream_readable") 15.863M +0.515M +1.147M
require("node:stream") 20.486M -0.687M -0.068M
process.stdout 26.998M -0.692M -0.071M
require("node:http") 36.550M -0.701M -0.064M
require("node:stream"), then the first map() 22.562M -0.050M -0.017M

Eager needs no name and length table, and it reads the captured globals when the class loads, as the rest of readable.ts does. Lazy follows Readable.prototype.compose (readable.ts:1380), which already loads its implementation on first call. The table has no prototype and is walked with for...in, so neither a replaced Object.keys nor a key added to Object.prototype changes which methods a Readable gets. It is tied to the exports of operators.ts with satisfies: the typecheck of the built-in modules fails when an operator is added there or removed. require("node:net") followed by require("node:stream") costs 2.90M more than require("node:net") alone on main. That is the price of a node:stream require in each consumer.

Other measurements (main against this head, same builds).

  • Per node Worker realm: +0.47M user instructions (17 against 1 sequential empty Workers, median of 9, noise 0.09M).
  • First operator call in a realm loads internal/streams/operators: the first map() costs 0.57M on main with node:stream loaded and 1.21M here, once.
  • Warm operator call: toArray() 3710 to 3786 instructions per call (noise 91). 0 allocations more per call (2e5 calls, heapStats().objectCount).
  • GC cells per realm at class load: net 4585 to 4679, crypto 4322 to 4419, _stream_readable 2934 to 3028 (about +9 KB heap). node:stream: 3718 to 3602.
  • Operator installs across require("node:net") then require("node:stream"): 11 then 0 more (main: 0 then 11). Evaluations of operators.ts before the first operator call: 0 (main: 1, at require("node:stream")).
  • Binary text: 140,286,627 to 140,286,035 bytes (-592). --compile --bytecode: crypto and net one-liners +0 B, stream one-liner -4,096 B.
  • Budgets of the ahead-of-time servers (test/aot/servers/budgets.test.ts, seven servers): all within. The node server makes 1807 closures at start where main makes 1812, with the same code bytes.
  • Wall clock was not measured on this base. On the base of 2026-10-25 two copies of one binary differed by 0.1 ms, which is the size of the expected delta.
  • Tools: instruction counts come from a small perf_event_open program (instructions:u, children and threads included). valgrind, perf, strace and bloaty are not in the container.

Surface changes besides the fix.

  • Differs from Node: under --experimental-stream-iter, toReadable() has the eleven in a process that has not loaded stream. Node v26.3.0 has none there.

Not in this PR.

  • The promise operators run one promise job after the call, as on main. r.toArray(); r.destroy(err) still ends the process with an uncaught 'error', where Node rejects the promise. The fix changes what main's stream tests record, so it needs a PR of its own.
  • The other statics of Stream (finished, pipeline, isDisturbed, ...), set in internal/stream.ts, stay dependent on load order.
  • So do the emitter methods of a global MessageChannel port (docs/runtime/nodejs-compat.mdx:340) and three members of the global performance (nodejs-compat.mdx:132).

Self-review (2026-10-25, before the merges of main). Nine concerns survived. Addressed: the module-load reads in operators.ts, the untested .bind change (now pinned by the restored-global test), the debug-only assert (replaced by the type tie), the two-line comment, and four points in this text (rework not rebase, the eager choice with per-mode numbers, no user report and the workaround, the sites left out). The ninth, the Node difference, is stated here and in the test header, without the code comment. Two recommendations were applied in another form:

  • Math.floor: the review kept the capture and listed a residual. This PR uses a local floor with no residual. It agrees with Math.floor on 5,000,059 inputs (edge values and random doubles) on JSC and V8.
  • drop() and take(): the review gave them Node's options = undefined default. That adds one GC cell per call, so they keep the signatures of main.

Rejected: a code comment in internal/streams/iter/classic.ts about the Node difference. That file only requires the class. The comment at the install site in readable.ts states what Node does and why Bun differs.

Suites run at the head. Release build: test/js/node/stream/ (12 files with the three below, 275 pass), test/js/node/visible-surface.test.ts, test/js/node/members-of-prototype-objects-for-any-receiver.test.ts, test/js/node/http/node-http-proxy-traps.test.ts, all 232 programs under test/aot/parallel/process and test/aot/parallel/library-modules through test/aot/run-parallel.ts, and 80 test files under test/aot that touch streams, stdio or replaced built-ins (four fail, the same four with the same counts on main). test/js/node/events, zlib, tty, readline, child_process, process, worker_threads, net and the top-level files of test/js/node give the same counts on main and here. Debug ASAN build: node-stream.test.js (142 pass), all 249 test/js/node/test/parallel/test-stream* files and 64 vendored net, tls, crypto, child_process, worker and webstreams files. Also bun src/codegen/check-types-of-modules.ts, bun fmt --check and bun lint. Linux x64 only: macOS and Windows were not run locally.

Three contrived shapes that change. A maintainer's fuzz run on 11-08 found them at 72581c2, three runs each, against main 2596ac5 and Node 26.12.1. Node v26.3.0 and Bun 1.4.3 give the same answers for Node and main here. The cause of each is in the diff.

shape main this PR Node
1. TypeError is replaced during the first toArray() and put back. Then the error of Readable.from([]).reduce(f) is instanceof TypeError true false true
2. calls of a Promise.prototype.then that the program replaced, for one awaited toArray() 2 1 0
3. a read-only index 1 on Array.prototype, toArray() of 3 items rejects, TypeError resolves, 3 items rejects, TypeError
  1. operators.ts has class ReduceAwareErrMissingArgs extends TypeError, which reads TypeError when the module loads. On main it loads with node:stream. Here it loads at the first operator call. With one operator call ahead of the mock the answer is true on all three.
  2. The wrapper chains with the private then now. The promise of the wrapper is still resolved with the promise of the operator, and that step reads then. node:stream: start toArray, forEach, reduce, some, every and find inside the call #47989 returns the promise of the operator, which ends the call.
  3. toArray() appends with $arrayPush. It defines the index on the array and asks no index of Array.prototype. Main called Array.prototype.push.

no test proof · iteration 7 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/stream/node-stream.test.js

@coderabbitai

coderabbitai Bot commented Aug 11, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • Resume reviews
  • Trigger review

No actionable comments were generated in the recent review.

Walkthrough

Readable stream operators are installed on Readable.prototype and load their implementations on first invocation. The changes update operator internals and add tests for operator availability, descriptors, results, errors, and stream integrations.

Changes

Readable stream operators

Layer / File(s) Summary
Update operator implementation details
src/js/internal/streams/operators.ts
A local floor helper replaces the cached Math.floor call. Promise settlement, array appends, and NaN checks use direct built-in calls. Explicit length overrides for drop and take are removed.
Install operators on Readable
src/js/internal/streams/readable.ts, src/js/internal/stream.ts
Eleven operators are installed on Readable.prototype with assigned names and lengths, and non-enumerable, configurable, writable properties. The methods load implementations on first invocation, reject construction with new, and return streams or promises. The previous registration loops are removed from stream.ts.
Test operator availability and behavior
test/js/node/stream/node-stream.test.js, test/js/node/stream/*, test/aot/*, test/expected-durations.json, test/aot/parallel/library-modules/*
Tests cover operator availability across stream-related modules, property descriptors, results, errors, replaced globals, prototype changes, and stream integrations. Stream test names and expected outputs change. Two AOT test stubs now make Object.keys return undefined.

Suggested reviewers: jarred-sumner

Merge Risk: Minimal · up to b9649

No actionable merge-blocking defect is established for this change.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: defining Readable operators with the class and loading their implementation on first use.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description covers the required template sections. The "What does this PR do?" equivalent is the Problem, Fix, Background, Downsides, and Notes content. It explains that the eleven Readable operat…
Full details: Description check

Explanation

The description covers the required template sections. The "What does this PR do?" equivalent is the Problem, Fix, Background, Downsides, and Notes content. It explains that the eleven Readable operators are missing on Readables from modules that load the class without loading node:stream (for example, net.Socket().map is undefined and child-process stdout.toArray() throws), and it states the fix: moving the operators into internal/streams/readable.ts and loading internal/streams/operators on first call. The "How did you verify your code works?" equivalent is covered in detail: the 19 new node-stream.test.js cases that fail on main, the 249 vendored test-stream files, 232 ahead-of-time programs, type checks, formatting, and lint, with the Linux x64-only limitation stated. It also gives measured performance tradeoffs, the test expectations that change, and known behavior differences from Node. The description is long and dense, and the template's section headings are not used verbatim. Some verification is deferred to CI for macOS and Windows, and the bot-generated evidence line flags that no platform-specific test proof was run locally.

  • 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.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between a61312a and b964999.

Files selected for processing: 1

  • src/js/internal/streams/readable.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

@robobun

robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on main e57d89b (debug and release builds), in a fresh process that never loads node:stream.

  • bun -e '{ const net = require("node:net"); console.log(typeof new net.Socket().map) }' prints undefined. Node prints function.
  • spawn("echo", ["hi"], { stdio: ["ignore", "pipe", "inherit"] }).stdout.toArray() throws TypeError: ... .toArray is not a function. Node prints hi.
  • The 19 new cases in test/js/node/stream/node-stream.test.js fail with the src/ of main (123 pass, 19 fail) and pass with this branch (142 pass, 0 fail) on the debug ASAN build.

The head a61312a has main b1007f1 merged in. It keeps what the tests of main record of the stream modules: the wrappers of main, the keys of node:stream, the order in which Object lists its members. One program of main recorded the fault and now shows the eleven operators from the start. Two ahead-of-time tests use another Object.keys answer. The PR body lists them.

The promise operators still run one promise job after the call, as on main. Three earlier commits on this branch changed that and recorded the expected files of main again. The head does not take them.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — straightforward relocation of the operator-install loops from internal/stream.ts to the end of internal/streams/readable.ts.

What was reviewed:

  • Verified internal/streams/operators only requires validators, shared and end-of-stream, all of which readable.ts already loads — no new require cycle.
  • Checked the moved block is byte-identical to the original except Stream.Readable.from → Readable.from and the loop-variable rename to avoid shadowing streamKeys used in wrap().
  • Confirmed ObjectKeys was the only removed helper in stream.ts and is already defined in readable.ts; no dead constants left behind.
  • Tests spawn fresh processes with piped stdout/stderr drained concurrently, use listen(0), and cover both operator groups end-to-end.
Extended reasoning...

Overview

The PR moves two for loops that install the eleven Readable operator wrappers (map/filter/flatMap/drop/take and every/forEach/reduce/toArray/some/find) from src/js/internal/stream.ts (the node:stream umbrella) to the bottom of src/js/internal/streams/readable.ts (where the Readable class is defined). The umbrella module drops the loops and its now-unused ObjectKeys/operators imports. Nine subprocess tests are added to test/js/node/stream/node-stream.test.js.

Security risks

None. This is a pure module-load-order fix in built-in JS; no user input parsing, no auth/crypto/permissions surface. The wrapper bodies (including the new.target guard, __proto__: null descriptors, and ``) are unchanged.

Level of scrutiny

Low-to-medium. The moved code is verbatim modulo two mechanical renames: Stream.Readable.from becomes the local Readable.from (defined ~20 lines above the new block, and still looked up at call time so user monkey-patching semantics are preserved), and streamKeys/promiseKeys become opStreamKeys/opPromiseKeys to avoid shadowing the existing streamKeys local inside Readable.prototype.wrap. The main risk with this kind of move is a require cycle; I checked internal/streams/operators.ts and it only pulls in internal/validators, internal/shared and internal/streams/end-of-stream, all three of which readable.ts already required at its top, so the dependency graph is unchanged.

Other factors

The tests follow the harness conventions well: it.concurrent.each over the seven affected entry points, subprocess pipes drained via Promise.all, a combined { stdout, stderr, exitCode } assertion, listen(0, "127.0.0.1") for the net test, and no external network or sleeps. The PR description explains why this diverges from Node's lib/stream.js placement (Bun's internal consumers bypass the umbrella on purpose) and notes the interaction with #35541. The bug hunter found nothing. This is small, self-contained, and well-covered — safe to approve without a human pass.

Comment thread src/js/internal/streams/readable.ts Resolved
@robobun

robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:10 PM PT - Nov 8th, 2026

❌ @robobun, your commit 0a9c082 has 1 failures in Build #138839 (All Failures):

  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.e5f543b7-10a8-4c5e-b636-11cb0e959bc7
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.d992193c-0fa1-4715-8b8d-60df5a25e4e4
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.4a48cd8e-0b7e-4fc8-b627-975270cd51d3
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.ff9b139b-0403-4419-9c2a-aad1e01327bf
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.aee6bad7-dbe1-44bb-b94f-5b66bf5bbdc4
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.2b36468c-254a-4773-997b-2c9e9862f4a6
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.dace147d-d957-4575-bc48-d8d002415147
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.c09d2c00-9def-4baa-8cb7-c54599cd4da9
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.a95c550b-bf53-4b4b-b6bf-33a8f74df366
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.e5b3f36a-c60c-4501-9873-704e4b4545fc
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.7b5a55a3-595b-4c26-8b94-e5c012d7993a
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.32c96ebb-4fda-4eb1-bde2-6590c6a8a634
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.f1861dff-803d-4a7f-b362-0fc3b540bcac
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.90ef4a6d-c516-43ea-8d31-e5cf063db971
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.e0125ae6-7a02-4ddc-8755-a76e7ccf13ad
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.e8c60650-deff-4fdb-9690-3b60636be309
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.99e31458-898f-4496-98e9-c35260ad7f1c
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.cbf0a775-be38-4907-a6e2-a000396b1730
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.6e1b9ed3-d573-4f0f-981d-b9bd4e34a95d
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.6d2683e0-2a66-4e8f-ba97-91500c643ed4
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.e781b490-876a-4c69-a98d-4082b0442963
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.0985bb1b-7fef-4623-b439-e05603664b73
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.9e3e6bcd-ff25-4997-b05c-c349495cc4ad
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.2b593986-4b35-4c5d-9ce0-807954778d43
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.f8bae594-11df-44c2-ad21-8fc51d924555
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.36fdc169-ac82-4878-a761-e59c62b8c7a4
  • Failed to create agent for 🐧 aarch64 - verify-baseline
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.6d81972e-ed7b-4c58-b7f2-2b636e92a295
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.11029eb5-dc6e-4528-b5f2-4669fd0dc560
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.40b69dff-2db4-41e2-a270-a03e23d872a0
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.c70dbb01-c335-41bf-9f7e-55d43a6c2f30
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.36bd05d4-ec3e-4615-ba5c-e7b802c7de12
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.ff198480-b7b2-4c55-afb2-a6a7162d2943
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.87dfe7fc-5557-4ad7-b319-b20994f095c7
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.ef7b95f4-0520-4850-8979-a8c26194392c
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.528daa10-4cfd-427f-a239-62e14ea06d64
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.ccea3941-d92a-462f-b12b-1bbb0183f14c
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.f1052a4a-2327-49e8-bf84-5dd3a179df1a
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.17675e05-2bfa-4c38-b452-ef7530e71428
  • Failed to create agent for 🐧 13 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.f3e4e754-bfe1-4162-b5ac-76d5dc954ee1
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.eda4c3dd-2508-4754-a9fd-b042ceba3b30
  • Failed to create agent for 🐧 25.04 aarch64 - test-bun
  • CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.61e9627b-0882-49c1-83d5-73bc6ce56f36

🧪   To try this PR locally:

bunx bun-pr 37395

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

bun-37395 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — this is a straight relocation of the existing operator-install loops from internal/stream.ts to the tail of internal/streams/readable.ts, so the class is complete regardless of which module reaches it first.

What was reviewed

  • Verified internal/streams/operators only requires validators/shared/end-of-stream (all already loaded by readable.ts) — no require cycle.
  • Wrapper bodies are byte-identical to the removed ones except Stream.Readable.from → Readable.from (same object; still looked up at call time), and Readable.from is defined earlier in the file.
  • ObjectKeys/ObjectDefineProperty are both in scope; the opStreamKeys rename avoids reusing the streamKeys local from wrap().

The comment-cop bot flag on line 1729 looks like a false positive — that's a two-line note explaining why the install location diverges from Node's lib/stream.js, not a workaround justification.

Extended reasoning...

Overview

Moves the two loops that install map/filter/flatMap/drop/take/forEach/toArray/some/every/find/reduce on Readable.prototype from src/js/internal/stream.ts (the node:stream umbrella) to the end of src/js/internal/streams/readable.ts (where Readable is defined). The umbrella now just requires internal/streams/readable and gets a fully-populated prototype. Nine subprocess tests are added to test/js/node/stream/node-stream.test.js covering net.Socket, crypto.createHash(), child-process stdout, and the four _stream_* module prototypes, plus two functional tests that actually run the operators.

Security risks

None. No user-controlled input parsing, no auth/crypto/permission surface. The operator wrappers themselves are unchanged (still use ., __proto__: null descriptors, and new.target guards), and no validation was weakened.

Level of scrutiny

Low-to-medium. This touches the widely-used node:stream compat layer, but the change is a mechanical cut-and-paste of ~40 lines. The only semantic delta is Stream.Readable.from(...) → Readable.from(...), which is the same function on the same object. I verified internal/streams/operators has no dependency on readable (it only requires validators, shared, end-of-stream), so moving the require earlier introduces no cycle. ObjectDefineProperty is newly captured at module top; ObjectKeys was already there for wrap().

Other factors

  • Tests are hermetic (spawn fresh bun -e processes, block-scope the script to avoid bun -e's builtin globals accidentally loading node:stream), use it.concurrent, drain both pipes with Promise.all, and assert the full {stdout, stderr, exitCode} object. The PR description confirms they fail under USE_SYSTEM_BUN=1 and pass with the change.
  • The removed ObjectKeys and streamReturningOperators/promiseReturningOperators imports from stream.ts have no other references in that file.
  • The github-actions comment-cop bot flagged the two-line comment above the new block. That comment explains why Bun installs here rather than mirroring Node's lib/stream.js — it's short, factual, and answers the "why not the alternative" question the review guidelines call for. Not a workaround; not a blocker.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — moves the existing operator-install loops from internal/stream.ts to the end of internal/streams/readable.ts so the prototype is complete regardless of which module loads it first.

What was reviewed:

  • Confirmed internal/streams/operators only requires validators, shared and end-of-stream, so no cycle is introduced by requiring it from readable.ts.
  • Wrapper bodies are byte-identical to the removed ones except Stream.Readable.from → Readable.from (same function, still looked up at call time).
  • ObjectKeys was already in scope in readable.ts; ObjectDefineProperty alias is added alongside the existing ObjectDefineProperties.
  • Tests spawn fresh block-scoped -e processes so node:stream never loads, cover all eleven operators across seven entry points, and functionally exercise map/filter/toArray.
Extended reasoning...

Overview

The PR relocates two for loops (~40 lines) that install the eleven Readable stream operators (map, filter, flatMap, drop, take, forEach, toArray, some, every, find, reduce) on Readable.prototype. Previously they ran in src/js/internal/stream.ts (the module behind node:stream); now they run at the bottom of src/js/internal/streams/readable.ts where Readable is defined. The umbrella module drops the loops and its now-unused ObjectKeys alias and operators require. A new describe block in test/js/node/stream/node-stream.test.js adds nine subprocess tests.

Security risks

None. This is a pure code move within built-in JS modules — no user input handling, no native code, no new API surface. The wrapper functions themselves are unchanged.

Level of scrutiny

Low-to-medium. The diff is small and mechanical: the two loops are copied verbatim, with Stream.Readable.from replaced by the local Readable.from (identical target) and the loop variable names renamed to avoid shadowing the existing streamKeys local in Readable.prototype.wrap. The main risk with a move like this is a require cycle, and I verified internal/streams/operators depends only on internal/validators, internal/shared and internal/streams/end-of-stream — all three are already required by readable.ts above the new block, so evaluation order is unchanged.

Other factors

  • ObjectKeys and ObjectDefineProperty are both in scope in the new location (the former already existed for wrap(), the latter is added).
  • The property descriptors (name, length, non-enumerable/configurable/writable) are preserved exactly, so Node compat for Object.getOwnPropertyDescriptor(Readable.prototype, 'map') etc. is unchanged.
  • Tests follow harness conventions: bunEnv/bunExe, concurrent draining of stdout/stderr/exited, it.concurrent.each for the matrix, port 0 for the net test, and a combined-object toEqual assertion. The block-scoping of -e scripts (to avoid Bun's lazy builtin globals loading node:stream) is documented and was iterated on in commit 3eb269e.
  • The comment-cop bot's inline note about a long comment was addressed in 98c9637 (shortened to one line) and the thread is resolved.

…lla module

The map/filter/forEach/toArray/... operators were defined on
Readable.prototype by internal/stream.ts, so any Readable reached without
requiring node:stream (net.Socket, tls, crypto hashes, child_process stdio,
_stream_readable and friends) lacked them until something else loaded the
umbrella module. Install them at the end of internal/streams/readable.ts so
every Readable has them as soon as the class exists.
Define the eleven operator methods at the end of internal/streams/readable.ts
from a table of names and lengths, and require internal/streams/operators
when one of them is first called. A process that loads net, tls, crypto or
child_process and never calls an operator does not evaluate that module.

The wrappers call Stream.Readable.from, as Node's do, so readable.ts sets
Stream.Readable. This also repairs the src/js typecheck of the moved block.

internal/streams/operators can now run its module body after user code, so
it no longer looks up Function.prototype.bind there.

The tests cover each module that reaches the class and never loads
node:stream, the names, lengths and attributes of the eleven properties,
and the first call of an operator.
internal/streams/operators now loads on the first operator call, which can
be while a test has a global mocked. Its module body captured Math.floor,
Number.isNaN, Array.prototype.push and Object.defineProperty, so a mock that
was restored afterwards stayed in use.

- toArray() uses $arrayPush, and the NaN check is a self-comparison.
- map() floors its options with a local function.
- drop() and take() no longer set their own length. The wrappers in
  readable.ts take their length from the table there.

The name and length table in readable.ts is now tied to the exports of
operators.ts by the type checker. The debug assert is gone: no CI lane
compiles it in.
@robobun
robobun force-pushed the farm/2927157e/readable-operators-without-stream-umbrella branch from 98c9637 to e2be90e Compare October 25, 2026 16:57
@robobun robobun changed the title node:stream: install the Readable operators with the class instead of the node:stream module node:stream: define the Readable operators with the class and load them on first use Oct 25, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked two things and ruled them out: the hard-coded length table in readable.ts matches every operator signature in operators.ts (including drop: 1, which replaces the deleted defineProperty override and matches Node), and the new Stream.Readable = Readable assignment at class-load time is the same object internal/stream.ts still assigns, so require("node:stream").Readable is unchanged.

Extended reasoning...

The change moves the eleven Readable operator installs from the node:stream umbrella into internal/streams/readable.ts with lazy loading of operators.ts, strips load-time global reads from operators.ts, and adds subprocess tests; it touches no auth, crypto or injection surface. Four inline findings were posted, and the ruled-out items above were verified from the current diff against the operator function signatures and the legacy Stream wiring.

Comment thread src/js/internal/streams/readable.ts Resolved
Comment thread src/js/internal/streams/operators.ts Resolved
Comment thread test/js/node/stream/node-stream.test.js Resolved
Comment thread src/js/internal/streams/readable.ts Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

The latest push folds both install loops into installOperator and routes the promise wrappers through $then, which covers what I raised earlier on those lines. Beyond the inline finding, I also checked the hand-written floor() in operators.ts against Math.floor for -0, negative fractions, Infinity and NaN (same results), and that ObjectKeys and Stream are already bound in readable.ts so the new install code reads no new global at class load; the only other oddity found was forEach: 2 in the length table where Node reports 1, which is pre-existing and carried over from the operator's own signature.

Extended reasoning...

The change moves the eleven Readable operator installs from internal/stream.ts into internal/streams/readable.ts with a lazy require of the operators module, removes load-time global captures in operators.ts, and adds a describe block of subprocess tests; it touches no auth, crypto or injection surface, only node:stream internals. It is a load-order and perf tradeoff the author explicitly leaves to a maintainer, and one confirmed (pre-existing) finding is posted inline, so a human decision is still needed rather than an automated approval.

Comment thread src/js/internal/streams/readable.ts Resolved
A toArray(), forEach(), reduce(), some(), every() or find() call went
through a promise job before the operator ran. A destroy(err) right
after the call emitted 'error' with no listener, and the process died.
Node calls the operator at once, so the promise rejects with err.
Comment thread src/js/internal/streams/readable.ts Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed the latest push and found no bugs; the earlier inline feedback (shared install helper, no public .then, synchronous promise operators) is now reflected in the code. Because this changes when internal/streams/operators loads and the sync-vs-microtask timing of the promise operators on every Readable (net, tls, child_process, crypto streams included), a human look is still worthwhile.

What was reviewed:

  • The hand-written floor() in operators.ts against Math.floor for NaN, ±Infinity, -0, negatives and non-numeric coercion — results match for everything validateInteger then sees.
  • Stream.Readable lookup through internal/streams/legacy and the ObjectKeys/ObjectDefineProperty captures in readable.ts — both exist at module scope, and readable.ts is now the only consumer of the operators module.
  • The deleted drop/take length overrides — the wrapper's hard-coded lengths (1) are what Node exposes, and nothing else reads the raw operator's length.
Extended reasoning...

The diff moves the eleven Readable iterator-helper operators from src/js/internal/stream.ts into src/js/internal/streams/readable.ts, installs them through one helper with a lazy require of internal/streams/operators, and strips module-scope reads of replaceable globals from operators.ts; it touches no security-sensitive surface. Two commits since the last review (dffe9ba, 7d6afea) switched the promise operators to a synchronous op.$apply as Node does and added a destroy()-right-after test. Production changes are about 80 lines and well covered by the new subprocess test matrix, but the change alters observable timing on a core node:stream path shared by several modules, and an unread github-actions inline comment on readable.ts:1779 from the final push remains open, so a human should confirm before merging.

main now declares $Node.Readable and $Node.ReadableConstructor. The install
block uses them for Stream.Readable, for `this` and for the operator
functions, and reads the operator names as keys of the tables.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline finding, I also checked the two new hard-coded *OperatorLengths tables in src/js/internal/streams/readable.ts against Node's lib/internal/streams/operators.js signatures and the effective Function.length of each function in src/js/internal/streams/operators.ts (drop/take/toArray/some/every = 1, map/filter/flatMap/forEach/find = 2, reduce = 3) - they match, so dropping the ObjectDefineProperty(drop/take, "length") overrides loses nothing. The local floor() replacement agrees with Math.floor on -0, negative fractions, NaN and ±Infinity.

Extended reasoning...

This push replaces the previous op.length/op.name reads with a hard-coded length table and a hand-written floor(), both of which are second copies of facts that live elsewhere; I verified each against its source of truth and found no mismatch. The one confirmed finding (checked-in expected outputs that still pin the pre-PR load-order behaviour) is posted inline and is what still needs the author's attention.

Comment thread src/js/internal/streams/readable.ts Resolved
…s, as anonymous functions

A for-in over each table reads no replaceable global. The wrapper is a
function expression again, so console.log() of a stream prints
[Function] for each operator, as before. The AOT test that made the
first load of node:zlib throw through a nonsense Object.keys now makes
Object.keys throw, since nothing in that load path reads its answer
any more.
…w make

The promise operators run at once, so the recorded reads of toArray()
and the other five lose their 'later' marker. Object.keys is first read
by internal/streams/readable after internal/streams/state loads, so the
order of the members of Object moves in the list that a compiled
executable reads when it makes process.stderr, and in the stored order.
The program that showed the operators arriving with the first pipe()
now shows them there with the class, under a new name.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


  • Fix comments: Fix CodeRabbit comments on this PR
Prompt to fix all issues
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. 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:
Review comments at @src/js/internal/streams/readable.ts:
- Line 1870: Update both operator-installation loops, including the loop over
streamOperatorLengths and its corresponding operator table, to process only own
table entries. Preserve the existing operator installation behavior while
preventing inherited properties from overriding Readable.prototype methods such
as pipe.

After applying the fix, consider running `coderabbit review --agent` for local
review. Visit https://docs.coderabbit.ai/cli?utm_source=ghpr
Review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between a7dfd22 and 06d03f0.

Files ignored due to path filters: 2

  • test/js/node/__snapshots__/members-of-prototype-objects-for-any-receiver.test.ts.snap is excluded by !**/*.snap
  • test/js/node/__snapshots__/visible-surface.test.ts.snap is excluded by !**/*.snap
Files selected for processing: 13

  • src/js/internal/streams/readable.ts
  • src/jsc/bindings/BunProcess.cpp
  • test/aot/a-built-in-module-that-threw-while-it-loaded-runs-again.test.ts
  • test/aot/parallel/library-modules/keys-of-built-in-objects-behind-modules-that-read-them.mjs
  • test/aot/parallel/library-modules/stream-operators-come-with-the-class.js
  • test/aot/parallel/library-modules/stream-reads-seen-stream-made-awaited.js
  • test/expected-durations.json
  • test/js/node/http/node-http-proxy-traps.expected.txt
  • test/js/node/stream/node-stream-proxy-traps.expected.txt
  • test/js/node/stream/node-stream-reads-and-messages.test.ts
  • test/js/node/stream/stream-operators-arrive-with-first-pipe.expected.txt
  • test/js/node/stream/stream-operators-come-with-the-class.expected.txt
  • test/js/node/stream/stream-reads-seen-without-proxy-receiver.expected.txt
Files with no reviewable changes: 1

  • test/js/node/stream/stream-operators-arrive-with-first-pipe.expected.txt
Files not fully reviewed because their changes were unavailable: 3

  • test/aot/parallel/library-modules/stream-reads-seen-stream-made-awaited.js
  • test/js/node/stream/node-stream-proxy-traps.expected.txt
  • test/js/node/stream/stream-reads-seen-without-proxy-receiver.expected.txt

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Comment thread src/js/internal/streams/readable.ts Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/js/internal/streams/readable.ts Resolved
A for-in over the tables walked Object.prototype too, so an enumerable
key that user code added there before the class loaded became a
method of Readable.prototype. The captured Object.keys returns the own
keys only, as the install in internal/stream.ts did.
…node:stream

main now has tests that hold what a program sees of the stream modules. The
operator install keeps all of it but the fault:

- The wrappers are those of main again. A promise operator runs from
  Promise.resolve().then(), and a stream operator reads Stream.Readable
  when it is called. readable.ts does not set Stream.Readable, so the keys
  of node:stream keep their order.
- internal/stream.ts reads Object.keys ahead of Object.defineProperty when
  it loads, as before, so Object lists its members in the same order.
- The install names the operators with for-in over a table that has no
  prototype. It calls no Object.keys, so a replaced one cannot change which
  methods a Readable gets.

Tests:
- stream-operators-arrive-with-first-pipe becomes
  stream-operators-come-with-the-readable-class: every row has the eleven.
- Two ahead-of-time tests made the stream modules throw with an Object.keys
  that answered an array. The loops that threw on it are gone, so it now
  answers undefined, which the remaining callers throw on.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the latest push (through 15c96ea) and found no new bugs; a maintainer look is still worthwhile because the PR itself leaves the lazy-vs-eager install choice open and the move changes startup cost for every realm that loads net, tls, crypto or child_process.

What was reviewed:

  • The installOperator loops in readable.ts now iterate the captured Object.keys of the two tables, so only the eleven own keys reach Readable.prototype; Stream.Readable is set before the wrappers read it, and the satisfies clauses tie the length table to the exports of operators.ts.
  • The floor() replacement in operators.ts: checked NaN, ±Infinity, -0, negative fractions, BigInt and Symbol inputs against Math.floor — same results and same throws.
  • The synchronous promise-operator call: the three proxy-trap/reads expected files, the two snapshots and the AOT key-order fixture plus BunProcess.cpp list agree with each other and with the new tests; the AOT reload test's switch from a nonsense Object.keys to a throwing one is explained in commit 5ec3ccd and keeps the test's purpose (a throw during load, then a clean retry).
Extended reasoning...

The change moves the eleven Readable iterator-helper operators from src/js/internal/stream.ts into src/js/internal/streams/readable.ts as lazily-loading wrappers, drops module-load global captures in operators.ts, reorders one member list in BunProcess.cpp, and updates seven recorded-output fixtures plus a new 350-line test block in test/js/node/stream/node-stream.test.js. It touches no auth, crypto-key or injection surface; the security-relevant aspect is tamper resistance of builtin JS, which the new code preserves by using captured Object.keys/defineProperty, $-intrinsics and a literal-string lazy require. The commit 15c96ea since the last review addresses the own-keys point, and no further defects were found. Deferring rather than approving because the author explicitly presents the lazy-versus-eager install as a maintainer decision with measured per-realm instruction-count tradeoffs, and because the promise operators now run synchronously, which is a Node-matching but observable timing change in core node:stream.

Those commits (5ec3ccd, 06d03f0, 15c96ea) kept the promise
operators that run in the call and recorded the reads and key orders
again that this changed. This tree takes the other way: the wrappers of
main and the first reads of main stay, so those records stay as they are
on main.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


  • Fix comments: Fix CodeRabbit comments on this PR
Prompt to fix all issues
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. 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:
Review comments at @src/js/internal/streams/readable.ts:
- Around line 1868-1870: Update installOperator’s promise-returning branch to
load and invoke the operator synchronously during the call, then return a
promise for its result. Convert synchronous loading or invocation failures into
rejected promises, preserving the asynchronous promise-based API.

After applying the fix, consider running `coderabbit review --agent` for local
review. Visit https://docs.coderabbit.ai/cli?utm_source=ghpr
Review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between a7dfd22 and a61312a.

Files selected for processing: 10

  • src/js/internal/stream.ts
  • src/js/internal/streams/readable.ts
  • test/aot/a-built-in-module-that-threw-while-it-loaded-runs-again.test.ts
  • test/aot/parallel/library-modules/stream-operators-come-with-the-readable-class.js
  • test/aot/the-first-read-of-a-standard-stream-calls-what-a-test-has-replaced.test.ts
  • test/expected-durations.json
  • test/js/node/stream/node-stream-reads-and-messages.test.ts
  • test/js/node/stream/node-stream.test.js
  • test/js/node/stream/stream-operators-arrive-with-first-pipe.expected.txt
  • test/js/node/stream/stream-operators-come-with-the-readable-class.expected.txt
Files with no reviewable changes: 1

  • test/js/node/stream/stream-operators-arrive-with-first-pipe.expected.txt

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Comment thread src/js/internal/streams/readable.ts Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked: none of readable.ts, operators.ts or stream.ts has a twin under src/js/ahead-of-time, so there is no second text to update; the two install loops now walk __proto__: null tables, so an enumerable key added to Object.prototype no longer reaches installOperator; Stream in readable.ts is the same internal/streams/legacy object that stream.ts assigns Readable onto, so the Stream.Readable ?? Readable fallback resolves to a replaced stream.Readable as intended; and the Object.keys mock changed from () => ["nonsense"] to () => undefined in the two AOT tests still makes node:zlib throw at load (zlib.ts:62 iterates ObjectKeys(codes)), which the old mock no longer did once stream.ts stopped calling Object.keys at load.

Extended reasoning...

The diff moves the eleven Readable operator installs from internal/stream.ts into internal/streams/readable.ts with a lazy require of internal/streams/operators, drops load-time captures of replaceable globals in operators.ts, and adds a 20-case subprocess suite plus a renamed AOT fixture and expected file; it touches no auth, crypto or input-parsing surface. The latest merge commit deliberately kept main's deferred Promise.$resolve().then wrapper, which the inline findings cover and which the PR description still describes the other way, so a human should weigh that choice.

Comment thread src/js/internal/streams/readable.ts Resolved
Comment thread src/js/internal/streams/readable.ts Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the latest push and found no bugs; it also addresses the two inline points I had open (the wrapper now dispatches through $then, and the description now states the promise operators stay deferred). Because this changes which module owns the Readable operators and makes internal/streams/operators load on first use, with the lazy-versus-eager startup trade-off left to a maintainer, a human look is still worthwhile.

What was reviewed:

  • The installOperator helper in src/js/internal/streams/readable.ts: name/length table matches Node's eleven signatures, the null-prototype tables keep inherited keys out of for...in, and $ERR_ILLEGAL_CONSTRUCTOR, attributes and install order match the deleted loops in internal/stream.ts.
  • The hand-rolled floor in operators.ts against Math.floor for NaN, ±Infinity, -0, negatives, strings and large integers — same results; $arrayPush and Promise.$resolve/$reject are intrinsics, so no load-time global capture remains for those.
  • The renamed AOT program and its expected file, plus the Object.keys fixture change in the two AOT tests — the new fixture still exercises a load-time throw, and the assertions are unchanged.
  • The new subprocess tests drain stdout/stderr/exited together and assert the combined object; the "replaced global" test would catch a load-time capture of Math.floor or Number.isNaN.
Extended reasoning...

The change moves the eleven Readable iterator-helper installs from src/js/internal/stream.ts into src/js/internal/streams/readable.ts, lazily requiring internal/streams/operators on first call, and strips load-time reads of replaceable globals from operators.ts; tests rename one AOT program and expected file, adjust two AOT fixtures, and add a ~320-line describe block of bun -e subprocess cases. It touches no auth, crypto or injection surface, only node:stream compat and built-in module load order (including the Object member read order that BunProcess.cpp replays). I could not run bun bd or the test suite in this session, so the author's claims about the AOT ordering test and the 249 vendored stream tests rest on CI. The deciding facts for defer rather than approve are the explicit lazy-versus-eager choice the description hands to a maintainer and the change in which realms carry the operators, both design calls beyond a mechanical review.

@robobun

robobun commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

#48026 carries this change as its first commit, because a process.stdout that no longer loads node:stream needs the operators to come with the class. Whichever lands first, the other rebases.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 test/aot/parallel/library-modules/stream-operators-come-with-the-readable-class.js — nit: maintainers lose the only program that recorded which of twelve ways loads all of node:stream; every row of the renamed program now prints the same text. The base rows differed (35 vs 46 own names) because the operators arrived with node:stream, which pipe() with end !== false loads at src/js/internal/streams/readable.ts:950 and end: false does not. Now the operators come with the class, so test/aot/parallel/library-modules/stream-operators-come-with-the-readable-class.js:64 prints identical values for all twelve ways and no longer witnesses that load. Fix: print a member that only node:stream adds, for example typeof net.Socket.finished or typeof Object.getPrototypeOf(net.Socket).Readable, so each way still records whether node:stream loaded.

    Why this was flagged

    The base program test/aot/parallel/library-modules/stream-operators-arrive-with-first-pipe.js and its expected file recorded for each of twelve ways whether node:stream was loaded: rows read 35 readableAborted,... where it was not and 46 drop,filter,... where it was. That signal came from the operators being installed by src/js/internal/stream.ts. After this change src/js/internal/streams/readable.ts:2047-2048 installs the eleven operators when the class loads, so test/aot/parallel/library-modules/stream-operators-come-with-the-readable-class.js:64 prints function function function 46 drop,... for every way. The load at readable.ts:950, which the rows pipe versus pipe, end: false previously distinguished, is no longer observed by any program under test/aot/parallel/library-modules. A regression that drops or widens that require would pass this program unchanged, whereas on the base the row would change.

    Verification: Any future regression in the pipe() load-order branch (src/js/internal/streams/readable.ts:946-951) would now go unnoticed, because the only program that recorded it no longer can. After this change readable.ts:2047-2048 installs the eleven when the class loads, so the new test/js/node/stream/stream-operators-come-with-the-readable-class.expected.txt has twelve byte-identical rows.

@robobun

robobun commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator Author

Merged main (2596ac5) into this branch. Two conflicts.

  • src/js/internal/streams/readable.ts: main added the load_all line at the end of the module, where this PR adds the operators. Both are kept.
  • test/expected-durations.json: main removed every aot/parallel/ entry. The entry that this PR renamed is gone with them.
  • Verified on a debug build of the merge: test/js/node/stream/node-stream.test.js passes. The two test/aot files that this PR changes pass, each in a process of its own. The program stream-operators-come-with-the-readable-class.js passes as compiled and under --aot-erase-types.
  • node-stream-reads-and-messages.test.ts: the row stream-reads-seen-without-proxy-receiver does not pass on a debug build. 144 rows differ only by the constructor read that $debug makes in emit. 46 more rows are cut short and cannot be compared. No other row differs.

…stream

Every row had the eleven, so the twelve ways printed one text. Each row
now ends with typeof net.Socket.finished, which node:stream sets when it
loads: five ways leave it undefined, seven make it a function.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator Author

CI state: the diff is green. Build 138742 passed 184 of 188 jobs, and the four macOS x64 shards expired waiting for an agent. The retriggered build 138839 cancelled 41 Linux aarch64 jobs with "Failed to create agent" before any test ran. The test failures on earlier builds were main's AOT tests (reported separately) and flakes that passed on retry. No further retriggers from me.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant