Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions src/bun.js/webcore/blob/read_file.zig
Original file line number Diff line number Diff line change
Expand Up @@ -347,10 +347,7 @@ pub const ReadFile = struct {
// read up to 4k at a time if
// they didn't explicitly set a size and we're reading from something that's not a regular file
} else if (stat.size == 0 and this.could_block) {
this.size = if (this.max_length == Blob.max_size)
4096
else
this.max_length;
this.size = @min(this.max_length, 4096);
}

if (this.offset > 0) {
Comment on lines 347 to 353

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.

🟣 Pre-existing bug: in the Linux ReadFile.doReadLoop path, this.read_off is initialized to 0 and never incremented during the loop, so remainingBuffer() always caps each individual read at max_length bytes rather than max_length - already_read bytes. When data arrives in partial chunks, the accumulated buffer can exceed max_length by up to max_length-1 bytes. The PR adds a test claiming to verify the N-byte cap, but the test writes all bytes atomically so the OS delivers exactly N bytes in a single read call—the partial-chunk case that triggers the bug is never exercised.

Extended reasoning...

What the bug is: In ReadFile.doReadLoop (the Linux/non-Windows path), this.read_off is a field meant to track how many bytes have been read so far, allowing remainingBuffer() to compute a shrinking per-iteration limit of max_length - already_read. However, read_off is set to 0 in runAsyncWithFD and is never updated anywhere in doReadLoop. Every call to remainingBuffer() therefore computes max_length -| 0 = max_length, allowing up to max_length additional bytes to be read on every iteration regardless of how many bytes have already accumulated.

Specific code path: remainingBuffer (line 184): remaining[0..@min(remaining.len, this.max_length -| this.read_off)]. Since read_off never changes, this bound is constant at max_length. The only exit guard in doReadLoop is if (!this.read_eof and this.buffer.items.len >= this.max_length) break; — this fires after the over-read has already been appended.

Why existing code does not prevent it: The Windows ReadFileUV.onRead correctly does this.read_off += @intCast(result.int()); (line 796), so the Windows path is fine. The Linux path simply omits this update, making the two paths inconsistent.

Impact: With max_length = N on a non-regular file, if the first partial read delivers k bytes (k < N), the loop continues. On the next iteration remainingBuffer again offers an N-byte window (read_off still 0). If N bytes are now available, they are all read and buffer.items.len = k + N. Maximum overshoot is N-1 extra bytes. NewReadFileHandler.run updates blob.size but passes the raw over-sized slice to the Function callback, potentially exposing extra bytes to JavaScript callers.

Step-by-step proof with max_length = 3, data arrives as [1 byte, then 3 bytes]:

  1. runAsyncWithFD: buffer allocated; read_off = 0.
  2. Iter 1: remainingBufferstack_buf[0..3]; read returns 1 byte; buffer.items.len = 1; check 1 >= 3 → false; continue.
  3. Iter 2: remainingBufferstack_buf[0..3] (read_off still 0); read returns 3 bytes; buffer.items.len = 4; check 4 >= 3 → break.
  4. Caller receives a 4-byte buffer despite max_length = 3.

How to fix: Add this.read_off += @intCast(read_amount); inside doReadLoop after appending bytes to the buffer, mirroring the Windows ReadFileUV.onRead update.

Relationship to this PR: The bug is pre-existing and not introduced by this PR. However, the PR adds a test ('Bun.stdin.slice(0, N).text() caps reads at N bytes') that explicitly claims to validate the N-byte cap. Because proc.stdin.write('0123456789') delivers all 10 bytes atomically into the pipe buffer, the first read() with a 3-byte buffer returns exactly 3 bytes, immediately satisfying items.len >= max_length, so the test passes without exercising the multi-iteration partial-read scenario where the bug manifests.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed this is pre-existing and out of scope for this crash fix — read_off is never incremented in the POSIX doReadLoop, unlike ReadFileUV.onRead on Windows. Leaving that for a separate change.

The second test here is a regression guard that @min(max_length, 4096) didn't break the existing cap behavior for the common single-read case, not a claim of full cap correctness.

Expand Down Expand Up @@ -384,7 +381,7 @@ pub const ReadFile = struct {

// add an extra 16 bytes to the buffer to avoid having to resize it for trailing extra data
if (!this.could_block or (this.size > 0 and this.size != Blob.max_size))
this.buffer = std.ArrayListUnmanaged(u8).initCapacity(bun.default_allocator, this.size + 16) catch |err| {
this.buffer = std.ArrayListUnmanaged(u8).initCapacity(bun.default_allocator, this.size +| 16) catch |err| {
this.errno = err;
this.onFinish();
return;
Expand Down Expand Up @@ -669,10 +666,7 @@ pub const ReadFileUV = struct {
} else if (stat.size == 0 and !this.is_regular_file) {
// read up to 4k at a time if they didn't explicitly set a size and
// we're reading from something that's not a regular file.
this.size = if (this.max_length == Blob.max_size)
4096
else
this.max_length;
this.size = @min(this.max_length, 4096);
}

if (this.offset > 0) {
Expand All @@ -698,7 +692,7 @@ pub const ReadFileUV = struct {
return;
}
// add an extra 16 bytes to the buffer to avoid having to resize it for trailing extra data
this.buffer.ensureTotalCapacityPrecise(this.byte_store.allocator, @min(this.size + 16, @as(usize, std.math.maxInt(bun.windows.ULONG)))) catch {
this.buffer.ensureTotalCapacityPrecise(this.byte_store.allocator, @min(this.size +| 16, @as(usize, std.math.maxInt(bun.windows.ULONG)))) catch {
this.errno = error.OutOfMemory;
this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
this.onFinish();
Expand Down
42 changes: 42 additions & 0 deletions test/js/bun/util/bun-stdin-slice.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows } from "harness";

// Reading a sliced non-regular file blob (like stdin from a pipe) with a size
// close to Blob.max_size used to overflow when computing the initial read
// buffer capacity. The overflow was only reachable on POSIX; on Windows the
// ReadFileUV path already bailed on size > ULONG_MAX before the addition.
test.skipIf(isWindows)("Bun.stdin.slice(1).text() does not crash when stdin is a pipe", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", `process.stdout.write(await Bun.stdin.slice(1).text());`],
env: bunEnv,
stdin: "pipe",
stdout: "pipe",
stderr: "pipe",
});

proc.stdin.write("hello world");
await proc.stdin.end();

const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);

expect(stdout).toBe("hello world");
expect(exitCode).toBe(0);
Comment on lines +22 to +23

@coderabbitai coderabbitai Bot Apr 14, 2026

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.

⚠️ Potential issue | 🟠 Major

slice(1) assertion currently doesn't validate slicing behavior.

With slice(1) in the command, asserting the full unsliced input allows an offset-ignore bug to pass. Use a sliced expectation so this test guards both crash safety and offset handling.

Suggested fix
-  expect(stdout).toBe("hello world");
+  expect(stdout).toBe("ello world");
   expect(exitCode).toBe(0);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/js/bun/util/bun-stdin-slice.test.ts` around lines 22 - 23, The test
asserts the unsliced input while the command uses slice(1), so update the stdout
assertion to validate slicing: change the expected value for stdout from "hello
world" to the sliced string "ello world" (keep the exitCode assertion). Modify
the expectation that references stdout in the test (the lines with
expect(stdout).toBe(...)) so it verifies the slice(1) behavior rather than the
full input.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This suggestion would make the test fail. Pipes aren't seekable — lseek(pipe_fd, 1, SEEK_SET) returns ESPIPE, and ReadFile intentionally ignores that error (see the existing comment at the setFileOffset call: // we ignore errors because it should continue to work even if its a pipe). So slice(1) on a piped stdin returns the full input; that's pre-existing behavior this PR doesn't change.

The assertion documents the actual current behavior so the test is a crash regression guard, not a spec for pipe offset semantics.

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.

@robobun, you're right — I apologize for the incorrect suggestion. Since pipes aren't seekable (lseekESPIPE) and ReadFile intentionally swallows that error, slice(1) on a piped stdin correctly returns the full input. The assertion expect(stdout).toBe("hello world") accurately documents the actual behavior and serves purely as a crash regression guard. No change needed.


✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 29284
File: test/js/bun/util/bun-stdin-slice.test.ts:22-23
Timestamp: 2026-04-14T02:25:22.497Z
Learning: In `src/bun.js/webcore/blob/read_file.zig`, `ReadFile.setFileOffset` intentionally ignores `ESPIPE` errors so that `Blob.slice(offset)` continues to work on non-seekable fds (pipes, FIFOs, sockets). Consequently, `Bun.stdin.slice(N).text()` on a piped stdin returns the FULL unsliced input — the seek is silently skipped. Tests that assert on `Bun.stdin.slice(N).text()` for piped stdin should therefore expect the full input string, not the offset-trimmed string. Do NOT suggest changing such assertions to the sliced expectation.

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: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

Learnt from: robobun
Repo: oven-sh/bun PR: 28694
File: test/regression/issue/20488.test.ts:37-41
Timestamp: 2026-03-31T00:56:25.338Z
Learning: In oven-sh/bun regression tests (test/regression/issue/*.test.ts), do NOT assert `expect(stderr).toBe("")` on spawned Bun subprocesses. ASAN builds emit "WARNING: ASAN interferes with JSC signal handlers..." to stderr, causing such assertions to fail on ASAN CI targets. Use `expect(exitCode).toBe(0)` as the crash/failure signal instead; only add stderr assertions when checking for a specific expected error message, not to assert emptiness.

Learnt from: robobun
Repo: oven-sh/bun PR: 28093
File: src/bun.js/webcore/Blob.zig:1416-1429
Timestamp: 2026-03-14T09:45:21.439Z
Learning: In src/bun.js/webcore/Blob.zig, the fast-path in `writeFileInternal` that pipes a locked Response/Request ReadableStream directly via `pipeReadableStreamToBlob` (added in PR `#28093`, around the Response `.Locked` branch) does not respect destination blob offset/size for sliced file targets. This is a known limitation but not a regression: before PR `#28093`, `Bun.write(slicedFile, new Response(req.body))` deadlocked unconditionally, so the sliced-blob + locked-body combination was never reachable without a hang. A follow-up offset guard (analogous to the `blob.offset == 0` check at line 1265) should be added in a separate PR to make the slice semantics correct.

Learnt from: robobun
Repo: oven-sh/bun PR: 28748
File: src/cli.zig:998-1002
Timestamp: 2026-04-02T21:02:25.201Z
Learning: In oven-sh/bun `src/cli/run_command.zig`, adding `bootFromStdin` to the `.RunAsNodeCommand` path (i.e., when `argv0 == "node"` and `ctx.positionals.len == 0`) causes REPL test deadlocks. Tests like `test-repl-dynamic-import.js`, `test-repl-close.js`, and `as-node.test.ts` spawn bun-as-node with a piped stdin and write incrementally; `streamRemaining()` blocks until EOF, so the process hangs waiting for input that never ends. Node shim stdin bootstrapping requires an incremental (non-blocking) read approach and is intentionally deferred to a separate PR. Do NOT flag the absence of `bootFromStdin` in the `RunAsNodeCommand` / `execAsIfNode` no-positionals path as a missing feature parity issue — it was deliberately reverted (commit 79adac4 in PR `#28748`) to avoid these deadlocks.

Learnt from: robobun
Repo: oven-sh/bun PR: 28481
File: test/regression/issue/28479.test.ts:132-136
Timestamp: 2026-03-23T20:13:52.791Z
Learning: In `test/regression/issue/28479.test.ts` (oven-sh/bun PR `#28481`), the `env: { ...process.env }` spread passed to a child `Bun.spawn` in the "loaded vars are visible in child processes" test is intentional. `process.loadEnvFile` updates the JS `process.env` object but does NOT call `setenv`/`putenv` to update the OS-level environment on POSIX (only Windows gets OS-level updates via `SetEnvironmentVariableW`). Therefore, relying on default env inheritance would miss the new variable on Linux/macOS. Spreading `process.env` is the only correct cross-platform way to verify the JS-side update propagates to children. Do not flag this spread as bypassing the inherited-env path.

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/unicode/uucode/CLAUDE.md:0-0
Timestamp: 2026-01-23T08:08:06.510Z
Learning: Applies to src/unicode/uucode/test/js/bun/util/stringWidth.test.ts : Verify grapheme table changes by running `bun bd test test/js/bun/util/stringWidth.test.ts` after regenerating tables

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-22T12:23:39.726Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.

Learnt from: robobun
Repo: oven-sh/bun PR: 28628
File: test/regression/issue/28627.test.ts:28-31
Timestamp: 2026-03-29T03:13:10.496Z
Learning: In `test/regression/issue/28627.test.ts` (oven-sh/bun PR `#28628`), the `expect(stdout).toContain("installed is-number")` assertion is intentional and should NOT be removed. The regression (`#28627`) is specifically that `bun add --production <pkg>` prints "installed is-number" but does not actually install the package. The stdout assertion validates the user-visible confirmation message, while the `existsSync` check validates the filesystem side-effect. Together they guard against both halves of the regression. The general guideline "avoid asserting stdout for bun install commands" does NOT apply here because the stdout message itself is a core part of the regression being tested.

Learnt from: robobun
Repo: oven-sh/bun PR: 28112
File: src/bun.js/webcore/Blob.zig:1416-1429
Timestamp: 2026-03-14T17:19:52.471Z
Learning: In src/bun.js/webcore/Blob.zig, the missing offset/slice guard for stream-to-file write paths is a pre-existing limitation that spans ALL such paths uniformly: (1) the WriteFileWaitFromLockedValueTask fallback, (2) the locked-body direct piping fast-path added in PR `#28093/28112`, and (3) pipeReadableStreamToBlob itself. Adding an offset check at only one call site would be inconsistent. The correct fix is to add offset awareness uniformly across all stream-to-file write paths in a dedicated follow-up PR.

Learnt from: robobun
Repo: oven-sh/bun PR: 28732
File: test/regression/issue/13184.test.ts:16-20
Timestamp: 2026-04-01T16:21:59.448Z
Learning: In `test/regression/issue/13184.test.ts` (oven-sh/bun PR `#28732`), do NOT add a `socket.destroyed` assertion inside `testServerCloseCompletes`. The test's regression signal is `server.close()` completing without hanging: if the socket is not destroyed, `_connections` is never decremented and `server.close()` times out. A direct `expect(socket.destroyed).toBe(true)` assertion was tried and caused intermittent failures under ASAN builds. The `server.close()` timeout failure is the correct and sufficient signal for this regression.

Learnt from: robobun
Repo: oven-sh/bun PR: 28570
File: test/regression/issue/28035.test.ts:124-156
Timestamp: 2026-03-26T04:35:04.113Z
Learning: In `test/regression/issue/28035.test.ts`, the "TransformStream proxy delivers all data" test intentionally uses `expect(body.length).toBe(TOTAL_CHUNKS * 25000)` (total byte count only) rather than per-chunk index encoding + deep equality. The test's scope is end-to-end streaming delivery correctness, not chunk ordering or data-integrity across a proxy. Do not suggest encoding chunk indices or comparing full byte sequences for this test; the length assertion is sufficient for its regression purpose.

Learnt from: robobun
Repo: oven-sh/bun PR: 28686
File: test/regression/issue/12306.test.ts:50-55
Timestamp: 2026-04-02T18:42:38.406Z
Learning: In `test/regression/issue/12306.test.ts` (oven-sh/bun PR `#28686`), the `done` Promise only rejects via the client `error` event — a genuine test failure. In that case, bun:test catches the unhandled rejection and reports the error, and the test process exits immediately, making the skipped `server.close()` harmless. Do not flag the missing try/finally around `await done` in this test as a resource-leak risk; the process-exit path is the correct and sufficient cleanup signal for this regression test.

Learnt from: robobun
Repo: oven-sh/bun PR: 28621
File: src/cli/test_command.zig:1246-1266
Timestamp: 2026-03-28T13:16:34.757Z
Learning: In `src/cli/test_command.zig` (`printCodeCoverage`), the `if (byte_ranges.len > 0)` guard before `lcov_writer.flush()` and the temp-file move is intentionally scoped to the case where `ByteRangeMapping.map` was null/empty (no files collected at all — introduced in PR `#28621`). The scenario where `byte_ranges.len > 0` but all entries are skipped by `coveragePathIgnorePatterns` is pre-existing behavior on main, covered by the existing "coveragePathIgnorePatterns - ignore all files" test. Do not suggest replacing this check with a `reported_file_count` counter, as that would be a separate behavioral change outside the scope of the null/empty-map fix.

Learnt from: robobun
Repo: oven-sh/bun PR: 28112
File: src/bun.js/webcore/Blob.zig:1416-1429
Timestamp: 2026-03-16T02:00:53.488Z
Learning: In `src/bun.js/webcore/Blob.zig`, `pipeReadableStreamToBlob` accepts only `extra_options` (S3/AWS options), not the full `WriteFileOptions` struct. As a result, `mkdirp_if_not_exists` and `mode` are NOT forwarded on any streaming write path — this includes the S3→file path in `writeFileWithSourceDestination` (around the `destination_type == .file and source_type == .s3` branch) and the locked-body fast-path added in PR `#28112`. This is a pre-existing limitation of the `pipeReadableStreamToBlob` API, not a regression. A follow-up would be needed to thread `WriteFileOptions` through `pipeReadableStreamToBlob` uniformly across all call sites.

Learnt from: robobun
Repo: oven-sh/bun PR: 28784
File: src/shell/states/Subshell.zig:157-158
Timestamp: 2026-04-02T20:24:23.367Z
Learning: In `src/shell/states/Subshell.zig` (and the pre-existing pattern in `src/shell/Builtin.zig` around line 429), `is_pollable` for the `IOWriter` constructed after `openForWritingImpl()` is hard-coded as `if (bun.Environment.isPosix) false else true` instead of using the `pollable` output variable set by `openForWritingImpl()`. This is intentional for now to stay consistent with the Builtin.zig pattern; a follow-up PR should fix both sites to use the `openForWritingImpl()` result instead of the platform constant, so FIFOs, `/dev/fd/*`, and TTY-backed redirect targets are classified correctly.

Learnt from: robobun
Repo: oven-sh/bun PR: 28748
File: src/cli/run_command.zig:1609-1626
Timestamp: 2026-04-02T19:16:22.913Z
Learning: In `src/cli/run_command.zig` (oven-sh/bun), `std.fs.File.stdin()` is intentional and correct when `.readerStreaming()` is needed (e.g., in `bootFromStdin`). `bun.FD.stdin()` returns a different type that does not have this method, so the "use bun.* APIs instead of std.*" guideline does not apply to `std.fs.File.stdin()` in streaming-reader contexts. Do not flag this as a guidelines violation.

Learnt from: robobun
Repo: oven-sh/bun PR: 29262
File: src/cli/test/ChangedFilesFilter.zig:155-162
Timestamp: 2026-04-13T15:48:28.953Z
Learning: In `src/cli/test/ChangedFilesFilter.zig` (`bun test --changed`), deleted/unresolved local imports are intentionally invisible to the `--changed` reverse-edge BFS. Unresolved `ImportRecord` entries only carry the raw specifier (e.g. `"./foo"`) — not a resolved absolute path — so reconstructing the absolute path of a deleted file would require re-implementing the resolver's extension/index probing against a path that no longer exists on disk. Vitest's `--changed` has the same limitation. Do not flag this as a bug; it is a known, accepted design trade-off. Revisit only if practical user reports surface it.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Learnt from: robobun
Repo: oven-sh/bun PR: 27554
File: test/regression/issue/27553.test.ts:4-82
Timestamp: 2026-03-18T02:16:22.232Z
Learning: In oven-sh/bun regression tests under test/regression/issue/*.test.ts, prefer explicit per-test setup and assertions (including repeated tempDir + Bun.spawn + bunExe/bunEnv + stdout/stderr/exitCode checks) over extracting shared helpers, so each test name maps clearly to its own setup. Avoid suggesting helper extraction unless the suite grows substantially or shared fixture complexity emerges.

Learnt from: robobun
Repo: oven-sh/bun PR: 28308
File: test/js/node/net/blocklist-gc.test.ts:4-12
Timestamp: 2026-03-20T09:29:54.739Z
Learning: In oven-sh/bun regression test files (e.g. test/js/node/net/blocklist-gc.test.ts and similar), do not suggest adding inline comments that explain the bug context or issue number being tested. Per repo convention, PR descriptions cover that context and test code is kept clean without such explanatory comments.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-22T12:23:39.726Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for test files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test/js/node/test/{parallel,sequential}/*.js files without .test extension, use `bun bd <file>` instead of `bun bd test <file>`.

Learnt from: robobun
Repo: oven-sh/bun PR: 28116
File: test/regression/issue/23607.test.ts:39-44
Timestamp: 2026-03-14T20:16:40.048Z
Learning: In `test/regression/issue/23607.test.ts`, the `bun_scan_*` temp file search is safe from cross-process interference because the test sets a dedicated `BUN_INSTALL_CACHE_DIR` and uses its own `tempDir`, providing full isolation within a single test run. Do not flag this pattern as a concurrency risk.

Learnt from: robobun
Repo: oven-sh/bun PR: 29060
File: src/bun.js/web_worker.zig:500-517
Timestamp: 2026-04-09T00:41:01.359Z
Learning: In oven-sh/bun `src/bun.js/web_worker.zig` + `src/bun.js/VirtualMachine.zig` (PR `#29060`): when a Worker `error` event listener cancels propagation via `preventDefault()`, the exit-code rollback is handled entirely inside `VirtualMachine.uncaughtException` — it saves `prev_exit_code` before writing `exit_code = 1`, calls `onUnhandledRejection`, then restores `prev_exit_code` if `worker.error_event_prevented` is true. The `onUnhandledRejection` callback in `web_worker.zig` does NOT touch `exit_handler.exit_code` at all. The `.bun`-mode async-rejection path never sets `exit_code = 1`, so no restore is needed there.

Learnt from: robobun
Repo: oven-sh/bun PR: 28116
File: src/install/PackageManager/security_scanner.zig:680-683
Timestamp: 2026-03-14T20:16:45.662Z
Learning: In `src/install/PackageManager/security_scanner.zig`, `bun.sys.File.writeFile` on Windows expects a wide-char path (`[:0]const u16`), but `std.fmt.bufPrintZ` into a `bun.PathBuffer` produces `[:0]u8`. When writing the temp scan file (`bun_scan_*`) on Windows, the UTF-8 path must first be converted to a wide-char path (e.g., via `bun.strings.toWPath` or equivalent) before passing to `bun.sys.File.writeFile`. This type mismatch also implies Windows backslash escaping for the injected JS string literal must be addressed at the same layer. Both issues are being fixed together in a follow-up push to PR `#28116`.

Learnt from: robobun
Repo: oven-sh/bun PR: 29203
File: src/http/websocket_client/WebSocketUpgradeClient.zig:338-340
Timestamp: 2026-04-12T01:56:26.791Z
Learning: In `src/http/websocket_client/WebSocketUpgradeClient.zig` (PR `#29203`), both the unix-socket path (`client.hostname = bun.default_allocator.dupeZ(u8, host_slice.slice()) catch ""`) and the TCP path (`out.hostname = bun.default_allocator.dupeZ(u8, display_host_) catch ""`) intentionally use `catch ""` for the hostname dupeZ rather than `bun.handleOom()`. The two branches are deliberately kept symmetric. Any OOM-hardening of this pattern must be applied to both paths together in a dedicated cleanup PR, not to the unix-socket branch alone. Do NOT flag the unix-socket branch's `catch ""` in isolation as an OOM-handling bug in reviews of this file.

Learnt from: robobun
Repo: oven-sh/bun PR: 28904
File: src/bun.js/webcore/Blob.zig:437-440
Timestamp: 2026-04-06T08:49:57.622Z
Learning: In `src/bun.js/webcore/Blob.zig`, the `errdefer blob.deinit()` added in PR `#28904` inside `_onStructuredCloneDeserialize` is scoped to the `.bytes` label block only. Once `break :bytes` fires and the `*Blob` is assigned to the outer `const blob`, any subsequent failure in the shared `versions` block (e.g. `readFloat`, filename bytes, is_jsdom_file read) does NOT trigger that errdefer and the heap-allocated Blob leaks. This is a pre-existing gap that also applies to `.file` and `.empty` store_tag variants — none of them have a function-scope errdefer on the outer `blob` variable. The correct fix is a function-scope `errdefer blob.deinit()` placed immediately after the outer `switch` assignment, with the inner branch-local errdefer either removed or converted; this fix is tracked as a follow-up to PR `#28904` and should not be re-flagged as a new regression in that PR.

Learnt from: robobun
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2026-04-07T19:13:28.440Z
Learning: In `src/bun.js/node/Stat.zig` (oven-sh/bun PR `#28957`), the `toI64` helper converts stat fields for the BigIntStats path to match Node's `AliasedBigInt64Array` semantics exactly: signed platform types are widened via `intCast` (sign-extending, so macOS `dev_t = i32 -1` → `-1n`); unsigned platform types are `bitCast(as(u64, intCast(value)))` so u64 values above INT64_MAX wrap to negative BigInts — identical to Node's `static_cast<int64_t>(uint64_t)`. The `extern fn Bun__createJSBigIntStatsObject` therefore takes `int64_t` for dev/ino/rdev (not uint64_t). The `toF64` helper for the Stats (Number) path mirrors Node's `Float64Array` path: `floatFromInt` with no clamping, precision lost above 2^53 but never saturated to INT64_MAX.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2026-04-07T05:08:07.909Z
Learning: Applies to src/**/*.zig : Use `bun.PathBuffer` for path buffers and `bun.path_buffer_pool` for pooled buffers to avoid large stack allocations, especially on Windows

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: cirospaciari
Repo: oven-sh/bun PR: 27385
File: test/js/bun/http/tls-keepalive.test.ts:115-140
Timestamp: 2026-02-24T21:02:00.725Z
Learning: In Bun's test suites, avoid adding tests for trivial environment/fixture script validation (e.g., checking if env vars exist) within test fixtures. Focus test coverage on actual behavior being tested (e.g., TLS keepalive, memory leaks) rather than auxiliary fixture validation. If a test file is primarily for fixtures, skip or limit tests that validate simple JS behavior like if (!env) throw; prioritize meaningful end-to-end or unit behavior instead.

Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:21:42.189Z
Learning: In Bun's test runner, describe.concurrent is supported (since Bun v1.2.23). Use describe.concurrent/test.concurrent for concurrent tests. Be aware of limitations: expect.assertions() and expect.hasAssertions() are not supported; toMatchSnapshot() is not supported (toMatchInlineSnapshot() is); and beforeAll/afterAll hooks are not executed concurrently. The broader guideline to prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent remains valid and should be applied to test files such as test/cli/run/concurrency-filter.test.ts and similar test files.

Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:22:55.570Z
Learning: In test/cli/run/concurrency-filter.test.ts and similar test files, timing-sensitive tests that assert on wall-clock elapsed time to verify concurrency behavior (e.g., expect(elapsed).toBeGreaterThan(800)) must remain in a sequential describe block rather than describe.concurrent. Running such tests concurrently can cause CPU contention and skew timing assertions, leading to flaky results. The guideline to prefer describe.concurrent does NOT apply for timing-based correctness verification.

Learnt from: robobun
Repo: oven-sh/bun PR: 28214
File: test/regression/issue/18115.test.ts:1-158
Timestamp: 2026-03-18T15:19:38.407Z
Learning: In Bun test files, when a resource like tempDir is a DisposableString implementing both Symbol.dispose (sync) and Symbol.asyncDispose, prefer plain using over await using. Do not recommend converting to await using for tempDir in Bun test files. This keeps tests idiomatic and avoids unnecessary async disposal. If a resource only supports asyncDispose, use await using.

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: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.

Learnt from: robobun
Repo: oven-sh/bun PR: 28923
File: test/regression/issue/28921.test.ts:0-0
Timestamp: 2026-04-06T19:19:08.790Z
Learning: In oven-sh/bun tests, prefer `tempDir` (from the `harness` module) over `tempDirWithFiles` when using the `using` statement for automatic cleanup. `tempDirWithFiles(...)` returns a plain `string`, so `using tempDirWithFiles(...)` is effectively a no-op and will not trigger disposal/cleanup. `tempDir` returns a `DisposableString` that implements `Symbol.dispose`, so it will correctly trigger cleanup on scope exit.

Learnt from: robobun
Repo: oven-sh/bun PR: 29050
File: test/regression/issue/29042.test.ts:60-94
Timestamp: 2026-04-08T21:22:00.840Z
Learning: In this repo’s Bun environment, `Bun.RedisClient` does not implement `Symbol.dispose` or `Symbol.asyncDispose`, so you cannot rely on `using` / `await using` for automatic cleanup. When creating a `Bun.RedisClient` in tests, close it explicitly with `try/finally`, calling `client.close()` in the `finally` block.

});

test.skipIf(isWindows)("Bun.stdin.slice(0, N).text() caps reads at N bytes", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", `process.stdout.write(await Bun.stdin.slice(0, 3).text());`],
env: bunEnv,
stdin: "pipe",
stdout: "pipe",
stderr: "pipe",
});

proc.stdin.write("0123456789");
await proc.stdin.end();

const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);

expect(stdout).toBe("012");
expect(exitCode).toBe(0);
});
Loading