Add missing exception check for ReadableStream#23932
Conversation
|
Updated 8:00 PM PT - Oct 21st, 2025
❌ @sosukesuzuki, your commit 5c178f2 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 23932That installs a local version of the PR into your bun-23932 --bun |
sosukesuzuki
force-pushed
the
fix-exception-check-for-readable-stream
branch
from
f90b828 to
3b3b467
Compare
WalkthroughModified ReadableStream exception handling in the webcore bindings by replacing an assertion check with an early-return guard. Added a test case with fixture to verify proper exception handling during ReadableStream creation from Response.body. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/bindings/webcore/ReadableStream.cpp (1)
256-265: Null-check jsDynamicCast result to avoid crash on bad arg.jsDynamicCast<JSReadableStream*>(callFrame->argument(0)) can be null; the code dereferences without validation. Add a guard that throws ThisTypeError on mismatch (pattern used elsewhere).
JSC_DEFINE_HOST_FUNCTION(jsFunctionTransferToNativeReadableStream, (JSGlobalObject * lexicalGlobalObject, CallFrame* callFrame)) { auto& vm = JSC::getVM(lexicalGlobalObject); auto throwScope = DECLARE_THROW_SCOPE(vm); - auto* readableStream = jsDynamicCast<JSReadableStream*>(callFrame->argument(0)); + auto* readableStream = jsDynamicCast<JSReadableStream*>(callFrame->argument(0)); + if (!readableStream) { + throwThisTypeError(lexicalGlobalObject, throwScope, "ReadableStream"_s); + return {}; + } readableStream->setTransferred(); readableStream->setDisturbed(true); return JSValue::encode(jsUndefined()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/bun.js/bindings/webcore/ReadableStream.cpp(1 hunks)test/js/web/streams/streams.test.js(2 hunks)untitled.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
src/bun.js/bindings/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.{cpp,h,hpp}: For JavaScript classes with a public constructor, implement three C++ classes: Foo (JSDestructibleObject if it has C++ fields), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define class properties using HashTableValue arrays
Add iso subspaces for classes that have C++ fields
Cache class structures in ZigGlobalObject
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/web/streams/streams.test.js
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/web/streams/streams.test.js
test/js/web/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Web API tests under test/js/web/, separated by category
Files:
test/js/web/streams/streams.test.js
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/web/streams/streams.test.js
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/web/streams/streams.test.js
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/web/streams/streams.test.js
🧠 Learnings (10)
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-10-20T04:22:55.575Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T04:22:55.575Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid shell commands in tests (e.g., find, grep); prefer Bun's Glob and built-in tools
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-10-20T04:22:55.575Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T04:22:55.575Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use tempDir from "harness" to create temporary directories; do not use fs.mkdtempSync or tmpdirSync
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
PR: oven-sh/bun#23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/web/streams/streams.test.js
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Use `tempDirWithFiles` (or `tempDir`) from `harness` for temporary directories/files
Applied to files:
test/js/web/streams/streams.test.js
🧬 Code graph analysis (1)
test/js/web/streams/streams.test.js (1)
test/harness.ts (2)
tmpdirSync(1211-1213)bunExe(102-105)
🔇 Additional comments (2)
test/js/web/streams/streams.test.js (1)
10-10: Import bunExe is correct for spawning Bun in tests.Matches harness guidance; no issues.
src/bun.js/bindings/webcore/ReadableStream.cpp (1)
458-470: Exception handling is correct and consistent.Verified that
fromJSHostCallinsrc/bun.js/jsc/host_fn.zigcorrectly handles the exception state returned byZigGlobalObject__createNativeReadableStream. The wrapper function checks if the return value equals.zero(which corresponds to the{}returned from C++) and converts it toerror.JSError, with validation viaassertExceptionPresenceMatches. The pattern matches how other similar C entry points handle EncodedJSValue returns.
| it("Handles exception during ReadableStream creation from Response.body", async () => { | ||
| const dir = tmpdirSync(); | ||
| const testFile = join(dir, "test-fixture.js"); | ||
| writeFileSync( | ||
| testFile, | ||
| ` | ||
| function recursiveFunction() { | ||
| const url = new URL("https://example.com/path"); | ||
| const response = new Response("test"); | ||
|
|
||
| // Access Response.body which creates a ReadableStream | ||
| const body = response.body; | ||
|
|
||
| // Set up infinite recursion via URL.pathname setter | ||
| url[Symbol.toPrimitive] = recursiveFunction; | ||
| try { | ||
| url.pathname = url; // Triggers toString() → toPrimitive → recursiveFunction() | ||
| } catch (e) { | ||
| // Stack overflow expected | ||
| if (e instanceof RangeError || e.message?.includes("stack")) { | ||
| process.exit(0); | ||
| } | ||
| throw e; | ||
| } | ||
| } | ||
| recursiveFunction(); | ||
| `, | ||
| ); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), testFile], | ||
| env: bunEnv, | ||
| cwd: dir, | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Solid e2e regression test; add stderr assertion and optional cleanup.
- Add an assertion that stderr lacks assertion text to catch regressions.
- Optionally unlink the temp file or use tempDirWithFiles for auto-cleanup.
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
+ // Ensure no native assertions leaked
+ expect(stderr).not.toMatch(/assert|EXCEPTION_ASSERT|ASSERT_FAILED/i);
expect(exitCode).toBe(0);
});Optional cleanup:
- const testFile = join(dir, "test-fixture.js");
+ const testFile = join(dir, "test-fixture.js");
writeFileSync(
testFile,
@@
);
@@
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
+ try { unlinkSync(testFile); } catch {}Based on learnings.
🤖 Prompt for AI Agents
In test/js/web/streams/streams.test.js around lines 1146 to 1185, the test
spawns a Bun process and reads stderr but doesn't assert on stderr contents or
clean up the temporary file; add an assertion after awaiting proc.stderr.text()
and proc.exited to ensure stderr does not contain assertion or error text (e.g.
expect(stderr).not.toMatch(/Assertion|Error|Unhandled/)) to catch regressions,
and add optional cleanup by unlinking the testFile or switching to a
tempDirWithFiles helper so the temp file is removed automatically.
| function f2() { | ||
| const v5 = new URL("https://example.com/path"); | ||
| const v9 = Response("bigint"); | ||
|
|
||
| // Access Response.body FIRST - creates ReadableStream | ||
| const body = v9.body; | ||
|
|
||
| // Then trigger URL.pathname recursion - causes exception scope mismatch | ||
| v5[Symbol.toPrimitive] = f2; | ||
| v5.pathname = v5; | ||
| } | ||
|
|
||
| f2(); |
There was a problem hiding this comment.
🧩 Analysis chain
Remove stray file from repo root.
This looks like a local repro snippet. It’s not under test/, unused, and should not ship.
If you want this preserved, move it under test/js/web/streams/ as a focused test; otherwise please delete it.
🏁 Script executed:
#!/bin/bash
# Verify it's unused and only present once
fd -a '^untitled\.js$' -HI
rg -n --hidden --glob '!untitled.js' 'untitled\.js'Length of output: 30
Delete stray file untitled.js from repo root.
Verification confirms this is an unused local repro snippet at the repository root and should not ship. The file contains no external references and exists nowhere else in the codebase.
🤖 Prompt for AI Agents
In untitled.js lines 1-13: this is an unused local repro snippet that must be
removed; delete the file from the repository root (remove untitled.js from the
working tree and commit the deletion) so it does not ship, and ensure there are
no references to it elsewhere (run git grep or your project search to confirm)
before pushing the commit.
5 participants
What does this PR do?
Adds missing exception check for ReadableStream.
How did you verify your code works?
Tests