Take the native blob path for Response-wrapped Bun.file() streams - #31674
Open
alii wants to merge 22 commits into
Open
Take the native blob path for Response-wrapped Bun.file() streams#31674alii wants to merge 22 commits into
alii wants to merge 22 commits into
Claude / Claude Code Review
completed
Jun 2, 2026 in 28m 27s
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/webcore/Body.rs:1814-1823 |
Body consumers no longer mark file-backed stream as disturbed |
| 🟡 Nit | test/js/web/fetch/body.test.ts:748-754 |
Dynamic require() in new tests violates test/CLAUDE.md |
Annotations
Check failure on line 1823 in src/runtime/webcore/Body.rs
claude / Claude Code Review
Body consumers no longer mark file-backed stream as disturbed
The new fast path consumes the file-backed stream without ever marking the JS `ReadableStream` as disturbed — `to_any_blob()`'s `done()` only cancels the native source, and `detach_readable_stream()` only clears the Response's cached slot. A captured stream reference can therefore be wrapped and consumed again (`const s = Bun.file(p).stream(); await new Response(s).text(); await new Response(s).text();` re-reads the file instead of throwing), which is a regression vs. the pre-PR JS-reader path.
Check warning on line 754 in test/js/web/fetch/body.test.ts
claude / Claude Code Review
Dynamic require() in new tests violates test/CLAUDE.md
nit: per `test/CLAUDE.md` ("Avoid dynamic import & require"), these new tests should use module-scope imports rather than `require("fs")` / `require("harness")` inside test bodies. The file already has `import { bunEnv, bunExe, exampleSite } from "harness"` at the top — add `tempDirWithFiles` there, and add `import { writeFileSync } from "fs"` at module scope, then drop the seven `require(...)` calls in this describe block.
Loading