Fix: after pausing stdin, a subprocess should be able to read from stdin #23341
Conversation
This implements proper stdin pause/resume by: 1. Adding is_paused flag to PosixBufferedReader.Flags 2. Implementing pause() to unregister FilePoll when paused 3. Implementing unpause() to allow re-registration on next read() 4. Checking is_paused in read() to prevent new reads while paused 5. Adding setFlowing() to FileReader that calls reader pause/unpause 6. Exposing setFlowing via ReadableStream bindings 7. Calling setFlowing from ProcessObjectInternals pause/resume This should fix issue #23333 where process.stdin.pause() continued reading data instead of stopping reads until resume() is called. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- When resuming from pause, check if there's buffered data and pending callback - If so, fulfill the pending callback with buffered data - Otherwise kick off a new read if needed - Still debugging test failure where script output isn't appearing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changed TCSADRAIN to TCSANOW in Bun__ttySetMode to avoid blocking on output drain, but test still fails. The issue: Bun hangs when setRawMode is called through the Python PTY wrapper (run-with-pty.py), but works fine with real PTY (via script command). Node works fine with the same PTY wrapper. setRawMode completes successfully when there's a real TTY: - With `script` command: works fine, logging shows immediate completion - With Python PTY wrapper: hangs, no output from process The pause/unpause implementation is correct but cannot be tested until this PTY/setRawMode interaction is fixed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added logging to: - rare_data.zig stdin() to trace initialization - wtf-bindings.cpp ttySetMode to trace tcsetattr calls - tty.ts setRawMode to trace JS-side calls Findings: - Accessing process.stdin property causes hang with Python PTY - Works fine without PTY or with real PTY (script command) - Works fine with Node through same Python PTY - No logging output appears, suggesting hang before any logging - Minimal scripts work, but accessing stdin causes immediate hang The hang occurs when process.stdin property is accessed, triggering stdin store initialization. Suspect issue with how Bun initializes stdin (possibly eager read attempt) vs Node's lazy approach. Need to investigate why Bun's stdin initialization blocks with Python PTY but Node's doesn't. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
|
|
Windows crash: const { spawn } = require("node:child_process");
spawn(
"bun-debug",
//["test", "fuzzy-wuzzy"],
[
"-p",
`
await Bun.sleep(10_000); // allow time to connect with a debugger
const stdin = process.stdin;
stdin.resume();
// sleep(1) here prevents the crash
stdin.pause();
await stdin.toArray();
`,
],
{
stdio: ["ignore", "inherit", "inherit"],
env: {
...process.env,
BUN_DEBUG_QUIET_LOGS: "0",
BUN_DEBUG: "out2.log",
},
},
); |
Jarred-Sumner
force-pushed
the
jarred/repro-23333
branch
from
b9316f6 to
621cd6d
Compare
Jarred-Sumner
force-pushed
the
jarred/repro-23333
branch
from
c4c43e2 to
60995f6
Compare
Jarred-Sumner
force-pushed
the
jarred/repro-23333
branch
from
16959d0 to
47c3c89
Compare
Jarred-Sumner
force-pushed
the
jarred/repro-23333
branch
from
5f77327 to
336538e
Compare
WalkthroughIntroduces flow-control toggling for file-based streams by adding setFlowing hooks from JS to native readers, adjusts pause/unpause/backpressure logic (POSIX vs non-POSIX), adds a stateful async file I/O subsystem in Source, reworks Windows pipe/file reader-writer lifecycles, integrates stdin flowing management, and adds PTY/readline and regression tests. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Comment |
WalkthroughIntroduces JS-exposed flow control for File-based ReadableStreams, adds pause/unpause semantics to PipeReader (POSIX/Windows), refactors Windows file write/close lifecycle, adds a structured Source.File state machine, wires stdin to setFlowing, applies a POSIX guard in FileRoute, and adds/readjusts tests for PTY/stdin behavior and regressions. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (2 warnings)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.3)test/js/node/readline/run-with-pty.py�[1;31mruff failed�[0m Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/io/PipeReader.zig (2)
450-525: POSIX: break out of read loop promptly when pausedCurrently pause() prevents new reads but doesn’t interrupt in-flight loops. Add a check to return early when is_paused is set for more responsive flow control.
fn readBlockingPipe(parent: *PosixBufferedReader, resizable_buffer: *std.ArrayList(u8), fd: bun.FileDescriptor, _: isize, received_hup: bool) void { while (true) { + if (parent.flags.is_paused) return; const streaming = parent.vtable.isStreamingEnabled();
636-689: POSIX: also gate the generic read loop on pauseMirror pause responsiveness in readWithFn’s main loop.
while (true) { + if (parent.flags.is_paused) return; bun.handleOom(resizable_buffer.ensureUnusedCapacity(16 * 1024));src/io/PipeWriter.zig (1)
1011-1039: Account for partial file writes in WindowsBufferedWriteruv_fs_write may complete with fewer bytes than requested. onWriteComplete currently derives “written” from pending_payload_size, which assumes full writes. Use fs.result (bytes written) to keep accounting accurate.
fn onFsWriteComplete(fs: *uv.fs_t) callconv(.C) void { const file = Source.File.fromFS(fs); const result = fs.result; const was_canceled = result.int() == uv.UV_ECANCELED; const parent_ptr = fs.data; // ALWAYS complete first file.complete(was_canceled); // If detached, file may be closing (owned fd) or just stopped (non-owned fd) if (parent_ptr == null) { return; } const this = bun.cast(*WindowsWriter, parent_ptr); + // If we have a positive write result, record the actual number of bytes written. + const int_res = result.int(); + if (int_res > 0) { + this.pending_payload_size = @as(usize, @intCast(int_res)); + } + if (was_canceled) { // Canceled write - clear pending state this.pending_payload_size = 0; return; }Consider mirroring a similar approach for StreamingWriter if partial writes are observed in practice (different buffer bookkeeping). Please verify with the provided Windows repros. [Based on learnings]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
src/bun.js/api/server/FileRoute.zig(2 hunks)src/bun.js/api/streams.classes.ts(1 hunks)src/bun.js/webcore/FileReader.zig(4 hunks)src/bun.js/webcore/ReadableStream.zig(1 hunks)src/io/PipeReader.zig(12 hunks)src/io/PipeWriter.zig(5 hunks)src/io/source.zig(1 hunks)src/js/builtins/ProcessObjectInternals.ts(2 hunks)test/internal/ban-limits.json(1 hunks)test/js/node/readline/child-reader.mjs(1 hunks)test/js/node/readline/run-with-pty.py(1 hunks)test/js/node/readline/stdin-pause-child-reads.mjs(1 hunks)test/js/node/readline/stdin-pause-pty.test.ts(1 hunks)test/regression/issue/09041.test.ts(1 hunks)test/regression/issue/09041/09041-fixture-test.txt(0 hunks)test/regression/issue/09041/09041-fixture.ts(1 hunks)test/regression/issue/11970.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/regression/issue/09041/09041-fixture-test.txt
🧰 Additional context used
📓 Path-based instructions (19)
src/js/builtins/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core JavaScript builtins (streams, console, etc.) under src/js/builtins/
Files:
src/js/builtins/ProcessObjectInternals.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/builtins/ProcessObjectInternals.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/js/node/readline/stdin-pause-child-reads.mjstest/regression/issue/09041/09041-fixture.tstest/js/node/readline/child-reader.mjstest/internal/ban-limits.jsontest/regression/issue/09041.test.tstest/js/node/readline/run-with-pty.pytest/regression/issue/11970.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/readline/stdin-pause-pty.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/readline/stdin-pause-pty.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/js/node/readline/stdin-pause-child-reads.mjstest/js/node/readline/child-reader.mjstest/js/node/readline/run-with-pty.py
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
test/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Node.js compatibility tests under test/js/node/
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/js/node/readline/stdin-pause-child-reads.mjstest/js/node/readline/child-reader.mjstest/js/node/readline/run-with-pty.py
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/webcore/ReadableStream.zigsrc/io/source.zigsrc/bun.js/api/server/FileRoute.zigsrc/io/PipeReader.zigsrc/bun.js/webcore/FileReader.zigsrc/io/PipeWriter.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/webcore/ReadableStream.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/webcore/FileReader.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/webcore/ReadableStream.zigsrc/io/source.zigsrc/bun.js/api/server/FileRoute.zigsrc/io/PipeReader.zigsrc/bun.js/webcore/FileReader.zigsrc/io/PipeWriter.zig
test/regression/issue/+([0-9])/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place regression tests under test/regression/issue/[number] (a numeric directory per issue)
Files:
test/regression/issue/09041/09041-fixture.ts
test/**/*-fixture.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Name test fixture files that are spawned by tests with the suffix
-fixture.ts
Files:
test/regression/issue/09041/09041-fixture.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue numberPlace regression tests under test/regression/issue/ (one per bug fix)
Files:
test/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/streams.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/api/streams.classes.ts
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/FileRoute.zig
🧠 Learnings (18)
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*-fixture.ts : Name test fixture files that are spawned by tests with the suffix `-fixture.ts`
Applied to files:
test/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/client-fixture.mjs : Client subprocess logic (page load, IPC, error/reload coordination) should live in client-fixture.mjs and be driven by Client
Applied to files:
test/regression/issue/09041/09041-fixture.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/regression/issue/09041/09041-fixture.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/regression/issue/09041/09041-fixture.tstest/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
📚 Learning: 2025-09-26T01:10:04.233Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: test/internal/ban-limits.json:25-26
Timestamp: 2025-09-26T01:10:04.233Z
Learning: The test/internal/ban-limits.json file contains accurate counts that are validated by tests - if the counts were incorrect, the associated tests would fail, so the values in this file reflect the actual current state of the codebase.
Applied to files:
test/internal/ban-limits.json
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `tempDir`/`tempDirWithFiles` from `harness` for temporary files and directories in tests
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/regression/issue/09041.test.tstest/regression/issue/11970.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/regression/issue/11970.test.ts
🧬 Code graph analysis (4)
test/js/node/readline/stdin-pause-pty.test.ts (2)
test/js/node/readline/stdin-pause-child-reads.mjs (1)
__dirname(5-5)test/harness.ts (1)
bunExe(102-105)
test/js/node/readline/stdin-pause-child-reads.mjs (1)
test/js/node/readline/child-reader.mjs (1)
chunkStr(7-7)
test/regression/issue/09041.test.ts (3)
test/js/node/stream/node-stream.test.js (3)
Bun(339-343)Bun(448-454)err(455-455)test/harness.ts (2)
bunExe(102-105)bunEnv(49-64)test/js/node/util/parse_args/default-args.test.mjs (3)
stdout(53-53)exitCode(50-50)exited(44-44)
test/regression/issue/11970.test.ts (2)
scripts/runner.node.mjs (5)
test(1492-1492)dir(2469-2469)env(590-594)env(1337-1340)bunEnv(1126-1145)test/harness.ts (2)
tempDir(277-284)bunExe(102-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (17)
test/regression/issue/09041.test.ts (1)
11-13: LGTM! Correct stream handling order.Collecting text from stdout/stderr before awaiting
exitedis the correct pattern to avoid potential deadlocks when buffers fill up.test/internal/ban-limits.json (1)
12-12: Count update looks correctNumbers in ban-limits.json are test-validated; this change aligns with current state.
Based on learnings
test/regression/issue/11970.test.ts (1)
30-33: Ignore env var change suggestion—BUN_INSTALL_BIN is correct
BUN_INSTALL_BIN is read bybun.getenvZ("BUN_INSTALL_BIN")insrc/install/PackageManager/PackageManagerOptions.zigand documented indocs/runtime/bunfig.md, so the test’s use of it is valid.Likely an incorrect or invalid review comment.
src/io/PipeReader.zig (1)
991-1011: Confirm safety offile.iovafterfile.complete()
Verify thatfs.deinit()(libuv’s cleanup) does not modify or freefile.iovbefore it’s consumed inonFileRead(src/io/PipeReader.zig); if it does, capture the I/O slice before callingfile.complete(...).src/js/builtins/ProcessObjectInternals.ts (2)
114-114: Excellent integration of flow control.The addition of
source?.setFlowing?.(true)properly enables flowing mode when acquiring the stdin reader, aligning with the new File stream flow-control mechanism. The optional chaining is appropriate since setFlowing is only available on File sources.
125-125: Correct flow control on disown.The addition of
source?.setFlowing?.(false)properly disables flowing mode when releasing the stdin reader, ensuring paused state is coordinated with the new flow-control API.test/js/node/readline/stdin-pause-pty.test.ts (1)
1-76: Comprehensive PTY test with excellent practices.This test demonstrates several best practices:
- Properly uses
await usingfor resource cleanup (line 13)- Leverages
bunExe()andbunEnvfrom harness (lines 2, 15)- Appropriately skips on Windows where PTY behavior differs (line 8)
- Filters noise from output (lines 22-27) before snapshot validation
- Validates interleaved parent/child communication showing correct pause/resume semantics
The snapshot clearly demonstrates the expected flow: parent reads, pauses, child reads, parent resumes, repeating until EOF.
src/bun.js/api/streams.classes.ts (1)
75-78: Correctly exposes setFlowing for File streams.The conditional inclusion of
setFlowingonly for File sources (not Blob or Bytes) is appropriate, as this flow-control mechanism is specific to file-based streams. The mapping tosetFlowingFromJSwith length 1 follows the same pattern as the existingsetRawModeAPI.src/bun.js/webcore/ReadableStream.zig (1)
580-591: Well-implemented JS-to-Zig bridge for flow control.The
setFlowingFromJSbridge function correctly:
- Uses
@hasDeclto safely check forsetFlowingmethod availability- Validates the boolean argument in debug builds (line 584)
- Converts JSValue to bool using the idiomatic
flag == .truecomparison- Always returns undefined, which is appropriate since
setFlowingis voidThis follows the same pattern as
setRawModeFromJSand properly integrates the flow-control mechanism.src/bun.js/webcore/FileReader.zig (4)
21-21: Appropriate default for flowing state.The default
flowing: bool = trueis correct—streams should flow by default until explicitly paused, matching standard ReadableStream semantics.
491-497: Correct deferred read mechanism when not flowing.When
flowingis false and there's no pending read, the code correctly enqueues the pending result rather than initiating a new read. This allows the stream to remain paused until explicitly resumed viasetFlowing(true)or until JS explicitly pulls data.
626-644: Well-coordinated flow control implementation.The
setFlowingimplementation correctly:
- Guards against no-op changes (lines 629-631) to avoid unnecessary work
- Calls
unpause()and initiates reads when enabling flow (lines 636-640)- Calls
pause()to stop reads when disabling flow (line 642)The direct coordination with
reader.unpause()/pause()ensures the underlying I/O reader state stays synchronized with the flowing state. The conditional read kickoff (lines 637-640) is a nice optimization that resumes streaming when re-enabled.
594-601: Verify delayed close when buffered data remains
The updated logic defersonClose()until buffered items are delivered in a subsequentonPull. If a consumer never callsonPull(orcancel) afteronReaderDone,onClose()won’t fire and the stream may never close. Confirm that all usages either drain or explicitly cancel the stream—and that there’s no abandoned‐stream path where buffered data lingers indefinitely.src/io/source.zig (1)
12-130: Excellent file lifecycle management with clear state machine.This File struct implementation demonstrates excellent engineering:
Documentation & Design:
- Comprehensive inline documentation (lines 12-33) explaining the state machine, usage patterns, and cancellation semantics
- Clear state transitions with appropriate assertions throughout
- Well-defined separation between public and private methods
State Machine:
- Four states (deinitialized, operating, canceling, closing) with clear transitions
- Proper handling of concurrent cancellation and detachment scenarios
- Automatic cleanup via
close_after_operationflagSafety:
canStart()guards against reuse before cleanup (line 62-64)prepare()marks fs as in-use with assertions (line 69-71)complete()always called before processing data, ensuring proper sequencing (line 103)fromFS()uses field offset to safely recover File from fs_t pointer (line 57-59)Resource Management:
detach()correctly handles both idle and in-progress states (line 90-99)onCloseComplete()properly deallocates after close (line 124-129)This is a model implementation of an asynchronous I/O resource lifecycle.
src/io/PipeWriter.zig (3)
1054-1066: Good: enforce canStart() and prepare()/complete() pairing for file writesThe addition of canStart(), prepare(), and complete(false) on the error path aligns with the new Source.File lifecycle and prevents concurrent write hazards.
1337-1365: Good: complete-first in onFsWriteComplete; canceled path clears current_payloadThis matches the new lifecycle and avoids use-after-free. Partial write accounting for StreamingWriter may still require follow-up if observed in the wild.
1401-1409: Good: canStart() and prepare() before uv_fs_write; complete(false) on sync errorBrings WindowsStreamingWriter into parity with the lifecycle guarantees.
| if (!bun.Environment.isPosix) { | ||
| this.reader.pause(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document why pause is Windows-only.
The POSIX guard on reader.pause() suggests platform-specific flow-control semantics, but the rationale isn't documented. Consider adding a brief comment explaining why pause is only needed on non-POSIX platforms when max_size is reached.
Apply this diff to add clarifying documentation:
if (state_ != .eof and max_size.* == 0) {
// artificially end the stream aka max_size reached
log("max_size reached, ending stream", .{});
if (this.route.server) |server| {
// dont need to ref because we are already holding a ref and will be derefed in onReaderDone
+ // On Windows, explicitly pause to stop reading
+ // On POSIX, flow control is managed by the new setFlowing mechanism
if (!bun.Environment.isPosix) {
this.reader.pause();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!bun.Environment.isPosix) { | |
| this.reader.pause(); | |
| } | |
| // dont need to ref because we are already holding a ref and will be derefed in onReaderDone | |
| // On Windows, explicitly pause to stop reading | |
| // On POSIX, flow control is managed by the new setFlowing mechanism | |
| if (!bun.Environment.isPosix) { | |
| this.reader.pause(); | |
| } |
| if (!bun.Environment.isPosix) { | ||
| this.reader.pause(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document why pause is Windows-only.
Same as the max_size scenario: the platform-specific pause behavior lacks documentation. Consider adding a comment explaining the flow-control difference between POSIX and Windows.
Apply this diff to add clarifying documentation:
switch (this.resp.write(chunk)) {
.backpressure => {
// pause the reader so deref until onWritable
defer this.deref();
this.resp.onWritable(*StreamTransfer, onWritable, this);
+ // On Windows, explicitly pause to stop reading
+ // On POSIX, flow control is managed by the new setFlowing mechanism
if (!bun.Environment.isPosix) {
this.reader.pause();
}🤖 Prompt for AI Agents
In src/bun.js/api/server/FileRoute.zig around lines 435 to 437, add a brief
comment above the if (!bun.Environment.isPosix) { this.reader.pause(); } block
that documents why pause is applied only on Windows: explain the
platform-specific flow-control difference (POSIX pipes/sockets support native
backpressure/non-blocking reads so explicit pause is unnecessary, whereas
Windows' IO model can keep feeding data without the same backpressure semantics
so we must explicitly pause the reader to prevent uncontrolled buffering), and
reference the analogous max_size handling for consistency.
| if (uv.uv_fs_read(uv.Loop.get(), &file_ptr.fs, file_ptr.file, @ptrCast(&file_ptr.iov), 1, if (this.flags.use_pread) @intCast(this._offset) else -1, onFileRead).toError(.write)) |err| { | ||
| file_ptr.complete(false); | ||
| this.flags.has_inflight_read = false; | ||
| this.flags.is_paused = true; | ||
| // we should inform the error if we are unable to keep reading | ||
| this.onRead(.{ .err = err }, "", .progress); | ||
| } |
There was a problem hiding this comment.
uv_fs_read error mapping should be .read, not .write
These paths report a write error for a read syscall.
- if (uv.uv_fs_read(uv.Loop.get(), &file_ptr.fs, file_ptr.file, @ptrCast(&file_ptr.iov), 1, if (this.flags.use_pread) @intCast(this._offset) else -1, onFileRead).toError(.write)) |err| {
+ if (uv.uv_fs_read(uv.Loop.get(), &file_ptr.fs, file_ptr.file, @ptrCast(&file_ptr.iov), 1, if (this.flags.use_pread) @intCast(this._offset) else -1, onFileRead).toError(.read)) |err| {
@@
- if (uv.uv_fs_read(uv.Loop.get(), &file.fs, file.file, @ptrCast(&file.iov), 1, if (this.flags.use_pread) @intCast(this._offset) else -1, onFileRead).toError(.write)) |err| {
+ if (uv.uv_fs_read(uv.Loop.get(), &file.fs, file.file, @ptrCast(&file.iov), 1, if (this.flags.use_pread) @intCast(this._offset) else -1, onFileRead).toError(.read)) |err| {Also applies to: 1111-1115
🤖 Prompt for AI Agents
In src/io/PipeReader.zig around lines 1061-1067 (and similarly at 1111-1115),
the uv_fs_read call is converting the syscall error to .write when it should be
.read; update the toError() argument from .write to .read for these uv_fs_read
invocations so read syscalls map to the read error variant, and keep the
surrounding error handling (file_ptr.complete, flags updates, and this.onRead)
unchanged.
| var buf = this.buffer(); | ||
| if (comptime bun.Environment.allow_assert) { | ||
| if (slice.len > 0 and !bun.isSliceInBuffer(slice, buf.allocatedSlice())) { | ||
| @panic("uv_read_cb: buf is not in buffer! This is a bug in bun. Please report it."); | ||
| } | ||
| } | ||
|
|
||
| // move cursor foward | ||
| buf.items.len += amount.result; | ||
|
|
||
| _ = this._onReadChunk(slice, hasMore); | ||
|
|
||
| if (hasMore == .eof) { | ||
| close(this); | ||
| } | ||
| } |
There was a problem hiding this comment.
Windows streaming buffer grows unbounded; clear buffer after delivering chunk
items.len is advanced on each read, but never reset; subsequent reads append after existing data, causing linear memory growth. Clear retaining capacity after invoking the chunk callback.
var buf = this.buffer();
@@
- // move cursor foward
+ // move cursor forward
buf.items.len += amount.result;
- _ = this._onReadChunk(slice, hasMore);
+ _ = this._onReadChunk(slice, hasMore);
+ // Reuse buffer memory for subsequent reads in streaming mode
+ if (this.vtable.onReadChunk != null and buf.items.len > 0) {
+ buf.clearRetainingCapacity();
+ }
if (hasMore == .eof) {
close(this);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var buf = this.buffer(); | |
| if (comptime bun.Environment.allow_assert) { | |
| if (slice.len > 0 and !bun.isSliceInBuffer(slice, buf.allocatedSlice())) { | |
| @panic("uv_read_cb: buf is not in buffer! This is a bug in bun. Please report it."); | |
| } | |
| } | |
| // move cursor foward | |
| buf.items.len += amount.result; | |
| _ = this._onReadChunk(slice, hasMore); | |
| if (hasMore == .eof) { | |
| close(this); | |
| } | |
| } | |
| var buf = this.buffer(); | |
| if (comptime bun.Environment.allow_assert) { | |
| if (slice.len > 0 and !bun.isSliceInBuffer(slice, buf.allocatedSlice())) { | |
| @panic("uv_read_cb: buf is not in buffer! This is a bug in bun. Please report it."); | |
| } | |
| } | |
| // move cursor forward | |
| buf.items.len += amount.result; | |
| _ = this._onReadChunk(slice, hasMore); | |
| // Reuse buffer memory for subsequent reads in streaming mode | |
| if (this.vtable.onReadChunk != null and buf.items.len > 0) { | |
| buf.clearRetainingCapacity(); | |
| } | |
| if (hasMore == .eof) { | |
| close(this); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/io/PipeReader.zig around lines 1208 to 1223, the buffer's items.len is
advanced on each read but never reset, causing unbounded growth on Windows;
after invoking the chunk callback (_onReadChunk) you should reset buf.items.len
to 0 (not free the buffer) to retain capacity while marking it empty so
subsequent reads reuse the allocated space; ensure this reset happens after
_onReadChunk returns and before any future reads (but keep the existing eof
handling).
| // Use state machine to handle close after operation completes | ||
| if (this.owns_fd) { | ||
| // always use close_fs here because we can have a operation in progress | ||
| file.close_fs.data = file; | ||
| _ = uv.uv_fs_close(uv.Loop.get(), &file.close_fs, file.file, onFileClose); | ||
| file.detach(); | ||
| } else { | ||
| // Don't own fd, just stop operations and detach parent | ||
| file.stop(); | ||
| file.fs.data = null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Canceled writes can skip cleanup when fs.data is nulled; proactively clear writer-local state
Nulling fs.data (Line 825) causes onFsWriteComplete to early-return (Lines 1019-1022, 1346-1349), which can leave pending state (e.g., pending_payload_size/current_payload) uncleared. This risks stale state and confusing end/close sequencing.
Suggestion: clear writer-local pending state in BaseWindowsPipeWriter.close() before fs.data = null using compile-time field checks.
} else {
- // Don't own fd, just stop operations and detach parent
- file.stop();
- file.fs.data = null;
+ // Don't own fd, just stop operations and detach parent.
+ // Proactively clear writer-local pending state so early-returning callbacks don't leave it stale.
+ if (@hasField(WindowsPipeWriter, "pending_payload_size")) {
+ @field(this, "pending_payload_size") = 0;
+ }
+ if (@hasField(WindowsPipeWriter, "current_payload")) {
+ @field(this, "current_payload").reset();
+ }
+ file.stop();
+ file.fs.data = null;
}Please also double-check onClose/onError ordering expectations when close() is invoked from error/cancel paths.
Also applies to: 1011-1039, 1337-1365
🏁 Script executed:
#!/usr/bin/env bash
set -eux
rg -n 'if \(this\.owns_fd\)' -C3 src/io/PipeWriter.zig
rg -n 'onFsWriteComplete' -C5 src/io/PipeWriter.zig
rg -n 'if.*fs\.data' -C3 src/io/PipeWriter.zig
rg -n 'pending_payload_size' -C3 src/io/PipeWriter.zigLength of output: 2618
🏁 Script executed:
#!/usr/bin/env bash
set -eux
rg -n -C20 'fn onFsWriteComplete' src/io/PipeWriter.zigLength of output: 3357
Reset writer state before nulling fs.data in close
In the non-owning-FD branch of PipeWriter.close() (src/io/PipeWriter.zig lines 823–826), clear this.pending_payload_size = 0; and this.current_payload.reset(); before file.fs.data = null;. Without this, cancelled writes hit the early-return in onFsWriteComplete (parent_ptr == null) and leave stale state. Double-check onClose/onError ordering when invoked from error/cancel paths.
🤖 Prompt for AI Agents
In src/io/PipeWriter.zig around lines 819 to 826, the non-owning-FD branch of
PipeWriter.close() must clear writer state before nulling file.fs.data: set
this.pending_payload_size = 0 and call this.current_payload.reset() immediately
before assigning file.fs.data = null; this prevents cancelled writes from
encountering stale state when onFsWriteComplete sees parent_ptr == null. Also
verify onClose/onError ordering on error/cancel paths so state reset always
happens before any callbacks that may observe the cleared parent.
| console.log(` | ||
| ====== stderr ====== | ||
| ${stderrText} | ||
| ====== stdout ====== | ||
| ${stdoutText} | ||
| ====== exit code ====== | ||
| ${exitCode} | ||
| `); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unconditional debug logging.
The console.log block adds noise to test output. Consider removing it or only logging on test failure. As per coding guidelines, tests should be clean and avoid unnecessary output.
Apply this diff to remove the debug logging:
- console.log(`
-====== stderr ======
-${stderrText}
-====== stdout ======
-${stdoutText}
-====== exit code ======
-${exitCode}
-`);
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(` | |
| ====== stderr ====== | |
| ${stderrText} | |
| ====== stdout ====== | |
| ${stdoutText} | |
| ====== exit code ====== | |
| ${exitCode} | |
| `); |
🤖 Prompt for AI Agents
In test/regression/issue/09041.test.ts around lines 15 to 22, remove the
unconditional console.log block that prints stderr/stdout/exit code; instead
eliminate the debug logging entirely (or wrap it behind a conditional that only
logs when the test fails) so tests produce no extraneous output. Update the test
to assert expected values directly and only emit the captured output on failure
(e.g., within a catch/failure branch) so normal test runs remain silent.
| import { $ } from "bun"; | ||
| import { join } from "path"; | ||
| test("09041", async () => { |
There was a problem hiding this comment.
Missing import: test is not defined.
The test uses test() on line 3 but doesn't import it from bun:test. This will cause a runtime error.
Apply this diff to add the missing import:
+import { test, expect } from "bun:test";
import { $ } from "bun";
import { join } from "path";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { $ } from "bun"; | |
| import { join } from "path"; | |
| test("09041", async () => { | |
| import { test, expect } from "bun:test"; | |
| import { $ } from "bun"; | |
| import { join } from "path"; | |
| test("09041", async () => { |
🤖 Prompt for AI Agents
In test/regression/issue/09041/09041-fixture.ts around lines 1 to 3, the file
calls test(...) but never imports test; add an import for test from "bun:test"
at the top of the file (e.g., import { test } from "bun:test";) so the symbol is
defined before usage.
| i += buffer.write(Number(j).toString(10) + ",", i); | ||
| } | ||
| const buns = Array.from({ length: 5 }, (_, i) => | ||
| $`${process.argv0} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use bunExe() from harness instead of process.argv0.
As per coding guidelines, when spawning Bun in tests, use bunExe() from the harness. This ensures consistency and correct behavior across different environments.
Based on coding guidelines
Add the import at the top:
import { test, expect } from "bun:test";
import { $ } from "bun";
import { join } from "path";
+import { bunExe } from "harness";Then apply this diff:
const buns = Array.from({ length: 5 }, (_, i) =>
- $`${process.argv0} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(),
+ $`${bunExe()} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $`${process.argv0} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(), | |
| // at the top of test/regression/issue/09041/09041-fixture.ts | |
| import { test, expect } from "bun:test"; | |
| import { $ } from "bun"; | |
| import { join } from "path"; | |
| import { bunExe } from "harness"; | |
| const buns = Array.from({ length: 5 }, (_, i) => | |
| $`${bunExe()} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(), | |
| ); |
🤖 Prompt for AI Agents
In test/regression/issue/09041/09041-fixture.ts around line 13, the test
currently uses process.argv0 to invoke Bun; replace that with the harness helper
bunExe() and update the command string to call bunExe() instead of
process.argv0, and add the required import for bunExe from the test harness at
the top of the file. Ensure the new import is added alongside other imports and
that the command keeps the same arguments and redirection (< ${buffer}) but uses
bunExe() for consistency across environments.
| const run = runs[i]; | ||
| console.log("===== STDERR =====\n" + run.stderr.toString("utf-8") + "\n====="); | ||
|
|
||
| expect(condense(run.stdout.toString("utf-8"))).toEqual(condense(buffer.toString("utf-8"))); |
There was a problem hiding this comment.
Missing import: expect is not defined.
The test uses expect() but doesn't import it. This is required from bun:test.
This will be fixed by adding the import suggested in the earlier comment for line 1-3.
🤖 Prompt for AI Agents
In test/regression/issue/09041/09041-fixture.ts around line 22, the test calls
expect(...) but never imports expect from bun:test; add the missing import (or
merge it into the existing top-of-file imports referenced in the earlier comment
for lines 1-3) so expect is defined for the assertion on run.stdout vs buffer,
ensuring the test file imports expect from 'bun:test' before usage.
test/regression/issue/11970.test.ts
Outdated
| // Verify the binaries were created | ||
| const exePath = join(globalBinPath, "testbin.exe"); | ||
| const bunxPath = join(globalBinPath, "testbin.bunx"); | ||
|
|
||
| expect(existsSync(exePath)).toBe(true); | ||
| expect(existsSync(bunxPath)).toBe(true); | ||
|
|
There was a problem hiding this comment.
Make Windows wrapper assertions resilient (.exe vs .cmd, .bunx vs .bunx.cmd)
Windows may emit .cmd wrappers instead of .exe/.bunx. Make the checks accept either.
- // Verify the binaries were created
- const exePath = join(globalBinPath, "testbin.exe");
- const bunxPath = join(globalBinPath, "testbin.bunx");
-
- expect(existsSync(exePath)).toBe(true);
- expect(existsSync(bunxPath)).toBe(true);
+ // Verify the binaries were created (accept .exe or .cmd variants)
+ const exeCandidates = [join(globalBinPath, "testbin.exe"), join(globalBinPath, "testbin.cmd")];
+ const bunxCandidates = [join(globalBinPath, "testbin.bunx"), join(globalBinPath, "testbin.bunx.cmd")];
+ const exeExists = exeCandidates.some(existsSync);
+ const bunxExists = bunxCandidates.some(existsSync);
+ expect(exeExists).toBe(true);
+ expect(bunxExists).toBe(true);
@@
- // Verify the binaries were removed
- expect(existsSync(exePath)).toBe(false);
- expect(existsSync(bunxPath)).toBe(false);
+ // Verify the binaries were removed
+ expect(exeCandidates.some(existsSync)).toBe(false);
+ expect(bunxCandidates.some(existsSync)).toBe(false);Also applies to: 66-69
🤖 Prompt for AI Agents
In test/regression/issue/11970.test.ts around lines 47 to 53 (and similarly
update lines 66 to 69), the assertions assume Windows will create .exe/.bunx
files, but on Windows wrappers may be emitted with .cmd suffixes (e.g., .cmd or
.bunx.cmd); change each assertion to accept either variant by constructing both
candidate paths (e.g., exePath and exePath + ".cmd" or bunxPath and bunxPath +
".cmd") and assert that existsSync is true for at least one of them (use
expect(existsSync(pathA) || existsSync(pathB)).toBe(true)). Ensure you update
both the .exe/.cmd and .bunx/.bunx.cmd checks accordingly.
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
src/bun.js/api/server/FileRoute.zig(2 hunks)src/bun.js/api/streams.classes.ts(1 hunks)src/bun.js/webcore/FileReader.zig(4 hunks)src/bun.js/webcore/ReadableStream.zig(1 hunks)src/io/PipeReader.zig(12 hunks)src/io/PipeWriter.zig(5 hunks)src/io/source.zig(1 hunks)src/js/builtins/ProcessObjectInternals.ts(2 hunks)test/internal/ban-limits.json(1 hunks)test/js/node/readline/child-reader.mjs(1 hunks)test/js/node/readline/run-with-pty.py(1 hunks)test/js/node/readline/stdin-pause-child-reads.mjs(1 hunks)test/js/node/readline/stdin-pause-pty.test.ts(1 hunks)test/regression/issue/09041.test.ts(1 hunks)test/regression/issue/09041/09041-fixture-test.txt(0 hunks)test/regression/issue/09041/09041-fixture.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/regression/issue/09041/09041-fixture-test.txt
🧰 Additional context used
📓 Path-based instructions (19)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/api/server/FileRoute.zigsrc/bun.js/webcore/ReadableStream.zigsrc/bun.js/webcore/FileReader.zigsrc/io/source.zigsrc/io/PipeWriter.zigsrc/io/PipeReader.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/FileRoute.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server/FileRoute.zigsrc/bun.js/webcore/ReadableStream.zigsrc/bun.js/webcore/FileReader.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/api/server/FileRoute.zigsrc/bun.js/webcore/ReadableStream.zigsrc/bun.js/webcore/FileReader.zigsrc/io/source.zigsrc/io/PipeWriter.zigsrc/io/PipeReader.zig
src/js/builtins/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core JavaScript builtins (streams, console, etc.) under src/js/builtins/
Files:
src/js/builtins/ProcessObjectInternals.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/builtins/ProcessObjectInternals.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/readline/stdin-pause-child-reads.mjstest/internal/ban-limits.jsontest/js/node/readline/run-with-pty.pytest/js/node/readline/child-reader.mjstest/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.tstest/regression/issue/09041/09041-fixture.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/node/readline/stdin-pause-child-reads.mjstest/js/node/readline/run-with-pty.pytest/js/node/readline/child-reader.mjstest/js/node/readline/stdin-pause-pty.test.ts
test/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Node.js compatibility tests under test/js/node/
Files:
test/js/node/readline/stdin-pause-child-reads.mjstest/js/node/readline/run-with-pty.pytest/js/node/readline/child-reader.mjstest/js/node/readline/stdin-pause-pty.test.ts
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/streams.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/api/streams.classes.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/readline/stdin-pause-pty.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/readline/stdin-pause-pty.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.tstest/regression/issue/09041/09041-fixture.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue numberPlace regression tests under test/regression/issue/ (one per bug fix)
Files:
test/regression/issue/09041.test.tstest/regression/issue/09041/09041-fixture.ts
test/regression/issue/+([0-9])/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place regression tests under test/regression/issue/[number] (a numeric directory per issue)
Files:
test/regression/issue/09041/09041-fixture.ts
test/**/*-fixture.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Name test fixture files that are spawned by tests with the suffix
-fixture.ts
Files:
test/regression/issue/09041/09041-fixture.ts
🧠 Learnings (18)
📚 Learning: 2025-09-26T01:10:04.233Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: test/internal/ban-limits.json:25-26
Timestamp: 2025-09-26T01:10:04.233Z
Learning: The test/internal/ban-limits.json file contains accurate counts that are validated by tests - if the counts were incorrect, the associated tests would fail, so the values in this file reflect the actual current state of the codebase.
Applied to files:
test/internal/ban-limits.json
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/node/readline/stdin-pause-pty.test.tstest/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `tempDir`/`tempDirWithFiles` from `harness` for temporary files and directories in tests
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*-fixture.ts : Name test fixture files that are spawned by tests with the suffix `-fixture.ts`
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errors
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/regression/issue/09041.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/client-fixture.mjs : Client subprocess logic (page load, IPC, error/reload coordination) should live in client-fixture.mjs and be driven by Client
Applied to files:
test/regression/issue/09041/09041-fixture.ts
🧬 Code graph analysis (5)
test/js/node/readline/stdin-pause-child-reads.mjs (1)
test/js/node/readline/child-reader.mjs (1)
chunkStr(7-7)
test/js/node/readline/child-reader.mjs (1)
test/js/node/readline/stdin-pause-child-reads.mjs (1)
chunkStr(10-10)
test/js/node/readline/stdin-pause-pty.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
test/regression/issue/09041.test.ts (3)
test/js/node/stream/node-stream.test.js (3)
Bun(339-343)Bun(448-454)err(455-455)test/harness.ts (2)
bunExe(102-105)bunEnv(49-64)test/js/node/util/parse_args/default-args.test.mjs (3)
stdout(53-53)exitCode(50-50)exited(44-44)
test/regression/issue/09041/09041-fixture.ts (1)
src/node-fallbacks/buffer.js (1)
Buffer(136-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (22)
test/regression/issue/09041.test.ts (2)
5-9: LGTM!The spawn setup correctly uses
bunExe()andbunEnvfrom harness, with proper stdio handling as per coding guidelines.
24-27: LGTM!The assertions correctly validate the spawned test's exit code and output.
test/regression/issue/09041/09041-fixture.ts (1)
16-26: LGTM!The validation logic thoroughly verifies the I/O roundtrip by checking condensed output, exit code, length, and exact buffer equality.
src/io/source.zig (1)
12-130: LGTM!The File state machine implementation is well-designed with:
- Comprehensive documentation explaining the lifecycle
- Proper state transitions with defensive assertions
- Clean separation between prepare/operation/complete phases
- Correct handling of cancellation and cleanup via detach
The public API is clear and the implementation follows libuv patterns correctly.
src/io/PipeReader.zig (5)
184-200: LGTM!The pause/unpause implementation correctly:
- Unregisters the FilePoll when paused to stop receiving events
- Clears the paused flag on unpause, allowing the next read() to re-register
- Guards the read path to prevent new reads while paused
This design allows the caller to control when reads resume after unpause.
351-354: LGTM!The guard prevents initiating new reads when paused, which is the correct behavior for flow control.
991-1088: LGTM!The onFileRead callback correctly integrates with the File state machine:
- Calls
file.complete(was_canceled)first to clean up the fs_t and update state- Handles detachment (when fs.data is null) early
- Processes deferred done callbacks during cancellation
- Uses
file.canStart()assertion before initiating the next read to prevent concurrent operationsThe defer block ensures reads continue when not paused, while respecting the state machine invariants.
1098-1126: LGTM!The startReading method correctly handles the File state machine:
- If a file operation is already in progress, it simply updates the data pointer and returns
- For new operations, it calls
prepare()before issuinguv_fs_read- On error, it properly calls
complete(false)to clean upThis ensures proper integration with the file lifecycle.
1174-1190: LGTM!The close method correctly handles deferred completion:
- Checks if a file operation is in progress (operating or canceling state)
- Defers the done callback until the operation completes to ensure proper cleanup ordering
- Otherwise calls closeImpl immediately
This prevents use-after-free issues during cancellation.
src/io/PipeWriter.zig (4)
819-826: LGTM!The close logic correctly distinguishes between owned and non-owned file descriptors:
- When the writer owns the fd,
detach()is called to schedule cleanup after the operation completes- When the fd is not owned, only
stop()is called and the parent reference is clearedThis prevents closing file descriptors that the writer doesn't own.
1010-1039: LGTM!The onFsWriteComplete callback follows the correct pattern:
- Calls
file.complete(was_canceled)first to clean up the fs_t- Handles detachment by returning early when fs.data is null
- Clears pending_payload_size when canceled to reset state
- Processes the write result normally otherwise
This ensures proper integration with the File state machine.
1054-1067: LGTM!The file write path correctly uses the File state machine:
- Asserts
canStart()to ensure no concurrent operations- Calls
prepare()before issuing the write- Calls
complete(false)on error to clean upThe pattern is consistent and correct.
1399-1414: LGTM!The streaming writer file path follows the same correct pattern:
- Asserts
canStart()before initiating a write- Calls
prepare()to mark the operation as in-flight- Issues
uv_fs_writewith the proper callback- Calls
complete(false)on immediate errorsThis ensures mutual exclusion and proper cleanup.
test/internal/ban-limits.json (1)
12-12: LGTM!The count update reflects that one instance of
// autofixwas removed from the codebase. Based on learnings, these counts are validated by tests, ensuring accuracy.src/bun.js/webcore/FileReader.zig (3)
21-21: Add flowing flag defaulted to true — LGTMGood default; keeps current behavior unless explicitly paused.
490-498: Respect paused/not‑flowing state in onPullDeferring read when not flowing and returning a pending fits the pause model and avoids starting new I/O.
Please confirm this path is only used for pipe/tty-like sources where pausing is desired, and that resume reliably triggers a read via setFlowing(true).
626-645: FileReader.setFlowing implementation — LGTMPause/unpause with a kick read on resume is correct; early return avoids redundant work.
Ensure reader.pause()/unpause() are idempotent across platforms and that hasPendingRead() covers in‑flight operations to prevent double reads.
src/bun.js/api/streams.classes.ts (1)
75-78: Wire setFlowing for File variant — LGTMScoped correctly to File; arity matches native binding.
src/bun.js/api/server/FileRoute.zig (2)
401-403: Avoid pausing on POSIX — LGTMConditional pause keeps POSIX paths simpler while still ending the read via return false.
Confirm that returning false without pause on POSIX reliably stops further onReadChunk invocations in BufferedReader.
435-437: Backpressure pause gated to non‑POSIX — LGTMMatches the max‑size path change; consistent behavior.
src/js/builtins/ProcessObjectInternals.ts (2)
110-117: Enable flowing on own() — LGTMCalled after acquiring the reader; fits the lifecycle.
123-131: Disable flowing on disown() — LGTMStopping new reads before releasing the lock is appropriate.
Please verify stdin on Windows still behaves with child process stdio (no unintended setFlowing on child stdout pipes).
| // Don't handle buffered data here - it will be returned on the next onPull | ||
| // This ensures proper ordering of chunks | ||
| } | ||
|
|
||
| this.parent().onClose(); | ||
| // Only close the stream if there's no buffered data left to deliver | ||
| if (this.buffered.items.len == 0) { | ||
| this.parent().onClose(); | ||
| } | ||
| if (this.waiting_for_onReaderDone) { |
There was a problem hiding this comment.
🧩 Analysis chain
onClose may never fire if buffered > 0 at EOF
Calling onClose only when buffered is empty risks never invoking the close handler because there's no later point that calls parent().onClose() after buffered data is delivered. This can break consumer onclose handlers and leak lifecycle signals.
Apply this minimal fix to always signal close when the reader is done (ordering-sensitive consumers can still rely on the “done” result ordering):
- // Only close the stream if there's no buffered data left to deliver
- if (this.buffered.items.len == 0) {
- this.parent().onClose();
- }
+ // Signal close exactly once when the reader is done.
+ // Ordering of data vs. close is handled at the JS layer via result flags.
+ this.parent().onClose();If strict “data then close” ordering is required, alternatively queue onClose on the event loop after pending.run() resolves.
Can you confirm whether any tests depend on close firing strictly after the last chunk? If so, I can propose an event‑loop queued onClose to preserve ordering.
🏁 Script executed:
#!/bin/bash
set -e
rg -n 'waiting_for_onReaderDone' -C3 src/bun.js/webcore/FileReader.zig
echo "-----"
rg -n 'onClose' -C3 src/bun.js/webcore/FileReader.zigLength of output: 1332
#!/bin/bash
set -e
sed -n '500,650p' src/bun.js/webcore/FileReader.zig🏁 Script executed:
sed -n '550,650p' src/bun.js/webcore/FileReader.zigLength of output: 2932
🏁 Script executed:
rg -n 'fn onPull' -C5 src/bun.js/webcore/FileReader.zigLength of output: 350
Always signal close when reader is done Guarding onClose() on buffered.items.len == 0 means it never fires if there’s buffered data at EOF. Unconditionally call parent().onClose() (or queue it on the event loop for strict ordering). Can you confirm whether any tests expect close to fire strictly after the last chunk?
🤖 Prompt for AI Agents
In src/bun.js/webcore/FileReader.zig around lines 594 to 602, the current guard
prevents calling parent().onClose() when buffered.items.len > 0 so onClose won't
fire at EOF if there is buffered data; remove the length guard and
unconditionally invoke parent().onClose() when the reader is done, or if strict
ordering is required, schedule parent().onClose() on the event loop (enqueue a
task) so it always fires after any remaining buffered chunks are delivered; also
ensure existing waiting_for_onReaderDone logic is preserved/updated to avoid
double-calls.
| pub fn setFlowingFromJS(this: *ReadableStreamSourceType, _: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue { | ||
| if (@hasDecl(Context, "setFlowing")) { | ||
| const flag = call_frame.argument(0); | ||
| if (Environment.allow_assert) { | ||
| bun.assert(flag.isBoolean()); | ||
| } | ||
| this.context.setFlowing(flag == .true); | ||
| return .js_undefined; | ||
| } | ||
|
|
||
| return .js_undefined; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mirror setRawModeFromJS: mark binding, set this, and compile‑time guard
Add markBinding and cache this for parity; prefer a compile‑time error if Context lacks setFlowing (like setRawMode).
Apply:
- pub fn setFlowingFromJS(this: *ReadableStreamSourceType, _: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue {
- if (@hasDecl(Context, "setFlowing")) {
+ pub fn setFlowingFromJS(this: *ReadableStreamSourceType, global: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue {
+ jsc.markBinding(@src());
+ this.this_jsvalue = call_frame.this();
+ if (@hasDecl(Context, "setFlowing")) {
const flag = call_frame.argument(0);
if (Environment.allow_assert) {
bun.assert(flag.isBoolean());
}
this.context.setFlowing(flag == .true);
return .js_undefined;
}
-
- return .js_undefined;
+ @compileError("setFlowing is not implemented on " ++ @typeName(Context));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn setFlowingFromJS(this: *ReadableStreamSourceType, _: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue { | |
| if (@hasDecl(Context, "setFlowing")) { | |
| const flag = call_frame.argument(0); | |
| if (Environment.allow_assert) { | |
| bun.assert(flag.isBoolean()); | |
| } | |
| this.context.setFlowing(flag == .true); | |
| return .js_undefined; | |
| } | |
| return .js_undefined; | |
| } | |
| pub fn setFlowingFromJS(this: *ReadableStreamSourceType, global: *jsc.JSGlobalObject, call_frame: *jsc.CallFrame) bun.JSError!JSValue { | |
| jsc.markBinding(@src()); | |
| this.this_jsvalue = call_frame.this(); | |
| if (@hasDecl(Context, "setFlowing")) { | |
| const flag = call_frame.argument(0); | |
| if (Environment.allow_assert) { | |
| bun.assert(flag.isBoolean()); | |
| } | |
| this.context.setFlowing(flag == .true); | |
| return .js_undefined; | |
| } | |
| @compileError("setFlowing is not implemented on " ++ @typeName(Context)); | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/webcore/ReadableStream.zig around lines 580-591, mirror the
pattern used in setRawModeFromJS: add a call to mark the JS binding, cache the
this/context locally, and use a compile-time guard for the presence of
Context.setFlowing. Specifically, invoke the appropriate markBinding on the call
frame before accessing arguments, store this.context into a local variable to
use for setFlowing, and replace the runtime @hasDecl check with a compile-time
check that emits @compileError if Context lacks setFlowing so the code fails to
compile rather than silently skipping behavior.
| @@ -0,0 +1,15 @@ | |||
| // Child that reads from stdin | |||
| process.stdin.setRawMode(true); | |||
| let count = 0; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the unused variable.
The variable count is declared but never used in this script.
Apply this diff to remove the unused variable:
-let count = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let count = 0; |
🤖 Prompt for AI Agents
In test/js/node/readline/child-reader.mjs around line 3, the variable
declaration "let count = 0;" is unused; remove that line from the file so the
unused variable is no longer declared.
| def waitForReady(): | ||
| # Wait for "%ready%" in stdout | ||
| buffer = b"" | ||
| print("PYTHON: waiting for ready", flush=True) | ||
| while b"%ready%" not in buffer: | ||
| ready = select.select([master_fd], [], [], 0.1)[0] | ||
| if ready: | ||
| data = os.read(master_fd, 1024) | ||
| if data: | ||
| buffer += data | ||
| sys.stdout.buffer.write(data) | ||
| sys.stdout.buffer.flush() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace global 3s alarm with per‑wait, configurable timeout; cancel alarm before waitpid
Current SIGALRM can fire mid‑exchange and during waitpid. Make timeouts per wait and configurable; cancel alarm once ready.
Apply:
@@
-import signal
+import signal
+
+TIMEOUT_SEC = int(os.environ.get("PTY_TEST_TIMEOUT", "15"))
@@
-def waitForReady():
+def waitForReady():
@@
- while b"%ready%" not in buffer:
+ start = time.time()
+ while b"%ready%" not in buffer:
+ if time.time() - start > TIMEOUT_SEC:
+ timeout_handler(None, None)
ready = select.select([master_fd], [], [], 0.1)[0]
if ready:
data = os.read(master_fd, 1024)
if data:
buffer += data
sys.stdout.buffer.write(data)
sys.stdout.buffer.flush()
@@
- signal.signal(signal.SIGALRM, timeout_handler)
- signal.alarm(3) # 3 second timeout
+ signal.signal(signal.SIGALRM, timeout_handler)
+ signal.alarm(0) # disable global alarm; per-wait timeouts handle hangs
@@
- print("PYTHON: waiting for exit", flush=True)
+ print("PYTHON: waiting for exit", flush=True)
+ signal.alarm(0) # ensure no alarm during waitpid
_, status = os.waitpid(pid, 0)Optionally, honor PTY_TEST_TIMEOUT in CI to tune LONG runs.
Also applies to: 52-54, 95-99
🤖 Prompt for AI Agents
In test/js/node/readline/run-with-pty.py around lines 9 to 21, replace the
current global SIGALRM usage with a per-wait, configurable timeout: read
PTY_TEST_TIMEOUT from the environment (default 3), compute an absolute deadline
= time.time() + timeout, and change the loop to use that deadline (e.g., while
b"%ready%" not in buffer and time.time() < deadline) returning or raising a
TimeoutError if expired; remove/stop using the global signal.alarm and ensure
any existing alarm is cancelled (signal.alarm(0)) immediately before any waitpid
calls; apply the same per-wait timeout pattern and alarm cancellation to the
other occurrences at lines 52-54 and 95-99.
| if (chunkStr.includes("\x03")) { | ||
| console.log("PARENT: exiting."); | ||
| process.stdin.pause(); | ||
| process.stdin.removeAllListeners(); | ||
| process.stdin.unref(); | ||
| console.log("%ready%"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Exit explicitly on Ctrl+C to reduce flakes
After signaling readiness, exit the process to avoid lingering handles keeping the event loop alive.
if (chunkStr.includes("\x03")) {
console.log("PARENT: exiting.");
process.stdin.pause();
process.stdin.removeAllListeners();
process.stdin.unref();
console.log("%ready%");
+ process.exit(0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (chunkStr.includes("\x03")) { | |
| console.log("PARENT: exiting."); | |
| process.stdin.pause(); | |
| process.stdin.removeAllListeners(); | |
| process.stdin.unref(); | |
| console.log("%ready%"); | |
| } | |
| if (chunkStr.includes("\x03")) { | |
| console.log("PARENT: exiting."); | |
| process.stdin.pause(); | |
| process.stdin.removeAllListeners(); | |
| process.stdin.unref(); | |
| console.log("%ready%"); | |
| process.exit(0); | |
| } |
🤖 Prompt for AI Agents
In test/js/node/readline/stdin-pause-child-reads.mjs around lines 12 to 18,
after signaling readiness with console.log("%ready%") the child process may
still keep the event loop alive; add an explicit process.exit(0) immediately
after the "%ready%" log so the process terminates cleanly (keeping the existing
process.stdin.pause(), removeAllListeners(), and unref() calls intact).
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isWindows } from "harness"; | ||
| import { dirname, join } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| test.skipIf(isWindows)("stdin pause should stop reading so child can read from stdin", async () => { | ||
| const script = join(__dirname, "stdin-pause-child-reads.mjs"); | ||
| const pty = join(__dirname, "run-with-pty.py"); | ||
| const runtime = process.env.TEST_RUNTIME || bunExe(); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [Bun.which("python3") ?? Bun.which("python") ?? "python", pty, runtime, script], | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| stdout: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| const lfilter = (l: string): boolean => { | ||
| if (!l) return false; | ||
| if (l.startsWith("%ready%")) return false; | ||
| if (l.startsWith("PYTHON:")) return false; | ||
| return true; | ||
| }; | ||
| expect( | ||
| stdout | ||
| .trim() | ||
| .split("\n") | ||
| .map(l => l.trim()) | ||
| .filter(lfilter), | ||
| ).toMatchInlineSnapshot(` | ||
| [ | ||
| "PARENT: reading", | ||
| "PARENT: received "1"", | ||
| "PARENT: received "2"", | ||
| "PARENT: received "3"", | ||
| "PARENT: received "\\r\\n"", | ||
| "PARENT: pause", | ||
| "CHILD: reading", | ||
| "CHILD: received "A"", | ||
| "CHILD: received "B"", | ||
| "CHILD: received "C"", | ||
| "CHILD: received "D"", | ||
| "CHILD: received "E"", | ||
| "CHILD: received "F"", | ||
| "CHILD: received "G"", | ||
| "CHILD: received "\\r\\n"", | ||
| "CHILD: exiting", | ||
| "PARENT: child exited with code 0. reading again.", | ||
| "PARENT: received "4"", | ||
| "PARENT: received "5"", | ||
| "PARENT: received "6"", | ||
| "PARENT: received "\\r\\n"", | ||
| "PARENT: pause", | ||
| "CHILD: reading", | ||
| "CHILD: received "H"", | ||
| "CHILD: received "I"", | ||
| "CHILD: received "J"", | ||
| "CHILD: received "K"", | ||
| "CHILD: received "L"", | ||
| "CHILD: received "M"", | ||
| "CHILD: received "N"", | ||
| "CHILD: received "O"", | ||
| "CHILD: received "P"", | ||
| "CHILD: received "\\r\\n"", | ||
| "CHILD: exiting", | ||
| "PARENT: child exited with code 0. reading again.", | ||
| "PARENT: received "\\u0003"", | ||
| "PARENT: exiting.", | ||
| ] | ||
| `); | ||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Flakiness risk: external 3s alarm in PTY harness
Test looks good, but the Python harness enforces a global 3s SIGALRM that can kill slow CI runs. Prefer a longer or per‑step timeout and cancel the alarm before waitpid.
I’ve proposed concrete changes in run-with-pty.py below to make timeouts per‑wait and configurable.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display the first part of the Python harness
fd run-with-pty.py -x sed -n '1,200p' {}
# Search for SIGALRM usage
rg -n "alarm" -C3 run-with-pty.pyLength of output: 2466
Global 3s SIGALRM in Python harness risks CI flakiness
In run-with-pty.py (parent process), signal.alarm(3) imposes a hard 3 second global timeout—replace it with configurable or per-step timeouts (and cancel the alarm before exit) to avoid premature SIGALRM on slow CI runs.
🤖 Prompt for AI Agents
In run-with-pty.py (parent process) where signal.alarm(3) is set, remove the
hardcoded global 3s alarm and replace it with a configurable or per-step
timeout; e.g., read a timeout from an environment variable or CLI arg (with a
sensible default), use per-operation timeouts (select/poll or timed reads)
rather than a global alarm, and ensure you cancel any alarm before exiting
(signal.alarm(0)) so a pending SIGALRM can’t kill the parent during slow CI
runs.
| console.log(` | ||
| ====== stderr ====== | ||
| ${stderrText} | ||
| ====== stdout ====== | ||
| ${stdoutText} | ||
| ====== exit code ====== | ||
| ${exitCode} | ||
| `); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider conditionalizing debug output.
The debug logging block aids troubleshooting but will print on every test run, potentially cluttering CI output. Consider moving this inside a conditional that only logs on test failure.
- console.log(`
-====== stderr ======
-${stderrText}
-====== stdout ======
-${stdoutText}
-====== exit code ======
-${exitCode}
-`);
-
expect(exitCode).toBe(0);
const err = stderrText;
+ if (exitCode !== 0) {
+ console.log(`
+====== stderr ======
+${stderrText}
+====== stdout ======
+${stdoutText}
+====== exit code ======
+${exitCode}
+`);
+ }
expect(err).toContain("1 pass");
expect(err).toContain("0 fail");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(` | |
| ====== stderr ====== | |
| ${stderrText} | |
| ====== stdout ====== | |
| ${stdoutText} | |
| ====== exit code ====== | |
| ${exitCode} | |
| `); | |
| expect(exitCode).toBe(0); | |
| const err = stderrText; | |
| if (exitCode !== 0) { | |
| console.log(` | |
| ====== stderr ====== | |
| ${stderrText} | |
| ====== stdout ====== | |
| ${stdoutText} | |
| ====== exit code ====== | |
| ${exitCode} | |
| `); | |
| } | |
| expect(err).toContain("1 pass"); | |
| expect(err).toContain("0 fail"); |
🤖 Prompt for AI Agents
In test/regression/issue/09041.test.ts around lines 15 to 22, the debug
console.log always prints test output and clutters CI; wrap the logging block in
a conditional that only runs on test failure (for example check exitCode !== 0
or non-empty stderrText) so the detailed debug output is printed only when the
test fails.
| import { $ } from "bun"; | ||
| import { join } from "path"; | ||
| test("09041", async () => { |
There was a problem hiding this comment.
Add missing imports for test utilities.
The file uses test() and expect() but doesn't import them from "bun:test". These must be explicitly imported per coding guidelines.
Apply this diff to add the required imports:
+import { test, expect } from "bun:test";
import { $ } from "bun";
import { join } from "path";
test("09041", async () => {🤖 Prompt for AI Agents
In test/regression/issue/09041/09041-fixture.ts around lines 1 to 3, the file
calls test() and expect() but doesn't import them; add an explicit import from
"bun:test" (e.g., import { test, expect } from "bun:test";) at the top of the
file so the test utilities are available before use.
| const buns = Array.from({ length: 5 }, (_, i) => | ||
| $`${process.argv0} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(), | ||
| ); |
There was a problem hiding this comment.
Use bunExe() instead of process.argv0.
The test spawns Bun processes using process.argv0, which violates coding guidelines. When spawning Bun in tests, you must use bunExe() from the harness to ensure the correct binary is used across different test environments.
As per coding guidelines.
Apply this diff to use bunExe():
+import { bunExe } from "harness";
import { $ } from "bun";
import { join } from "path"; const buns = Array.from({ length: 5 }, (_, i) =>
- $`${process.argv0} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(),
+ $`${bunExe()} run ${join(import.meta.dir, "09041-fixture.mjs")} < ${buffer}`.quiet(),
);🤖 Prompt for AI Agents
In test/regression/issue/09041/09041-fixture.ts around lines 12 to 14, replace
usage of process.argv0 when spawning Bun with bunExe() from the test harness:
update the template to call bunExe() instead of process.argv0, e.g.
$`${bunExe()} run ...`, and ensure bunExe is imported from the harness at the
top of the file (add the appropriate import if missing).
| function condense(str: string) { | ||
| const nums = str.split(",").map(n => (isNaN(+n) ? n : +n)); | ||
| let out: { len: number; start: number | string }[] = []; | ||
| for (let i = 0; i < nums.length; i++) { | ||
| const val = nums[i]!; | ||
| const last = out[out.length - 1]; | ||
| if (typeof val === "number" && last && typeof last.start === "number" && last.start + last.len === val) { | ||
| last.len++; | ||
| continue; | ||
| } | ||
| out.push({ len: 1, start: val }); | ||
| } | ||
| return out | ||
| .map(o => (o.len === 1 ? `${o.start}` : `RANGE(${o.start} through ${(o.start as number) + o.len - 1})`)) | ||
| .join(","); | ||
| } |
There was a problem hiding this comment.
Handle empty strings and edge cases in parsing.
The condense helper has potential edge cases:
- If the buffer ends with a comma,
split(",")produces an empty trailing string. - Line 30:
isNaN(+n)treats empty strings as0, which may not be the intended behavior. - Line 42: The type assertion
(o.start as number)assumes numeric values but could fail if the range contains string values.
Consider filtering out empty strings and adding validation:
function condense(str: string) {
- const nums = str.split(",").map(n => (isNaN(+n) ? n : +n));
+ const nums = str.split(",").filter(n => n !== "").map(n => (isNaN(+n) ? n : +n));
let out: { len: number; start: number | string }[] = [];
for (let i = 0; i < nums.length; i++) {
const val = nums[i]!;
const last = out[out.length - 1];
if (typeof val === "number" && last && typeof last.start === "number" && last.start + last.len === val) {
last.len++;
continue;
}
out.push({ len: 1, start: val });
}
return out
- .map(o => (o.len === 1 ? `${o.start}` : `RANGE(${o.start} through ${(o.start as number) + o.len - 1})`))
+ .map(o => {
+ if (o.len === 1) return `${o.start}`;
+ if (typeof o.start !== "number") throw new Error("Expected numeric range");
+ return `RANGE(${o.start} through ${o.start + o.len - 1})`;
+ })
.join(",");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function condense(str: string) { | |
| const nums = str.split(",").map(n => (isNaN(+n) ? n : +n)); | |
| let out: { len: number; start: number | string }[] = []; | |
| for (let i = 0; i < nums.length; i++) { | |
| const val = nums[i]!; | |
| const last = out[out.length - 1]; | |
| if (typeof val === "number" && last && typeof last.start === "number" && last.start + last.len === val) { | |
| last.len++; | |
| continue; | |
| } | |
| out.push({ len: 1, start: val }); | |
| } | |
| return out | |
| .map(o => (o.len === 1 ? `${o.start}` : `RANGE(${o.start} through ${(o.start as number) + o.len - 1})`)) | |
| .join(","); | |
| } | |
| function condense(str: string) { | |
| const nums = str.split(",").filter(n => n !== "").map(n => (isNaN(+n) ? n : +n)); | |
| let out: { len: number; start: number | string }[] = []; | |
| for (let i = 0; i < nums.length; i++) { | |
| const val = nums[i]!; | |
| const last = out[out.length - 1]; | |
| if (typeof val === "number" && last && typeof last.start === "number" && last.start + last.len === val) { | |
| last.len++; | |
| continue; | |
| } | |
| out.push({ len: 1, start: val }); | |
| } | |
| return out | |
| .map(o => { | |
| if (o.len === 1) return `${o.start}`; | |
| if (typeof o.start !== "number") throw new Error("Expected numeric range"); | |
| return `RANGE(${o.start} through ${o.start + o.len - 1})`; | |
| }) | |
| .join(","); | |
| } |
|
There's no automated testing for it but I manually confirmed this fixes the issue on windows ( |
pfgithub
changed the title
Fixes #23333, Fixes #13978
What does this PR do?
How did you verify your code works?