Rework LinearFifo uninit accessors, replace stream raw-pointer/static-mut forges, thread JS errors through body bufferer - #31835
Open
Jarred-Sumner wants to merge 7 commits into
Conversation
Collaborator
Contributor
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
Author
Contributor
There was a problem hiding this comment.
Thanks for addressing both nits. I didn't find any further issues, but the core changes here (MaybeUninit accessor rework + AnyBitPattern marker in linear_fifo, RawSliceMut/NonNull replacing the static-mut forges in FileReader/ByteStream, BufferError threading) are unsafe-Rust soundness work touching the event-loop/streams hot paths, so this warrants a human pass.
Extended reasoning...
Overview
The PR makes three substantive changes, all in the unsafe-Rust soundness space: (1) src/collections/linear_fifo.rs reworks the buffer trait to expose &[MaybeUninit<T>] instead of casting the whole partially-uninitialized backing store to &[T], gating the writable_slice/pump family behind a new AnyBitPattern unsafe marker trait; (2) src/runtime/webcore/{streams,FileReader,ByteStream,FileSink}.rs introduce RawSliceMut<T> to replace &'static mut [u8] lifetime forges and move StreamResult::Pending/Writable::Pending from *mut to NonNull; (3) Body.rs/html_rewriter.rs add a two-variant BufferError so JS exceptions propagate instead of being flattened to a synthetic string. The remaining ~180 touched files are autofix.ci formatting (trailing-newline removal, import reordering, derive merging) with no semantic content.
Security risks
No direct auth/crypto/permission surface. The risk profile is memory-safety: the LinearFifo accessors back the event-loop task queue, test-runner result queue, and valkey promise pairs; the RawSliceMut wrapper holds raw pointers into GC-managed JS typed-array buffers across async boundaries. A mistake in the assume-init windows, the AnyBitPattern bound, or the GC-rooting argument for slice_mut() re-borrows would be a soundness hole rather than a logic bug.
Level of scrutiny
High. This is explicitly latent-UB remediation in core collections and the streams layer, with a new unsafe trait, hand-written SAFETY arguments at every re-borrow site, and design tradeoffs the description itself flags as deferred (RawSliceMut placement, option-(a) vs option-(b) for writable_slice). The changes are behavior-neutral by design, which means correctness rests entirely on the unsafe reasoning rather than on observable test deltas.
Other factors
Both of my earlier inline nits (Zig comment regression, mislabeled html-rewriter test) were addressed in 4e80f24 and resolved. The bug-hunting pass found nothing. New Rust unit tests target the NonNull-bearing element case under Miri, and new JS GC-stress tests cover the stream paths — good coverage, but the JS tests are acknowledged as regression guards rather than pre-fix-failing. CI shows linker warnings/build failures on several targets in the latest robobun update, which should be green before merge. Given the unsafe scope and the open CI state, deferring to a human reviewer.
…-mut forges, thread JS errors through body bufferer
Three latent-UB / error-fidelity fixes in the streams and collections layers. (1) LinearFifo's buffer trait now exposes `&[MaybeUninit<T>]` instead of casting the whole (partially uninitialized) backing store to `&[T]`; only logically-written subranges are assumed-init, and the `writable_slice`/`writable_with_size`/`pump` family is gated behind a new `AnyBitPattern` marker (integers + raw pointers) so NonNull-bearing element types (test-runner result queue, valkey promise pairs, event-loop tasks) can no longer be materialized over uninit slots. (2) FileReader's `&'static mut [u8]` pending-view forge is replaced with a `RawSliceMut<u8>` raw fat-pointer wrapper (also adopted by ByteStream's `pending_buffer`); every re-borrow is a scoped unsafe block citing the GC rooting in force, and `StreamResult::Pending`/`Writable::Pending` payloads moved from bare `*mut` to `NonNull`. (3) `Body::ValueBufferer::run` no longer flattens thrown JS exceptions into a synthetic "JSError" string: a new two-variant `BufferError { Js, Native }` threads the pending-on-VM exception through, and the HTMLRewriter caller now rethrows the original exception instead of masking it with ERR_STREAM_CANNOT_PIPE. Tested via new Rust unit tests (including a NonNull-bearing element matrix designed to fail under Miri pre-fix), a new `bun:internal-for-testing` fifo probe scenario, and GC-stress JS tests for file/pipe/fetch streaming and HTMLRewriter transforms. NOTE (post-review): the JS tests are regression guards / GC-stress coverage, not pre-fix-failing validation — the representation changes are behavior-neutral and the BufferError::Js branch is unreachable from plain JS. The discriminating coverage is (a) the new NonNull-bearing Rust unit tests in linear_fifo.rs, designed to be Miri-UB pre-rework, and (b) the fifo probe scenario 2 in test/internal/linear-fifo.test.ts (old binaries return [] for unknown scenarios).
… html-rewriter test The SliceBuffer doc comment re-added a Zig-fill reference that an earlier commit deliberately stripped from this file. The 'stream already locked' regression test actually exercised the JS-defined-stream ERR_STREAM_CANNOT_PIPE catch-all, not the locked-stream path (which maps StreamAlreadyUsed to ERR_STREAM_ALREADY_FINISHED). Rename the test and comments to describe what it pins and drop the irrelevant getReader() call.
Jarred-Sumner
force-pushed
the
claude/complex-01-rework-linearfifo-uninit-accessors-repla
branch
from
June 5, 2026 03:34
6e12269 to
aef6335
Compare
Contributor
|
Warning Review limit reached
More reviews will be available in 18 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR hardens stream pointer safety by introducing ChangesStream Pointer Safety and Error Handling
LinearFifo Memory Safety Refactoring
Regression Tests: Stream GC Safety and Integration Validation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/collections/linear_fifo.rs (1)
560-575:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
offsetagainstwritable_length(), not total capacity.
writable_uninit_slice()still accepts anyoffset <= buf_len, but the wrapped branch computeswritable - offset. Whenoffsetfalls in(self.writable_length(), self.buf_len()], that underflows andwritable_slice()panics instead of returning an empty slice.Suggested fix
fn writable_uninit_slice(&mut self, offset: usize) -> &mut [MaybeUninit<T>] { let buf_len = self.buf_len(); - if offset > buf_len { - return &mut []; - } - let head = self.head; - let count = self.count; - let writable = buf_len - count; + let head = self.head; + let count = self.count; + let writable = buf_len - count; + if offset > writable { + return &mut []; + } let buf = self.buf.as_uninit_slice_mut(); let tail = head + offset + count;As per coding guidelines,
**/*.rs: Rust code: treat all size/index/length arithmetic on external data as adversarial with bounds-checking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/collections/linear_fifo.rs` around lines 560 - 575, In writable_uninit_slice, the offset is currently only compared to buf_len allowing offsets > writable_length() which causes underflow in the wrapped branch; change the guard to check against the available writable length (use writable or a writable_length() helper) so if offset > writable you return an empty slice immediately, and keep the existing logic for the non-empty case; update references inside writable_uninit_slice (variables: offset, buf_len, writable, tail, start) accordingly to prevent underflow and ensure correct empty-slice returns.
🤖 Prompt for all review comments with AI agents
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:
In `@src/collections/linear_fifo.rs`:
- Around line 167-188: SliceBuffer currently stores &'a mut [T], which creates a
live &mut [T] while debug-only poison() calls (used by LinearFifo::discard and
LinearFifo::realign) write bytes into the backing memory, causing UB for types
that are not AnyBitPattern; change SliceBuffer to hold &'a mut [MaybeUninit<T>]
(or otherwise ensure no &mut [T] exists while poisoning) and update its methods
(SliceBuffer, as_uninit_slice, as_uninit_slice_mut) to operate directly on that
MaybeUninit slice, confine any & [T] / &mut [T] casts to only the
logically-initialized window, and adjust call sites that construct SliceBuffer
accordingly so poisoning never mutates memory behind an active &mut [T].
In `@test/js/workerd/html-rewriter.test.js`:
- Line 835: Replace string repetition with Buffer.alloc as per guidelines:
change occurrences where piece.repeat(perChunk) is used (e.g., inside
controller.enqueue(encoder.encode(...)) with variables piece and perChunk) to
use Buffer.alloc(perChunk, piece).toString() instead; update both the shown call
and the similar occurrence around the later use (the occurrence noted at line
858) so the test uses Buffer.alloc(perChunk, piece).toString() in place of
".repeat()".
---
Outside diff comments:
In `@src/collections/linear_fifo.rs`:
- Around line 560-575: In writable_uninit_slice, the offset is currently only
compared to buf_len allowing offsets > writable_length() which causes underflow
in the wrapped branch; change the guard to check against the available writable
length (use writable or a writable_length() helper) so if offset > writable you
return an empty slice immediately, and keep the existing logic for the non-empty
case; update references inside writable_uninit_slice (variables: offset,
buf_len, writable, tail, start) accordingly to prevent underflow and ensure
correct empty-slice returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 201b70c9-bb58-4deb-9e0f-3ff8ef534855
📒 Files selected for processing (12)
src/collections/linear_fifo.rssrc/ptr/lib.rssrc/runtime/api/html_rewriter.rssrc/runtime/linear_fifo_testing.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ByteStream.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/streams.rstest/internal/linear-fifo.test.tstest/js/web/streams/streams.test.jstest/js/workerd/html-rewriter.test.js
Comment on lines
167
to
188
| /// `buffer_type == .Slice` — caller-provided `[]T`. | ||
| /// | ||
| /// Viewed as `MaybeUninit<T>` internally; callers must treat the backing | ||
| /// slice's contents as unspecified once handed to the fifo (debug builds | ||
| /// poison discarded slots). | ||
| pub struct SliceBuffer<'a, T>(&'a mut [T]); | ||
|
|
||
| impl<'a, T> LinearFifoBuffer<T> for SliceBuffer<'a, T> { | ||
| const POWERS_OF_TWO: bool = false; // Any size slice could be passed in | ||
| const DYNAMIC: bool = false; | ||
|
|
||
| #[inline] | ||
| fn as_slice(&self) -> &[T] { | ||
| self.0 | ||
| fn as_uninit_slice(&self) -> &[MaybeUninit<T>] { | ||
| // SAFETY: `&[T]` → `&[MaybeUninit<T>]` is layout-identical and always sound. | ||
| unsafe { &*(ptr::from_ref::<[T]>(self.0) as *const [MaybeUninit<T>]) } | ||
| } | ||
| #[inline] | ||
| fn as_mut_slice(&mut self) -> &mut [T] { | ||
| self.0 | ||
| fn as_uninit_slice_mut(&mut self) -> &mut [MaybeUninit<T>] { | ||
| // SAFETY: layout-identical view; the fifo only writes valid `T` (or | ||
| // debug poison over discarded slots — see type-level doc). | ||
| unsafe { &mut *(ptr::from_mut::<[T]>(self.0) as *mut [MaybeUninit<T>]) } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant implementation
fd -a "linear_fifo.rs" . || true
ls -la src/collections | head
rg -n "SliceBuffer|init_slice|realign|discard|0xAA|MaybeUninit|Channel::init_slice" src/collections/linear_fifo.rs
sed -n '1,220p' src/collections/linear_fifo.rs
sed -n '220,520p' src/collections/linear_fifo.rs
sed -n '520,900p' src/collections/linear_fifo.rsRepository: oven-sh/bun
Length of output: 39634
Fix UB risk: SliceBuffer holds &mut [T] while debug poisoning writes arbitrary bytes into discarded slots
src/collections/linear_fifo.rs defines SliceBuffer<'a, T>(&'a mut [T]), but LinearFifo::discard/realign (under #[cfg(debug_assertions)]) call poison() which uses ptr::write_bytes(..., 0xAA, ...) over the corresponding MaybeUninit<T> region. For T that cannot represent all byte patterns (non-AnyBitPattern), this can create invalid T behind the still-live &mut [T] stored in SliceBuffer, even if those slots are never read as T.
Store &'a mut [MaybeUninit<T>] in SliceBuffer (or otherwise ensure no &mut [T] reference exists for the buffer while poisoning/discarded slots may be overwritten), and keep the &[T] casts confined to the logically-initialized window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/collections/linear_fifo.rs` around lines 167 - 188, SliceBuffer currently
stores &'a mut [T], which creates a live &mut [T] while debug-only poison()
calls (used by LinearFifo::discard and LinearFifo::realign) write bytes into the
backing memory, causing UB for types that are not AnyBitPattern; change
SliceBuffer to hold &'a mut [MaybeUninit<T>] (or otherwise ensure no &mut [T]
exists while poisoning) and update its methods (SliceBuffer, as_uninit_slice,
as_uninit_slice_mut) to operate directly on that MaybeUninit slice, confine any
& [T] / &mut [T] casts to only the logically-initialized window, and adjust call
sites that construct SliceBuffer accordingly so poisoning never mutates memory
behind an active &mut [T].
Collaborator
Author
There was a problem hiding this comment.
The poison writes go through a reborrowed &mut [MaybeUninit<T>] derived from self.0, so there is no aliasing UB while the fifo is alive; the residual hazard (reading poisoned non-bit-pattern T after the fifo is dropped, debug builds only) is documented as a caller obligation in the SliceBuffer type-level doc, and the sole in-tree user (threading::Channel::init_slice) uses T: Copy. A follow-up could tighten the API to take &mut [MaybeUninit<T>] or require T: AnyBitPattern, but the suggested rewrite isn't needed for soundness as used.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, thanks for the precise walkthrough — you're right. The as_uninit_slice_mut reborrow means the original &mut [T] in SliceBuffer.0 is inaccessible for the duration of the poison write (Rust's borrow rules enforce that), so there's no aliasing at play. The remaining note (caller's backing slice may contain poisoned bytes after the fifo is dropped in debug builds) is a caller obligation that is correctly documented at the type level and is a non-issue for the sole in-tree user's T: Copy element type. I'll record this for future reviews.
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31835
File: src/collections/linear_fifo.rs:167-188
Timestamp: 2026-06-05T07:13:23.063Z
Learning: In `src/collections/linear_fifo.rs` (oven-sh/bun), `SliceBuffer<'a, T>` stores `&'a mut [T]`, but `as_uninit_slice_mut` derives a `&mut [MaybeUninit<T>]` from it via a pointer cast. When `poison()` is called through `readable_uninit_slice_mut` → `as_uninit_slice_mut`, Rust's borrow rules prevent simultaneous access to the original `&mut [T]`, so there is no aliasing UB during the poison write while the fifo is alive. The residual hazard — poisoned bytes in the caller's backing slice after the fifo is dropped (debug builds only) — is documented as a caller obligation in the `SliceBuffer` type-level doc. The sole in-tree user (`threading::Channel::init_slice`) uses `T: Copy` types, making this non-issue in practice. Do NOT flag this as an aliasing UB bug; the suggested change to hold `&'a mut [MaybeUninit<T>]` instead is a possible future API tightening but is not required for soundness.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to src/**/*.rs : Never let a pointer or slice outlive the memory it points into - audit slices of stack buffers, growable containers across calls, and temporary values
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/bun_core/util.rs:2150-2151
Timestamp: 2026-06-05T01:49:45.910Z
Learning: In oven-sh/bun `src/bun_core/util.rs`, do not flag `ThreadLock`'s debug-only `locked_at: Cell<StoredTrace>` access as needing full synchronization merely because it can race with `owning_thread`. This intentionally matches `src/safety/ThreadLock.zig` semantics, where the original has no synchronization on either field. `ThreadLock` is a debug-only single-threaded-access assertion; the `locked_at` trace is best-effort diagnostic data read only on the already-panicking path, and adding synchronization would add debug-build lock/unlock cost for a diagnostic that fires once before abort.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/css/properties/border.rs:1614-1621
Timestamp: 2026-06-05T01:50:02.242Z
Learning: In oven-sh/bun `src/css/properties/border.rs`, the `prop!` macro arm inside `flush_unparsed` (the non-logical path for `border-block-start*`/`border-block-end*` unparsed properties) deliberately does NOT push the rewritten `UnparsedProperty` to `dest`. This preserves a bug that also exists in the upstream Zig reference PropertyHandler, where unparsed border-block declarations are similarly dropped instead of being emitted as physical `border-top`/`border-bottom` equivalents. Fixing this behavior requires a separate PR with dedicated CSS tests; do not flag the missing `dest.push` as a bug in port-cleanup PRs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/broadcast.ts:146-147
Timestamp: 2026-06-05T02:31:21.560Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/broadcast.ts` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/streams/iter/broadcast.js`. The `#error` field is intentionally initialized to `null` (not `undefined`), and all checks such as `if (self.#error)` and `if (this.#ended || this.#error)` are truthy checks that match upstream lines 90, 182, 199, and 320 exactly. As a consequence, falsy cancellation reasons (e.g., `cancel(0)`, `cancel("")`, `cancel(false)`) behave identically in Node — they do NOT trigger rejection and instead resolve as `{ done: true }`. The vendored upstream tests assert this behavior. Do NOT suggest changing these to `!== undefined` / `!== null` checks in Bun; any fix must go to nodejs/node first. The file is kept byte-close to enable clean future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/from.ts:289-310
Timestamp: 2026-06-05T02:31:36.078Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/from.ts` is a verbatim line-for-line port of Node.js v26.3.0 `lib/internal/streams/iter/from.js`. In `normalizeAsyncSource`, the async-iterable branch (corresponding to Node upstream lines ~337-362) intentionally yields pre-batched `Uint8Array[]` arrays as-is and accumulates normalized chunks without `FROM_BATCH_SIZE` chunking — only the sync paths (Node upstream lines ~210-252) apply `FROM_BATCH_SIZE` sub-slicing. Do NOT flag the async branch's lack of `FROM_BATCH_SIZE` bounding as a bug; any fix must go to nodejs/node first. The file is kept verbatim to enable clean future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/pull.ts:870-875
Timestamp: 2026-06-05T02:31:45.270Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/pull.ts` is a faithful port of Node.js v26.3.0 `lib/internal/streams/iter/pull.js`. The `writer.writev(batch, opts).then(...)` call at the equivalent of upstream line 925 intentionally assumes `writev` returns a Promise — this is the `stream/iter` writer contract. There is no thenable guard because upstream Node makes the same assumption. Do NOT flag the absence of a thenable guard on `writer.writev()` in this file as a bug; it is upstream behavior and kept verbatim to preserve parity.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/transform.ts:586-610
Timestamp: 2026-06-05T02:31:44.523Z
Learning: In `src/js/internal/streams/iter/transform.ts` (oven-sh/bun PR `#31826`), `makeZlibTransformSync`'s inner generator intentionally finalizes ONLY on a `null` batch flush marker and has NO `finalized` guard and NO end-of-loop fallback. This is a verbatim port of Node.js v26.3.0 `lib/internal/streams/iter/transform.js` lines 636-660. The sync pipeline driver (`pullSync`) always emits a trailing `null` so the upstream contract guarantees finalization — the `finalized` guard exists only in the async path (`makeZlibTransform`) because abort signals can interrupt the loop before the driver emits the null. Do NOT flag the absence of a `finalized` guard or an end-of-loop finalize call in the sync transform as a bug.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 31817
File: test/js/bun/net/socket.test.ts:1589-1594
Timestamp: 2026-06-04T22:27:41.981Z
Learning: In `test/js/bun/net/socket.test.ts` (oven-sh/bun), subprocess-based abort-regression tests (e.g., "Bun.listen with an invalid socket handler throws ERR_INVALID_ARG_TYPE instead of aborting") intentionally do NOT assert `expect(stderr).toBe("")`. The no-abort contract is fully encoded by `expect(stdout).toBe(<expected lines>)` and `expect(exitCode).toBe(0)`: a reintroduced abort produces a non-zero exit and stdout that does not match the expected error lines. Debug and ASAN builds may write benign diagnostics to stderr, so the established pattern in this file is to destructure-and-discard stderr with `void stderr` rather than asserting it empty. Do NOT flag this omission as a missing assertion.
Learnt from: robobun
Repo: oven-sh/bun PR: 31822
File: src/jsc/bindings/BunProcess.cpp:391-394
Timestamp: 2026-06-05T03:54:21.949Z
Learning: In oven-sh/bun rename-only PRs that mechanically migrate JSC binding identifiers from `Zig::GlobalObject` to `Bun::GlobalObject`, do not flag pre-existing direct downcasts such as `static_cast<Zig::GlobalObject*>` becoming `static_cast<Bun::GlobalObject*>` as requiring `defaultGlobalObject()` or node:vm hardening. Treat those behavior-preserving casts as out of scope unless the PR changes their behavior or touches an explicit TODO requiring hardening.
Learnt from: robobun
Repo: oven-sh/bun PR: 31876
File: src/runtime/cli/repl.rs:1179-1205
Timestamp: 2026-06-05T06:57:02.307Z
Learning: In oven-sh/bun (`src/runtime/cli/repl.rs` and related Rust code), prefer `strings::is_valid_utf8(&bytes[..len])` (the repo-standard simdutf wrapper from `bun_core::strings`) over `core::str::from_utf8` for UTF-8 validation. This covers overlong encodings, surrogates, and values above U+10FFFF. Do not suggest `core::str::from_utf8` as an alternative in code review comments for this repo.
Learnt from: robobun
Repo: oven-sh/bun PR: 31822
File: src/codegen/generate-classes.ts:659-663
Timestamp: 2026-06-05T03:54:16.251Z
Learning: In oven-sh/bun PR `#31822`, `src/codegen/generate-classes.ts` is intentionally performing a mechanical 1:1 rename from `Zig::GlobalObject` to `Bun::GlobalObject` in generated C++ code. The generated constructor `call()` template already used `reinterpret_cast<Zig::GlobalObject*>(lexicalGlobalObject)` before this PR, while `construct()` already used `defaultGlobalObject()`. Do not flag the `call()` path's continued `reinterpret_cast<Bun::GlobalObject*>` as a PR `#31822` regression; changing it to `defaultGlobalObject()` is a behavioral change that belongs in a focused follow-up.
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.
Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31175
File: src/valkey/valkey_protocol.rs:407-408
Timestamp: 2026-05-22T01:23:05.156Z
Learning: In oven-sh/bun Rust crates configured with `edition = "2024"`, `size_of::<T>()` is available via the Rust 2024 prelude. When reviewing, do not flag `size_of::<T>()` calls as missing `use core::mem::size_of` or `use std::mem::size_of` imports in these crates.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31338
File: src/runtime/shell/shell_body.rs:923-926
Timestamp: 2026-05-24T10:04:48.082Z
Learning: When reviewing oven-sh/bun code, do NOT treat calls to `JSValue::get_own_truthy` as a “missing JS truthiness” check. Despite the misleading name, `get_own_truthy` only performs an own-property slot lookup (via `get_own`, no prototype walk) and filters out only `undefined` (internally: `!prop.is_undefined()`). Values like `""`, `false`, `0`, and `null` are allowed and should return `Some(...)`. So reviews should not flag uses for not excluding other falsy values; the correct semantic gate to expect is only the `undefined` guard.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31335
File: src/install/PackageManager/PackageManagerDirectories.rs:842-848
Timestamp: 2026-05-24T10:58:13.503Z
Learning: When reviewing Rust code in this repo that indexes/slices a `PathBuffer` (which derefs to a `&[u8]` in `src/bun_core/util.rs`), treat slice indexing like `buf[..n]`, `buf[n..len]`, or `buf[len] = 0` as bounds-checked by Rust. Rust’s slice/indexing machinery will panic on out-of-bounds/overflow rather than write past the buffer. Therefore, do not flag these operations as potential out-of-bounds/unsafe writes, and do not recommend adding `debug_assert!` bounds checks that merely duplicate the existing Rust slice bounds checks (the checks are already enforced at indexing).
Learnt from: robobun
Repo: oven-sh/bun PR: 31579
File: src/sys/lib.rs:9225-9231
Timestamp: 2026-05-29T19:17:51.017Z
Learning: In this repo’s Rust code, don’t treat pattern matching on a borrowed enum/struct field as an incorrect “move-out-of-&” error when the pattern only binds payload fields that are `Copy` (e.g., matching `WriteFileData::Buffer { buffer }` where `buffer: &[u8]`, or `PathOrFileDescriptor::Path(bytes)` / `Fd(fd)` where `bytes: &[u8]` and `Fd` is `Copy`). Rust allows copying `Copy` payloads out of a borrowed aggregate without requiring the aggregate enum/struct itself to be `Copy`. In code review, if the compile error is about moving the aggregate but the bound fields are `Copy`, don’t recommend making the whole enum/struct `Copy` (e.g., don’t suggest `#[derive(Copy)]` on the enum solely for this match); keeping it non-`Copy` may be intentional for future non-`Copy` variants.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/jsc/VirtualMachine.rs:2598-2599
Timestamp: 2026-05-30T00:05:21.707Z
Learning: When reviewing Rust in this repo, do not flag direct `.0` access on `bun_core::String` as an encapsulation / sealed-field violation if it is used to reach the public inner newtype field and then call a public inner method (e.g., `bun_core::String::ascii(msg).0.mark_global()`). This usage is intended to preserve the prior Zig pattern (e.g., `...init(...).mark_global()`). Prefer considering `message.mark_global()`-style alternatives as style-only; only raise an issue if there is an actual API/compilation/semantic problem (e.g., method not public or code won’t compile / behavior changes), not merely because `.0` was accessed.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_fs.rs:6118-6119
Timestamp: 2026-05-30T00:05:31.762Z
Learning: When reviewing migration-only diffs that change implementation details like string types or API renames (e.g., ZigString -> BunString), do not treat pre-existing OOM handling patterns such as `.expect("oom")` as PR-introduced issues. Confirm in the base branch (before the change) that the same `.expect("oom")` usage pattern already existed; only flag it if the PR actually introduces or modifies the OOM-handling code path, not just the surrounding string/type/API migration.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_process.rs:398-398
Timestamp: 2026-05-30T00:05:32.312Z
Learning: In this codebase (oven-sh/bun), during ZigString → BunString migration reviews, treat `BunString::ascii(bytes)` **without** a subsequent `.with_encoding(...)` as byte-level equivalent to the old `ZigString::init(bytes)` **when the original Zig code had no encoding marking**. Therefore, do not flag `BunString::ascii(...)` (no `.with_encoding`) as a new encoding regression in that migration context. If the new code uses `.with_encoding(...)`, then encoding-related regression checks may still apply.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/server/server_body.rs:2002-2006
Timestamp: 2026-05-30T00:05:28.628Z
Learning: In this repo, avoid raising an issue just because code accesses the intentionally public inner tuple field of `bun_core::String` via `.0` (e.g., `some_bun_string.0.mark_utf8()`). Since `.0` is public and `bun_alloc::String::mark_utf8` is a public method, this pattern may be used to preserve base behavior during BunString/StringView migration. Only flag cases when there’s a separate, real problem (e.g., incorrect logic, misuse of the API), not merely the presence of `.0` field access.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/webcore/Blob.rs:6941-6943
Timestamp: 2026-05-30T00:05:38.722Z
Learning: When reviewing this repo’s Rust code for the (BunString) “compile-breaking sealed-internals” concern, do not flag tuple-field access like `bun_string.0.mark_global()` / `bun_string.0.mark_utf8()` as sealed-internals—provided that the tuple field `.0` is publicly visible and the invoked `mark_*` methods are also public. In that case, treat it as a style preference at most. Only raise/retain the sealed-internals issue if, for the codebase version you’re reviewing, the tuple field `.0` is not public (or the target `mark_*` methods are not public), making the access compile-breaking.
| const stream = new ReadableStream({ | ||
| async pull(controller) { | ||
| for (let i = 0; i < chunks; i++) { | ||
| controller.enqueue(encoder.encode(piece.repeat(perChunk))); |
Contributor
There was a problem hiding this comment.
Replace .repeat() with Buffer.alloc(...).toString() in this test.
This test currently uses string repetition in hot paths; repo guidance requires Buffer.alloc(count, fill).toString() for debug-build performance.
Suggested change
- const piece = "<p>x</p>";
+ const piece = "<p>x</p>";
+ const pieceLen = Buffer.byteLength(piece);
+ const makeRepeated = count => Buffer.alloc(pieceLen * count, piece).toString();
...
- controller.enqueue(encoder.encode(piece.repeat(perChunk)));
+ controller.enqueue(encoder.encode(makeRepeated(perChunk)));
...
- expect(text).toBe(piece.repeat(perChunk * chunks));
+ expect(text).toBe(makeRepeated(perChunk * chunks));As per coding guidelines, “To create repetitive strings in tests, use Buffer.alloc(count, fill).toString() instead of "string".repeat(count) for better performance in debug builds.”
Also applies to: 858-858
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/workerd/html-rewriter.test.js` at line 835, Replace string repetition
with Buffer.alloc as per guidelines: change occurrences where
piece.repeat(perChunk) is used (e.g., inside
controller.enqueue(encoder.encode(...)) with variables piece and perChunk) to
use Buffer.alloc(perChunk, piece).toString() instead; update both the shown call
and the similar occurrence around the later use (the occurrence noted at line
858) so the test uses Buffer.alloc(perChunk, piece).toString() in place of
".repeat()".
Collaborator
Author
There was a problem hiding this comment.
Not applying: encoder.encode() consumes a string, so String.repeat is the direct fit here — the Buffer.alloc guideline targets building large binary buffers, not a test-only string chunk generator.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, understood — the guideline targets building large binary buffers, not string inputs to TextEncoder.encode(). String.repeat is the right fit here. I'll note this nuance for future reviews.
🐇 hops away quietly
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31835
File: test/js/workerd/html-rewriter.test.js:835-835
Timestamp: 2026-06-05T07:13:12.642Z
Learning: In oven-sh/bun test files, the `Buffer.alloc(count, fill).toString()` guideline (from `test/CLAUDE.md`) applies to building large repetitive *binary* buffers. It does NOT apply when `String.prototype.repeat()` is used solely to generate a plain string that is then consumed by `TextEncoder.encode()` or similar string-consuming APIs. In those cases, `String.repeat` is the idiomatic and correct choice. Do NOT flag `str.repeat(n)` as a guideline violation when the repeated string is passed directly to `TextEncoder.encode()` or other string-to-bytes APIs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/transform.ts:586-610
Timestamp: 2026-06-05T02:31:44.523Z
Learning: In `src/js/internal/streams/iter/transform.ts` (oven-sh/bun PR `#31826`), `makeZlibTransformSync`'s inner generator intentionally finalizes ONLY on a `null` batch flush marker and has NO `finalized` guard and NO end-of-loop fallback. This is a verbatim port of Node.js v26.3.0 `lib/internal/streams/iter/transform.js` lines 636-660. The sync pipeline driver (`pullSync`) always emits a trailing `null` so the upstream contract guarantees finalization — the `finalized` guard exists only in the async path (`makeZlibTransform`) because abort signals can interrupt the loop before the driver emits the null. Do NOT flag the absence of a `finalized` guard or an end-of-loop finalize call in the sync transform as a bug.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/pull.ts:870-875
Timestamp: 2026-06-05T02:31:45.270Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/pull.ts` is a faithful port of Node.js v26.3.0 `lib/internal/streams/iter/pull.js`. The `writer.writev(batch, opts).then(...)` call at the equivalent of upstream line 925 intentionally assumes `writev` returns a Promise — this is the `stream/iter` writer contract. There is no thenable guard because upstream Node makes the same assumption. Do NOT flag the absence of a thenable guard on `writer.writev()` in this file as a bug; it is upstream behavior and kept verbatim to preserve parity.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/from.ts:289-310
Timestamp: 2026-06-05T02:31:36.078Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/from.ts` is a verbatim line-for-line port of Node.js v26.3.0 `lib/internal/streams/iter/from.js`. In `normalizeAsyncSource`, the async-iterable branch (corresponding to Node upstream lines ~337-362) intentionally yields pre-batched `Uint8Array[]` arrays as-is and accumulates normalized chunks without `FROM_BATCH_SIZE` chunking — only the sync paths (Node upstream lines ~210-252) apply `FROM_BATCH_SIZE` sub-slicing. Do NOT flag the async branch's lack of `FROM_BATCH_SIZE` bounding as a bug; any fix must go to nodejs/node first. The file is kept verbatim to enable clean future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/broadcast.ts:146-147
Timestamp: 2026-06-05T02:31:21.560Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/broadcast.ts` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/streams/iter/broadcast.js`. The `#error` field is intentionally initialized to `null` (not `undefined`), and all checks such as `if (self.#error)` and `if (this.#ended || this.#error)` are truthy checks that match upstream lines 90, 182, 199, and 320 exactly. As a consequence, falsy cancellation reasons (e.g., `cancel(0)`, `cancel("")`, `cancel(false)`) behave identically in Node — they do NOT trigger rejection and instead resolve as `{ done: true }`. The vendored upstream tests assert this behavior. Do NOT suggest changing these to `!== undefined` / `!== null` checks in Bun; any fix must go to nodejs/node first. The file is kept byte-close to enable clean future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/duplex.ts:112-123
Timestamp: 2026-06-05T02:31:19.406Z
Learning: In oven-sh/bun, `src/js/internal/streams/iter/duplex.ts` is a verbatim port of Node.js v26.3.0 `lib/internal/streams/iter/duplex.js`. The abort listener is registered with `{ once: true }` and is intentionally never removed via `removeEventListener` when channels close — this matches upstream exactly. The retention window is bounded by the signal's lifetime. Do NOT suggest adding `removeEventListener` cleanup in the channel `close()` handlers; any such fix must go to nodejs/node first.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-04-21T21:51:50.964Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : To create repetitive strings in tests, use `Buffer.alloc(count, fill).toString()` instead of `"string".repeat(count)` for better performance in debug builds.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/node/repl.js:1297-1307
Timestamp: 2026-06-04T23:12:14.305Z
Learning: In oven-sh/bun, `src/js/node/repl.js` is a verbatim port of Node.js v26.3.0 `lib/repl.js`. Do not suggest fixes for bugs that originate in the upstream source (e.g., the `dw.length - up.length` NaN depth bug in `_memory()` around line 1300-1302 of the ported file). The vendored REPL tests assert upstream behavior, so patching divergences would break those tests. Any fixes must go upstream to nodejs/node first.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: test/js/node/test/parallel/test-inspector.js:315-315
Timestamp: 2026-06-04T22:08:18.155Z
Learning: In oven-sh/bun, `test/js/node/test/parallel/test-inspector.js` is a verbatim byte-identical sync from upstream Node.js (v26.3.0 `test/parallel/test-inspector.js`). Do not suggest modifications to this file—including fixing apparent bugs like the duplicate `${expectedExitCode}` placeholder on line ~315 (should be `${exitCode}`) that originates in the upstream source. Any fixes must go to nodejs/node first. The file is kept unmodified to enable clean diffable future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/completion.js:282-286
Timestamp: 2026-06-05T00:49:51.829Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/completion.js` is a verbatim port of Node.js v26.3.0 `lib/internal/repl/completion.js`. Do NOT suggest fixing:
1. `StringPrototypeIncludes(extensions, extension)` being called on an array (should conceptually be `ArrayPrototypeIncludes`) — this matches upstream lines 256 and 317 exactly.
2. `propHasGetterOrIsProxy` only using `ObjectGetOwnPropertyDescriptor` (own-property only) before accessing `obj[prop]` — this is upstream behaviour.
Any such fixes must go to nodejs/node first. The file is kept verbatim to allow clean future syncs and to keep vendored test assertions aligned with upstream behaviour.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31825
File: test/js/node/test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjs:18-20
Timestamp: 2026-06-05T01:57:55.903Z
Learning: In oven-sh/bun, `test/js/node/test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjs` is a verbatim byte-identical copy of Node.js v26.3.0 `test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjs`. The upstream `cp()` call intentionally omits `force: false` even though the filename and comment mention it — this is an upstream inconsistency. Do NOT suggest adding `force: false` to this test; any fix must go upstream to nodejs/node first. The file is kept unmodified to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 31817
File: test/js/bun/net/socket.test.ts:1589-1594
Timestamp: 2026-06-04T22:27:41.981Z
Learning: In `test/js/bun/net/socket.test.ts` (oven-sh/bun), subprocess-based abort-regression tests (e.g., "Bun.listen with an invalid socket handler throws ERR_INVALID_ARG_TYPE instead of aborting") intentionally do NOT assert `expect(stderr).toBe("")`. The no-abort contract is fully encoded by `expect(stdout).toBe(<expected lines>)` and `expect(exitCode).toBe(0)`: a reintroduced abort produces a non-zero exit and stdout that does not match the expected error lines. Debug and ASAN builds may write benign diagnostics to stderr, so the established pattern in this file is to destructure-and-discard stderr with `void stderr` rather than asserting it empty. Do NOT flag this omission as a missing assertion.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: robobun
Repo: oven-sh/bun PR: 31822
File: src/jsc/bindings/BunProcess.cpp:391-394
Timestamp: 2026-06-05T03:54:21.949Z
Learning: In oven-sh/bun rename-only PRs that mechanically migrate JSC binding identifiers from `Zig::GlobalObject` to `Bun::GlobalObject`, do not flag pre-existing direct downcasts such as `static_cast<Zig::GlobalObject*>` becoming `static_cast<Bun::GlobalObject*>` as requiring `defaultGlobalObject()` or node:vm hardening. Treat those behavior-preserving casts as out of scope unless the PR changes their behavior or touches an explicit TODO requiring hardening.
Learnt from: robobun
Repo: oven-sh/bun PR: 31876
File: src/runtime/cli/repl.rs:1179-1205
Timestamp: 2026-06-05T06:57:02.307Z
Learning: In oven-sh/bun (`src/runtime/cli/repl.rs` and related Rust code), prefer `strings::is_valid_utf8(&bytes[..len])` (the repo-standard simdutf wrapper from `bun_core::strings`) over `core::str::from_utf8` for UTF-8 validation. This covers overlong encodings, surrogates, and values above U+10FFFF. Do not suggest `core::str::from_utf8` as an alternative in code review comments for this repo.
Learnt from: robobun
Repo: oven-sh/bun PR: 31822
File: src/codegen/generate-classes.ts:659-663
Timestamp: 2026-06-05T03:54:16.251Z
Learning: In oven-sh/bun PR `#31822`, `src/codegen/generate-classes.ts` is intentionally performing a mechanical 1:1 rename from `Zig::GlobalObject` to `Bun::GlobalObject` in generated C++ code. The generated constructor `call()` template already used `reinterpret_cast<Zig::GlobalObject*>(lexicalGlobalObject)` before this PR, while `construct()` already used `defaultGlobalObject()`. Do not flag the `call()` path's continued `reinterpret_cast<Bun::GlobalObject*>` as a PR `#31822` regression; changing it to `defaultGlobalObject()` is a behavioral change that belongs in a focused follow-up.
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:25.185Z
Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.
Learnt from: robobun
Repo: oven-sh/bun PR: 28425
File: test/regression/issue/28422.test.ts:65-79
Timestamp: 2026-03-22T10:12:05.719Z
Learning: In oven-sh/bun test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}, follow CLAUDE.md by asserting the command exit code LAST—after all other assertions such as stdout/stderr checks and filesystem validation. Do not assert exitCode earlier than those checks. Also, avoid asserting stdout for commands like bun install whose output can vary between runs.
Learnt from: robobun
Repo: oven-sh/bun PR: 29359
File: test/js/node/test/parallel/test-macos-app-sandbox.js:57-61
Timestamp: 2026-04-20T21:14:19.191Z
Learning: In Node.js inline eval scripts executed via `node -e` / `node --eval` (e.g., child-process scripts that embed code passed to `--eval`), core modules like `fs`, `path`, `os`, `assert`, etc. are available as implicit globals via Node’s `evalScript`/`createGlobalRequire` behavior. When reviewing code that targets `node -e`/`--eval` inline script content, do not report “undefined variable” issues for calls such as `fs.readdirSync(...)`, `path.join(...)`, or other core-module APIs used without explicit `require(...)` within that inline script.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:663-681
Timestamp: 2026-04-22T20:47:10.896Z
Learning: In oven-sh/bun code reviews, do not recommend adding standalone regression tests that depend on setting `BUN_JSC_validateExceptionChecks=1` to exercise JSC throw-scope/exception-scope validator paths (e.g., PropertyCallback/reify interactions like `reifyAllStaticProperties`). Per `CLAUDE.md`, tests are expected to pass with `USE_SYSTEM_BUN=1`, and `BUN_JSC_validateExceptionChecks` is a no-op on release/system Bun builds. Instead, treat this class of validator coverage issue as covered by: (1) the x64-asan CI shard that enables the validator automatically, and (2) the `test/no-validate-exceptions.txt` opt-out list for tests that hit pre-existing throw-scope assertion failures unrelated to the change under review. If helpful, add an in-source comment pointing to the specific existing exerciser (e.g., the relevant `tsgo/bun-types` test) to document the intent without relying on the env var.
Learnt from: robobun
Repo: oven-sh/bun PR: 30118
File: test/js/node/zlib/zlib-writestate-detached.test.ts:78-90
Timestamp: 2026-05-04T20:37:57.348Z
Learning: In this Bun repository, do not flag code in Bun subprocess fixtures/tests where `console.log(...)` (or similar synchronous stdout/stderr writes) is immediately followed by `process.exit(n)` as a potential output-loss problem. Bun’s `process.exit()` flushes stdout and stderr synchronously before exiting (per the implementation in `src/runtime/node/process/exit.zig`), so `console.log` + `process.exit` is considered a safe, established Bun convention.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.255Z
Learning: When reviewing JavaScript/TypeScript regex literals, treat `\b` as an escaped backslash followed by `b` (i.e., it matches a literal backslash and then `b`), not the regex word-boundary metacharacter. The word-boundary metacharacter is an unescaped `\b` in the source code (i.e., `\b` in the pattern string/literal syntax), which has word-boundary semantics.
So: do not flag `\b` inside a regex as a word-boundary issue by default. Only flag `\b` when the intent is to match a literal backslash+`b` and word-boundary semantics would be incorrect. Example: `/^\\\.\\pipe\\/` (as written) matches the Windows named-pipe prefix `\\.\pipe\`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30936
File: test/bundler/transpiler/runtime-transpiler.test.ts:225-225
Timestamp: 2026-05-17T19:03:05.577Z
Learning: This repo (oven-sh/bun) does not enforce Biome lint rules in CI because there is no root Biome config (`biome.json` or `.biome*`). Therefore, during code review do not suggest adding `// biome-ignore` (or similar) suppression comments for Biome rule violations.
Additionally, in test files under `test/bundler/transpiler/`, do not “fix” switch-case code by wrapping intentionally-bare (unwrapped) `const` declarations in `{}` blocks when the test is specifically asserting TDZ/const-inlining behavior across sibling cases (e.g., regression tests like issue `#30932`). Adding a `{}` block can interfere with the const-prefix inliner and the single-use substitution pass, causing the test to miss the intended failure mode.
Jarred-Sumner
commented
Jun 5, 2026
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
Collaborator
Author
There was a problem hiding this comment.
This iseems overall bad. Why don't we just rewrite linear fifo?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three latent-UB / error-fidelity fixes in the streams and collections layers. (1) LinearFifo's buffer trait now exposes
&[MaybeUninit<T>]instead of casting the whole (partially uninitialized) backing store to&[T]; only logically-written subranges are assumed-init, and thewritable_slice/writable_with_size/pumpfamily is gated behind a newAnyBitPatternmarker (integers + raw pointers) so NonNull-bearing element types (test-runner result queue, valkey promise pairs, event-loop tasks) can no longer be materialized over uninit slots. (2) FileReader's&'static mut [u8]pending-view forge is replaced with aRawSliceMut<u8>raw fat-pointer wrapper (also adopted by ByteStream'spending_buffer); every re-borrow is a scoped unsafe block citing the GC rooting in force, andStreamResult::Pending/Writable::Pendingpayloads moved from bare*muttoNonNull. (3)Body::ValueBufferer::runno longer flattens thrown JS exceptions into a synthetic "JSError" string: a new two-variantBufferError { Js, Native }threads the pending-on-VM exception through, and the HTMLRewriter caller now rethrows the original exception instead of masking it with ERR_STREAM_CANNOT_PIPE. Tested via new Rust unit tests (including a NonNull-bearing element matrix designed to fail under Miri pre-fix), a newbun:internal-for-testingfifo probe scenario, and GC-stress JS tests for file/pipe/fetch streaming and HTMLRewriter transforms. NOTE (post-review): the JS tests are regression guards / GC-stress coverage, not pre-fix-failing validation — the representation changes are behavior-neutral and the BufferError::Js branch is unreachable from plain JS. The discriminating coverage is (a) the new NonNull-bearing Rust unit tests in linear_fifo.rs, designed to be Miri-UB pre-rework, and (b) the fifo probe scenario 2 in test/internal/linear-fifo.test.ts (old binaries return [] for unknown scenarios).Deferred
Verification
Implemented and verified on a unified integration branch: full debug build (linux-x64, ASAN), cargo check across the workspace, and the affected test files run against the debug build (failures cross-checked against main's build to exclude pre-existing issues). Each change was reviewed twice (compile/API correctness and GC/concurrency/semantics lenses) with findings repaired before landing.