Conversation
WalkthroughUpdate default WebKit SHA; change HTML entrypoint C++ binding to return Changes
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Updated 12:03 AM PT - Sep 1st, 2025
❌ @Jarred-Sumner, your commit 0e6b0d6 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 22256That installs a local version of the PR into your bun-22256 --bun |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/tools/SetupWebKit.cmake (1)
1-6: Use CACHE STRING/PATH instead of option() for non-boolean vars (reduces footguns when passing hashes/paths).
This keeps WEBKIT_VERSION a proper string and WEBKIT_PATH a PATH, instead of boolean options.-option(WEBKIT_VERSION "The version of WebKit to use") +set(WEBKIT_VERSION "" CACHE STRING "WebKit commit hash or tag to use") -option(WEBKIT_PATH "The path to the WebKit directory") +set(WEBKIT_PATH "" CACHE PATH "Path to a prebuilt WebKit directory")Also applies to: 16-20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmake/tools/SetupWebKit.cmake(1 hunks)
⏰ 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
| if(NOT WEBKIT_VERSION) | ||
| set(WEBKIT_VERSION f9e86fe8dc0aa2fc1f137cc94777cb10637c23a4) | ||
| set(WEBKIT_VERSION 0afe46bfb970553b8a353bd95a8184eb9c243041) | ||
| endif() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Default WebKit hash bump looks fine; please verify release/tag and assets exist to prevent CI download failures.
The URL relies on tag autobuild-0afe46bfb970553b8a353bd95a8184eb9c243041. Confirm the release exists and includes the expected bun-webkit-{windows,macos,linux}-{amd64,arm64} artifacts for your targets.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
commit="0afe46bfb970553b8a353bd95a8184eb9c243041"
tag="autobuild-${commit}"
repo="oven-sh/WebKit"
echo "Listing assets for ${repo} ${tag}…"
curl -fsSL "https://api.github.com/repos/${repo}/releases/tags/${tag}" \
| jq -r '.assets[].name' | sort
echo "Ensure assets for your OS/ARCH (and -lto/-debug/-asan if applicable) are present."Length of output: 1516
Release tag and assets verified; Windows/ARM64 missing
The autobuild-0afe46bfb970553b8a353bd95a8184eb9c243041 release includes linux-{amd64,arm64}, macOS-{amd64,arm64}, and windows-amd64 artifacts but no Windows/ARM64 packages. If Windows ARM64 is a supported target, publish bun-webkit-windows-arm64*.tar.gz.
🤖 Prompt for AI Agents
In cmake/tools/SetupWebKit.cmake around lines 4 to 6, the pinned WEBKIT_VERSION
references a release that lacks a Windows/ARM64 binary; if Windows/ARM64 is a
supported target add and publish the corresponding release artifact named
bun-webkit-windows-arm64*.tar.gz to the autobuild release, rebuild the package
for that architecture and update any release manifest/upload scripts to include
windows-arm64; if Windows/ARM64 is not supported, update documentation/build
matrix and any CI/packaging scripts so the target is omitted to avoid expecting
a windows-arm64 artifact.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/js/builtins/TransformStreamInternals.ts (1)
157-165: Critical: backpressure promise never resolves due to wrong property name$newPromiseCapability(Promise) returns { promise, resolve, reject }. Using .$resolve here will be undefined and throw, breaking backpressure flips and causing stalls/timeouts.
Apply this fix:
- if (backpressureChangePromise !== undefined) backpressureChangePromise.$resolve.$call(); + if (backpressureChangePromise !== undefined) backpressureChangePromise.resolve.$call();src/js/node/fs.promises.ts (1)
516-547: File descriptor leak + incorrect early-return in FileHandle.close when fd === -1.
- close(): if kFd is set to -1 before an in-flight close completes, returning an already-resolved Promise masks work-in-progress and can short-circuit pending close().
- [kUnref] sets kFd = -1 before delegating to close(), causing close() to NO-OP and leak the OS fd.
Reorder the checks in close() and stop zeroing kFd in [kUnref]; let close() capture fd and perform the actual fs.close(fd).
Apply:
@@ - close = () => { - const fd = this[kFd]; - if (fd === -1) { - return Promise.$resolve(); - } - - if (this[kClosePromise]) { - return this[kClosePromise]; - } + close = () => { + const fd = this[kFd]; + // If a close is already in-flight, return that same promise. + if (this[kClosePromise]) { + return this[kClosePromise]; + } + // If already fully closed, resolve immediately. + if (fd === -1) { + return Promise.$resolve(); + } @@ - [kUnref]() { - if (--this[kRefs] === 0) { - this[kFd] = -1; - this.close().$then(this[kCloseResolve], this[kCloseReject]); - } - } + [kUnref]() { + if (--this[kRefs] === 0) { + // Delegate to close(); it will capture the fd and set kFd = -1 safely. + this.close().$then(this[kCloseResolve], this[kCloseReject]); + } + }Also applies to: 596-601
src/js/internal/webstreams_adapters.ts (1)
214-227: Replace improper$resolve/$rejectcalls
- In src/js/internal/webstreams_adapters.ts line 215, change
backpressurePromise.$resolve()→backpressurePromise.resolve()- In src/js/internal/webstreams_adapters.ts line 226, change
backpressurePromise.$reject(error)→backpressurePromise.reject(error)
No other occurrences found.src/js/bun/sql.ts (1)
174-197: Extract and use a generic pooled‐connection type instead ofPooledPostgresConnection
- In
src/js/bun/sql.tsat thequeryFromTransaction(ln 174) andunsafeQueryFromTransaction(ln 198) signatures, replace the Postgres‐specificpooledConnection: PooledPostgresConnectionwith a sharedPooledConnectiontype imported frominternal/sql/shared.- In
internal/sql/shared.ts, export aPooledConnection<Connection>alias (or similar) matching theConnectiontype parameter ofDatabaseAdapter<Connection, ConnectionHandle, QueryHandle>, and adjust each adapter (postgres,mysql,sqlite) to satisfy that generic.- Update both functions to accept
pooledConnection: PooledConnection<any>and ensureConnectionHandleis likewise imported frominternal/sql/sharedso that all adapters are supported.
🧹 Nitpick comments (11)
src/js/node/diagnostics_channel.ts (1)
15-17: Primordials-safe.thenfor consistency
No directPromise.resolve/rejectcalls remain in src/js/**. However, many modules still invoke.thendirectly. To guard against prototype tampering and align with other primordials, replace the local helper with the primordial-safe variant:- const PromisePrototypeThen = (promise, onFulfilled, onRejected) => promise.then(onFulfilled, onRejected); + const PromisePrototypeThen = $Promise.prototype.$then;And update its usage:
- return PromisePrototypeThen(promise, resolve, reject); + return PromisePrototypeThen.$call(promise, resolve, reject);Apply this optional refactor across remaining
.thencall sites in JS internals for hardened scheduling.src/js/internal/stream.ts (1)
53-53: Prefer $then for consistency with intrinsicsTo match the rest of the intrinsics migration and avoid prototype indirection, use $then.
- return Promise.$resolve().then(() => op.$apply(this, args)); + return Promise.$resolve().$then(() => op.$apply(this, args));src/js/node/fs.ts (1)
1078-1084: Callback path drops errors; add rejection handlerWhen a callback is provided, this returns a promise and never forwards rejections to the callback. Add an error handler to preserve Node-style callback semantics.
- if (!$isUndefinedOrNull(cb)) { - validateFunction(cb, "callback"); - return this.read().then(entry => cb(null, entry)); - } + if (!$isUndefinedOrNull(cb)) { + validateFunction(cb, "callback"); + return this.read().then(entry => cb(null, entry), err => cb(err)); + }src/js/internal/fs/cp.ts (1)
20-22: Bind Promise.$reject for consistency and to avoid illegal invocationElsewhere in the repo we bind these helpers. Binding is safer if the internal impl expects
thisto be the constructor.-const PromisePrototypeThen = $Promise.prototype.$then; -const PromiseReject = Promise.$reject; +const PromisePrototypeThen = $Promise.prototype.$then; +const PromiseReject = Promise.$reject.bind(Promise);src/js/node/fs.promises.ts (1)
8-8: Prefer removing the TODO and document the binding.The switch to $Promise.prototype.$finally is fine. Drop the TODO and add a short comment to clarify why we bind to the internal wrapper.
-var PromisePrototypeFinally = $Promise.prototype.$finally; //TODO +// Use Bun's internal Promise wrapper to avoid userland prototype tampering. +var PromisePrototypeFinally = $Promise.prototype.$finally;src/js/node/dns.ts (1)
44-47: Simplify rejection translation.You can avoid allocating a new rejected promise by rethrowing inside catch; behavior is identical and slightly cheaper.
-function translateErrorCode(promise: Promise<any>) { - return promise.catch(error => { - return Promise.$reject(withTranslatedError(error)); - }); -} +function translateErrorCode(promise: Promise<any>) { + return promise.catch(error => { + throw withTranslatedError(error); + }); +}src/js/internal/webstreams_adapters.ts (1)
248-262: Optional: prefer SafePromisePrototypeFinally for uniformity.You already alias $finally as SafePromisePrototypeFinally; use it for consistency instead of native finally in new Promise cases.
- return SafePromisePrototypeFinally.$call(backpressurePromise.promise, () => { + return SafePromisePrototypeFinally.$call(backpressurePromise.promise, () => { backpressurePromise = undefined; });src/js/node/timers.promises.ts (1)
71-73: Optional: use internal finally wrapper for consistency.Swap native finally() with $Promise.prototype.$finally to avoid prototype interference.
- return typeof onCancel !== "undefined" - ? returnValue.finally(() => signal.removeEventListener("abort", onCancel)) - : returnValue; + return typeof onCancel !== "undefined" + ? $Promise.prototype.$finally.$call(returnValue, () => signal.removeEventListener("abort", onCancel)) + : returnValue; @@ - return typeof onCancel !== "undefined" - ? returnValue.finally(() => signal.removeEventListener("abort", onCancel)) - : returnValue; + return typeof onCancel !== "undefined" + ? $Promise.prototype.$finally.$call(returnValue, () => signal.removeEventListener("abort", onCancel)) + : returnValue;Also applies to: 111-112
src/js/internal/sql/query.ts (1)
316-334: Consider exporting new symbols for diagnostics (optional)If other internal modules or test harnesses need to introspect, consider adding
_adapterand_getQueryHandleto thesymbolsexport. Not required if intentional encapsulation.symbols: { _resolve, _reject, _handle, _run, _queryStatus, _handler, _strings, _values, _flags, _results, + // Optionally expose for internal tooling: + // _adapter, + // _getQueryHandle, },test/js/bun/util/fuzzy-wuzzy.test.ts (2)
80-81: Keep signature parity forReadable.prototype.destroy(optional)Node’s destroy accepts an optional error and returns the stream. A no-op is fine for this fuzz, but a closer shape reduces surprises.
-require("stream").Readable.prototype.destroy = () => {}; +require("stream").Readable.prototype.destroy = function destroy(_err) { return this; };
83-91: Safer redefinition of prototype accessorsRedefining built-in accessor properties can throw if non-configurable. Wrap in try/catch and set configurable/writable where possible after deletion.
-Object.defineProperty(ReadableStreamDefaultReader.prototype, "closed", { value: undefined }); -Object.defineProperty(ReadableStreamBYOBReader.prototype, "closed", { value: undefined }); -Object.defineProperty(WritableStreamDefaultWriter.prototype, "ready", { value: undefined }); -Object.defineProperty(WritableStreamDefaultWriter.prototype, "closed", { value: undefined }); +for (const [proto, name] of [ + [ReadableStreamDefaultReader.prototype, "closed"], + [ReadableStreamBYOBReader.prototype, "closed"], + [WritableStreamDefaultWriter.prototype, "ready"], + [WritableStreamDefaultWriter.prototype, "closed"], +]) { + try { + Object.defineProperty(proto, name, { value: undefined, configurable: true, writable: true }); + } catch {} +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
src/bun.js/bindings/HTMLEntryPoint.cpp(2 hunks)src/js/builtins/BundlerPlugin.ts(1 hunks)src/js/builtins/ReadableStream.ts(2 hunks)src/js/builtins/ReadableStreamInternals.ts(5 hunks)src/js/builtins/StreamInternals.ts(1 hunks)src/js/builtins/TransformStreamInternals.ts(1 hunks)src/js/builtins/WritableStreamInternals.ts(1 hunks)src/js/bun/sql.ts(19 hunks)src/js/internal/fs/cp.ts(1 hunks)src/js/internal/primordials.js(2 hunks)src/js/internal/sql/query.ts(12 hunks)src/js/internal/stream.ts(1 hunks)src/js/internal/streams/end-of-stream.ts(1 hunks)src/js/internal/streams/from.ts(2 hunks)src/js/internal/streams/operators.ts(1 hunks)src/js/internal/webstreams_adapters.ts(3 hunks)src/js/node/diagnostics_channel.ts(1 hunks)src/js/node/dns.ts(4 hunks)src/js/node/events.ts(2 hunks)src/js/node/fs.promises.ts(2 hunks)src/js/node/fs.ts(1 hunks)src/js/node/readline.ts(1 hunks)src/js/node/timers.promises.ts(7 hunks)src/js/node/util.ts(1 hunks)src/js/node/vm.ts(1 hunks)src/js/node/worker_threads.ts(1 hunks)src/js/thirdparty/node-fetch.ts(1 hunks)test/js/bun/util/fuzzy-wuzzy.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
src/js/node/diagnostics_channel.tssrc/js/builtins/WritableStreamInternals.tssrc/js/builtins/BundlerPlugin.tssrc/js/internal/primordials.jssrc/js/node/fs.tssrc/js/node/util.tssrc/js/internal/webstreams_adapters.tssrc/js/node/events.tssrc/js/node/worker_threads.tssrc/js/builtins/ReadableStream.tssrc/js/internal/streams/operators.tssrc/js/node/dns.tssrc/js/node/readline.tssrc/js/node/fs.promises.tssrc/js/internal/streams/end-of-stream.tssrc/js/builtins/StreamInternals.tssrc/js/internal/fs/cp.tssrc/js/thirdparty/node-fetch.tssrc/js/internal/stream.tssrc/js/builtins/TransformStreamInternals.tssrc/js/bun/sql.tssrc/js/node/timers.promises.tssrc/js/node/vm.tssrc/js/internal/streams/from.tstest/js/bun/util/fuzzy-wuzzy.test.tssrc/js/builtins/ReadableStreamInternals.tssrc/js/internal/sql/query.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/node/diagnostics_channel.tssrc/js/builtins/WritableStreamInternals.tssrc/js/builtins/BundlerPlugin.tssrc/js/internal/primordials.jssrc/js/node/fs.tssrc/js/node/util.tssrc/js/internal/webstreams_adapters.tssrc/js/node/events.tssrc/js/node/worker_threads.tssrc/js/builtins/ReadableStream.tssrc/js/internal/streams/operators.tssrc/js/node/dns.tssrc/js/node/readline.tssrc/js/node/fs.promises.tssrc/js/internal/streams/end-of-stream.tssrc/js/builtins/StreamInternals.tssrc/js/internal/fs/cp.tssrc/js/thirdparty/node-fetch.tssrc/js/internal/stream.tssrc/js/builtins/TransformStreamInternals.tssrc/js/bun/sql.tssrc/js/node/timers.promises.tssrc/js/node/vm.tssrc/js/internal/streams/from.tssrc/js/builtins/ReadableStreamInternals.tssrc/js/internal/sql/query.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/js/bun/util/fuzzy-wuzzy.test.ts
test/js/bun/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Put Bun-specific API tests under test/js/bun/
Files:
test/js/bun/util/fuzzy-wuzzy.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/js/bun/util/fuzzy-wuzzy.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/js/bun/util/fuzzy-wuzzy.test.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/util/fuzzy-wuzzy.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/bun/util/fuzzy-wuzzy.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/util/fuzzy-wuzzy.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/bun/util/fuzzy-wuzzy.test.ts
**/*.{cpp,cxx,cc,hpp,hxx,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/HTMLEntryPoint.cpp
src/bun.js/bindings/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.{cpp,hpp,h}: When implementing JavaScript classes in C++, create Foo, FooPrototype, and FooConstructor for public constructors
Define properties using JSC HashTableValue arrays for JS class bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for bound classes
Files:
src/bun.js/bindings/HTMLEntryPoint.cpp
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/HTMLEntryPoint.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/HTMLEntryPoint.cpp
🧠 Learnings (12)
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
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/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Cache structures in ZigGlobalObject for bound classes
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
src/bun.js/bindings/HTMLEntryPoint.cpp
🧬 Code graph analysis (6)
src/js/builtins/BundlerPlugin.ts (1)
src/js/node/timers.ts (2)
promises(12-14)promises(15-17)
src/js/node/events.ts (1)
src/js/node/worker_threads.ts (7)
value(153-155)value(159-159)value(163-163)value(167-167)value(171-171)value(175-177)value(181-181)
src/js/internal/streams/operators.ts (1)
src/js/internal/primordials.js (2)
PromiseResolve(97-97)PromisePrototypeThen(86-86)
src/js/internal/streams/end-of-stream.ts (1)
src/js/internal/primordials.js (1)
PromisePrototypeThen(86-86)
src/js/node/vm.ts (1)
src/js/internal/primordials.js (2)
PromisePrototypeThen(86-86)PromiseResolve(97-97)
src/js/internal/sql/query.ts (1)
src/js/bun/sqlite.ts (1)
handle(432-434)
⏰ 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 (29)
src/js/node/util.ts (1)
249-251: LGTM: switched to Promise.$resolve for early-abort pathConsistent with the repo-wide primordials migration; no behavior change.
src/js/thirdparty/node-fetch.ts (1)
192-194: LGTM: blobFrom now uses Promise.$resolveMatches internal Promise usage elsewhere; API remains Promise.
src/js/builtins/StreamInternals.ts (1)
39-41: LGTM: bind $then from $Promise.prototypeThis avoids userland prototype tampering and keeps microtask semantics intact.
src/js/node/readline.ts (1)
46-46: LGTM: PromiseReject bound to internal wrapperKeeps rejection paths consistent with primordials across the codebase.
src/js/builtins/BundlerPlugin.ts (1)
127-127: Switch to Promise.$reject is fine hereConsistent with the internal promise migration and keeps Promise.all behavior intact.
src/js/internal/primordials.js (1)
86-86: LGTM: migrate to $then/$resolve intrinsicsAligns with internal wrappers and avoids userland Promise hooks.
Also applies to: 98-98
src/js/node/worker_threads.ts (1)
332-335: LGTM: wrap non-promise exit code with Promise.$resolveKeeps terminate() contract stable and consistent with intrinsics.
src/js/node/fs.ts (1)
1083-1084: Switch to Promise.$resolve is consistent with repo-wide promise wrappersChange aligns with the internal $-prefixed Promise usage and should be behaviorally identical.
src/js/node/vm.ts (1)
20-22: Rebinding to $Promise wrappers looks goodUsing $then and $resolve here matches primordials and avoids userland Promise tampering.
src/js/internal/streams/operators.ts (1)
11-14: LGTM on migrating to $-prefixed Promise helpersConsistent with primordials; protects against userland mutation and maintains perf.
src/js/node/events.ts (1)
517-517: LGTM: moved iterator paths to Promise.$resolve/$reject.The migration keeps semantics while avoiding userland Promise tampering.
Also applies to: 525-525, 626-627
src/js/node/dns.ts (2)
739-747: LGTM: $resolve for fast-paths.Using Promise.$resolve in lookup() fast-paths is consistent with the internal wrappers.
Also applies to: 753-753
777-778: LGTM: $reject in error path.Consistent with translateErrorCode and wrapper usage.
src/js/internal/webstreams_adapters.ts (6)
26-29: LGTM: bind internal Promise helpers once.Using $then/$finally consistently via aliases is good.
276-279: LGTM: use bound Promise.$resolve for no-op close().Consistent with internal wrappers.
356-363: LGTM: $then usage across writer/reader paths.The stream adapters correctly centralize on $Promise.prototype.$then.
Also applies to: 677-685, 720-723, 729-740, 742-752
477-485: LGTM: known-internal error shaping.The mapping to AbortError/TypeError from zlib failures is sound.
494-511: LGTM: ReadableStream adapter lifecycle.start/pull/cancel wiring matches Node<>WebStreams expectations.
541-569: LGTM: duplex pair creation mirrors readable/writable states.Correctly handles destroyed and half-open cases.
src/js/node/timers.promises.ts (1)
36-56: LGTM: migrated to Promise.$reject/$resolve.Consistent usage in validation and abort paths; aligns with internal Promise wrappers.
Also applies to: 81-95, 127-128, 137-138, 147-148, 156-156, 163-163, 221-221
src/js/bun/sql.ts (1)
123-125: Migration to Promise.$reject looks goodConsistently using Bun’s promise intrinsics keeps these paths insulated from userland Promise monkey-patching.
Also applies to: 138-140, 194-196, 222-224
src/js/internal/sql/query.ts (1)
26-26: Good abstraction: lazy, symbol-keyed handle accessor + $-promise plumbingThe new
_getQueryHandleand adapter field clean up handle creation and centralize error propagation.$then/$catch/$finallyavoids userland hooks. Looks solid.Also applies to: 52-66, 121-139, 164-173, 180-190, 200-209, 226-234, 242-250, 259-286
src/js/builtins/ReadableStream.ts (1)
117-118: LGTM: switched to Promise.$reject/$resolve in helpersMatches the project-wide migration to Bun’s Promise intrinsics and avoids userland interference.
Also applies to: 129-130, 326-327, 340-341
src/js/internal/streams/from.ts (1)
3-3: Align to intrinsics: using $Promise.prototype.$then is correctMatches primordials; safe swap for internal thenable handling.
src/js/internal/streams/end-of-stream.ts (1)
26-26: Switch to $Promise.prototype.$then looks goodConsistent with primordials and other modules.
src/js/builtins/ReadableStreamInternals.ts (4)
572-572: Early-return via Promise.$resolve() is fineKeeps semantics; no behavior change.
615-617: Microtask scheduling via Promise.$resolve().$then is fineNo concerns.
624-624: Second early-return via Promise.$resolve() is fineConsistent with the above.
1056-1056: Route errors through $Promise.prototype.$catchCorrect alignment with internal intrinsics.
| #include <JavaScriptCore/JSPromise.h> | ||
| namespace Bun { | ||
| using namespace JSC; | ||
| extern "C" JSInternalPromise* Bun__loadHTMLEntryPoint(Zig::GlobalObject* globalObject) | ||
| extern "C" JSPromise* Bun__loadHTMLEntryPoint(Zig::GlobalObject* globalObject) | ||
| { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ABI change to JSPromise; align Zig/C++ declarations and remove unused include*
- Return type changed from JSInternalPromise* to JSPromise*. Ensure all Zig externs and call sites use JSPromise* (or EncodedJSValue wrapper) consistently.
#include <JavaScriptCore/JSInternalPromise.h>appears unused now.
-#include <JavaScriptCore/JSInternalPromise.h>
+#include <JavaScriptCore/JSPromise.h>Also applies to: 19-20, 33-41
🏁 Script executed:
#!/bin/bash
# Find all references to Bun__loadHTMLEntryPoint and their signatures
rg -nP 'Bun__loadHTMLEntryPoint' -C3
# Check for JSInternalPromise* usages that may need updating
rg -nP 'JSInternalPromise\s*\*' src/bun.js -C2
# Confirm Zig extern matches JSPromise* or EncodedJSValue
rg -nP 'extern\s+"C"\s+.*Bun__loadHTMLEntryPoint' -n -C2Length of output: 26600
Align Zig extern and C++ signatures for Bun__loadHTMLEntryPoint
- In
src/bun.js/VirtualMachine.zig(around line 2096), changetoextern fn Bun__loadHTMLEntryPoint(global: *JSGlobalObject) *JSInternalPromise;
extern fn Bun__loadHTMLEntryPoint(global: *JSGlobalObject) *JSPromise;
- Update the
reloadEntryPointcall at line 2143 to accept*JSPromise(or useEncodedJSValue) instead of*JSInternalPromise. - No
JSInternalPromise.hinclude remains inHTMLEntryPoint.cpp—it already uses<JavaScriptCore/JSPromise.h>.
🤖 Prompt for AI Agents
In src/bun.js/bindings/HTMLEntryPoint.cpp around lines 9 to 13, the C++
signature uses JSPromise but Zig declares Bun__loadHTMLEntryPoint as returning
*JSInternalPromise; change the Zig extern to return *JSPromise (or
EncodedJSValue) and update the call site at VirtualMachine.zig reloadEntryPoint
(around line 2143) to accept *JSPromise (or convert to EncodedJSValue) so the
Zig and C++ signatures match; no changes needed to HTMLEntryPoint.cpp includes
since it already includes <JavaScriptCore/JSPromise.h>.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/js/bun/sql.ts (1)
291-296: Replace this.connectionClosedError() with pool.connectionClosedError() inside arrow/closure contexts.These closures don’t bind
this; callingthis.connectionClosedError()can throw TypeError and mask the real error. Use thepoolinstance directly.Apply targeted fixes (pattern repeated across the ranges):
- return Promise.$reject(this.connectionClosedError()); + return Promise.$reject(pool.connectionClosedError());- return Promise.$reject(this.connectionClosedError()); + return Promise.$reject(pool.connectionClosedError());- throw this.connectionClosedError(); + throw pool.connectionClosedError();- return Promise.$reject(this.connectionClosedError()); + return Promise.$reject(pool.connectionClosedError());Similarly update other occurrences within the listed ranges.
Also applies to: 321-335, 342-360, 424-442, 552-557, 562-568, 593-599, 817-825, 858-869, 877-891, 897-905
🧹 Nitpick comments (1)
test/js/bun/util/fuzzy-wuzzy.test.ts (1)
87-90: Make these defined properties configurable/writable to avoid freezing prototypes.
Defining with only value defaults to non-writable, non-configurable. Prefer explicit descriptors to reduce brittleness.Apply this diff:
-Object.defineProperty(ReadableStreamDefaultReader.prototype, "closed", { value: undefined }); -Object.defineProperty(ReadableStreamBYOBReader.prototype, "closed", { value: undefined }); -Object.defineProperty(WritableStreamDefaultWriter.prototype, "ready", { value: undefined }); -Object.defineProperty(WritableStreamDefaultWriter.prototype, "closed", { value: undefined }); +Object.defineProperty(ReadableStreamDefaultReader.prototype, "closed", { configurable: true, writable: true, enumerable: false, value: undefined }); +Object.defineProperty(ReadableStreamBYOBReader.prototype, "closed", { configurable: true, writable: true, enumerable: false, value: undefined }); +Object.defineProperty(WritableStreamDefaultWriter.prototype, "ready", { configurable: true, writable: true, enumerable: false, value: undefined }); +Object.defineProperty(WritableStreamDefaultWriter.prototype, "closed", { configurable: true, writable: true, enumerable: false, value: undefined });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/js/bun/sql.ts(20 hunks)src/js/internal/sql/query.ts(11 hunks)src/js/internal/streams/from.ts(1 hunks)src/js/internal/webstreams_adapters.ts(1 hunks)src/js/node/fs.promises.ts(2 hunks)test/js/bun/util/fuzzy-wuzzy.test.ts(4 hunks)test/js/sql/sqlite-sql.test.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/js/internal/streams/from.ts
- src/js/node/fs.promises.ts
- src/js/internal/webstreams_adapters.ts
- src/js/internal/sql/query.ts
🧰 Additional context used
📓 Path-based instructions (10)
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/js/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
test/js/sql/sqlite-sql.test.tssrc/js/bun/sql.tstest/js/bun/util/fuzzy-wuzzy.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/js/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/js/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.test.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.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/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.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/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.test.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/bun/sql.ts
test/js/bun/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Put Bun-specific API tests under test/js/bun/
Files:
test/js/bun/util/fuzzy-wuzzy.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/util/fuzzy-wuzzy.test.ts
🧠 Learnings (17)
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/**/*.test.ts : Prefer async/await over callbacks in tests
Applied to files:
test/js/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/**/*.test.ts : Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Applied to files:
test/js/sql/sqlite-sql.test.tstest/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Applied to files:
test/js/sql/sqlite-sql.test.ts
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
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/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : Put Bun-specific API tests under test/js/bun/
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
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/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
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/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : When spawning Bun processes, use bunExe() and bunEnv from harness
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
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/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
Learning: Applies to test/**/*.{js,ts} : Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
test/js/bun/util/fuzzy-wuzzy.test.ts
🧬 Code graph analysis (1)
test/js/sql/sqlite-sql.test.ts (1)
src/js/internal-for-testing.ts (1)
SQL(17-17)
🔇 Additional comments (5)
test/js/bun/util/fuzzy-wuzzy.test.ts (2)
22-22: LGTM: importing expect from bun:test is appropriate here.
80-80: No-op Readable.prototype.destroy is fine for this fuzzer.
Prevents uncatchable teardown errors without side effects in this test scope.test/js/sql/sqlite-sql.test.ts (1)
1094-1094: Verify changed error text won’t break consumers.Message changed to “Inner! transaction failed”. Confirm no snapshots or string-matching tests elsewhere depend on the old text.
src/js/bun/sql.ts (2)
748-749: Savepoint finalization ordering LGTM.Returning
await promise.finally(...)preserves the resolved value/rejection while ensuring cleanup runs.
123-124: Consistent migration to Promise.$reject/$resolve looks good.These changes align with the internal $Promise helpers and reduce micro-allocations.
Also applies to: 138-139, 195-196, 223-223, 385-386, 424-425, 441-442, 649-650, 902-903
| test("call", () => { | ||
| expect(async () => await callAllMethods(require(mod), `require("${mod}")`)).not.toThrow(); | ||
| }); | ||
| test("construct", () => { | ||
| expect(async () => await constructAllConstructors(require(mod), `require("${mod}")`)).not.toThrow(); | ||
| }); | ||
| test("construct-subclass", () => { | ||
| expect( | ||
| async () => await constructAllConstructorsWithSubclassing(require(mod), `require("${mod}")`), | ||
| ).not.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix: expect(...).not.toThrow used with an async wrapper masks failures.
toThrow only catches synchronous throws. Wrapping a sync function in an async arrow returns a Promise; synchronous throws become rejections and won’t be caught by toThrow, leading to false positives.
Apply this diff:
- test("call", () => {
- expect(async () => await callAllMethods(require(mod), `require("${mod}")`)).not.toThrow();
- });
+ test("call", () => {
+ expect(() => callAllMethods(require(mod), `require("${mod}")`)).not.toThrow();
+ });
- test("construct", () => {
- expect(async () => await constructAllConstructors(require(mod), `require("${mod}")`)).not.toThrow();
- });
+ test("construct", () => {
+ expect(() => constructAllConstructors(require(mod), `require("${mod}")`)).not.toThrow();
+ });
- test("construct-subclass", () => {
- expect(
- async () => await constructAllConstructorsWithSubclassing(require(mod), `require("${mod}")`),
- ).not.toThrow();
- });
+ test("construct-subclass", () => {
+ expect(() => constructAllConstructorsWithSubclassing(require(mod), `require("${mod}")`)).not.toThrow();
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/js/bun/util/fuzzy-wuzzy.test.ts around lines 454 to 465, the tests wrap
async calls in expect(...).not.toThrow() which only catches synchronous throws
and masks promise rejections; change each test to be async and assert the
promise resolves instead — e.g. make the test callback async and replace
expect(async () => await callAllMethods(...)).not.toThrow() with await
expect(callAllMethods(require(mod),
`require("${mod}")`)).resolves.toBeUndefined(); do the same replacement for
constructAllConstructors and constructAllConstructorsWithSubclassing so
rejections fail the test.
| expect(async () => await callAllMethods(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow(); | ||
| await Bun.sleep(1); | ||
| }); | ||
| // TODO: hangs in CI on Windows. | ||
| test.skipIf(isWindows && Global === Bun)("construct", async () => { | ||
| await Bun.sleep(1); | ||
| constructAllConstructors(Global, Global === Bun ? "Bun" : "globalThis"); | ||
| expect(async () => await constructAllConstructors(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow(); | ||
| await Bun.sleep(1); | ||
| }); | ||
| test.skipIf(isWindows && Global === Bun)("construct-subclass", async () => { | ||
| await Bun.sleep(1); | ||
| constructAllConstructorsWithSubclassing(Global, Global === Bun ? "Bun" : "globalThis"); | ||
| expect( | ||
| async () => await constructAllConstructorsWithSubclassing(Global, Global === Bun ? "Bun" : "globalThis"), | ||
| ).not.toThrow(); | ||
| await Bun.sleep(1); |
There was a problem hiding this comment.
Same issue: async wrapper with toThrow hides synchronous throws.
Use a direct sync callback with toThrow. Keep the surrounding sleeps as-is if needed for CI flakiness.
Apply this diff:
- expect(async () => await callAllMethods(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow();
+ expect(() => callAllMethods(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow();
@@
- expect(async () => await constructAllConstructors(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow();
+ expect(() => constructAllConstructors(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow();
@@
- expect(
- async () => await constructAllConstructorsWithSubclassing(Global, Global === Bun ? "Bun" : "globalThis"),
- ).not.toThrow();
+ expect(() => constructAllConstructorsWithSubclassing(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow();📝 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.
| expect(async () => await callAllMethods(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow(); | |
| await Bun.sleep(1); | |
| }); | |
| // TODO: hangs in CI on Windows. | |
| test.skipIf(isWindows && Global === Bun)("construct", async () => { | |
| await Bun.sleep(1); | |
| constructAllConstructors(Global, Global === Bun ? "Bun" : "globalThis"); | |
| expect(async () => await constructAllConstructors(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow(); | |
| await Bun.sleep(1); | |
| }); | |
| test.skipIf(isWindows && Global === Bun)("construct-subclass", async () => { | |
| await Bun.sleep(1); | |
| constructAllConstructorsWithSubclassing(Global, Global === Bun ? "Bun" : "globalThis"); | |
| expect( | |
| async () => await constructAllConstructorsWithSubclassing(Global, Global === Bun ? "Bun" : "globalThis"), | |
| ).not.toThrow(); | |
| await Bun.sleep(1); | |
| expect(() => callAllMethods(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow(); | |
| await Bun.sleep(1); | |
| }); | |
| // TODO: hangs in CI on Windows. | |
| test.skipIf(isWindows && Global === Bun)("construct", async () => { | |
| await Bun.sleep(1); | |
| expect(() => constructAllConstructors(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow(); | |
| await Bun.sleep(1); | |
| }); | |
| test.skipIf(isWindows && Global === Bun)("construct-subclass", async () => { | |
| await Bun.sleep(1); | |
| expect(() => constructAllConstructorsWithSubclassing(Global, Global === Bun ? "Bun" : "globalThis")).not.toThrow(); | |
| await Bun.sleep(1); |
🤖 Prompt for AI Agents
In test/js/bun/util/fuzzy-wuzzy.test.ts around lines 515 to 529, the tests wrap
async calls in an async arrow passed to expect(...).not.toThrow(), which hides
synchronous exceptions; change those to direct synchronous callbacks that invoke
the async function (e.g., expect(() => callAllMethods(...)).not.toThrow()) so
any synchronous throw is caught by toThrow, and keep the surrounding Bun.sleep
calls and test.skipIf guards as-is.
| expect(async () => await sql`INSERT INTO test VALUES (2)`.execute()).toThrowErrorMatchingInlineSnapshot( | ||
| `"attempt to write a readonly database"`, | ||
| ); |
There was a problem hiding this comment.
Fix async wrapper with toThrow; use sync callback (or .rejects for Promises).
toThrow* expects a synchronous-throwing callback. Wrapping the call in async () => await ... turns errors into rejected Promises, so these assertions will not behave as intended and will intermittently pass/fail. For query APIs that synchronously throw on .execute(), pass a plain sync callback. For APIs that return Promises (e.g., reserve()), assert with .rejects.
Apply representative fixes (repeat across the listed ranges):
- expect(async () => await sql`INSERT INTO test VALUES (2)`.execute()).toThrowErrorMatchingInlineSnapshot(
+ expect(() => sql`INSERT INTO test VALUES (2)`.execute()).toThrowErrorMatchingInlineSnapshot(- expect(
- async () => await sql`CREATE TABLE dynamic_structure (${columns})`.execute(),
- ).toThrowErrorMatchingInlineSnapshot(`"near "?": syntax error"`);
+ expect(() => sql`CREATE TABLE dynamic_structure (${columns})`.execute())
+ .toThrowErrorMatchingInlineSnapshot(`"near "?": syntax error"`);For promise-returning APIs:
- expect(async () => await sql.reserve()).toThrowErrorMatchingInlineSnapshot(
- `"This adapter doesn't support connection reservation"`,
- );
+ await expect(sql.reserve()).rejects.toThrowErrorMatchingInlineSnapshot(
+ `"This adapter doesn't support connection reservation"`,
+ );Also applies to: 694-696, 969-971, 994-997, 1029-1031, 1037-1039, 1110-1116, 1347-1349, 1354-1356, 1360-1367, 1372-1390, 1395-1397, 1401-1412, 1426-1429, 1465-1468, 1473-1475, 1478-1481, 1571-1573
🤖 Prompt for AI Agents
In test/js/sql/sqlite-sql.test.ts around lines 333 to 335, the test wraps the
call in an async arrow (expect(async () => await
sql`...`.execute()).toThrow...), which converts thrown errors into rejected
Promises and breaks toThrow; replace the async wrapper with a plain synchronous
callback if .execute() throws synchronously (e.g., expect(() => sql`INSERT INTO
test VALUES (2)`.execute()).toThrowErrorMatchingInlineSnapshot(...)), or—if the
API actually returns a Promise—use the Promise-aware form (await
expect(sql`...`.execute()).rejects.toThrow(...)); apply the same pattern to the
other ranges listed in the comment.
- Fix query.ts: Replace incorrect `super.catch.$apply` with `super.$catch.$apply` - Fix query.ts: Replace incorrect `super.finally.$apply` with `super.finally.$apply` - Add null safety checks for PostgreSQL readyConnections to prevent crashes This addresses the test failures in: - test/js/sql/sql-mysql.transactions.test.ts - test/js/sql/sql.test.ts (PostgreSQL connection error handling) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/internal/sql/query.ts (2)
165-173: Don’t create a handle during resolve/reject; it can change the rejection reason and double-rejectCalling [_getQueryHandle] inside reject() can throw and recursively call reject(), overwriting the original error and potentially causing a second rejection. Same for resolve(): creating a handle just to call done() is unnecessary if one doesn’t exist.
resolve(x: T) { this[_queryStatus] &= ~SQLQueryStatus.active; - const handle = this[_getQueryHandle](); - - if (!handle) { - return this; - } - - handle.done?.(); + const handle = this[_handle]; + handle?.done?.(); return this[_resolve](x); } reject(x: Error) { this[_queryStatus] &= ~SQLQueryStatus.active; this[_queryStatus] |= SQLQueryStatus.error; - if (!(this[_queryStatus] & SQLQueryStatus.invalidHandle)) { - const handle = this[_getQueryHandle](); - - if (!handle) { - return this[_reject](x); - } - - handle.done?.(); - } + const handle = this[_handle]; + handle?.done?.(); return this[_reject](x); }Also applies to: 175-190
226-234: Guard raw()/values() against notTagged to avoid premature handle creationThese mode setters currently create a handle even for notTagged queries, which bypasses the established gating in run()/then()/catch()/finally(). Throw early here for consistency and to avoid unnecessary allocations/work.
raw() { + if (this[_flags] & SQLQueryFlags.notTagged) { + throw this[_adapter].notTaggedCallError(); + } const handle = this[_getQueryHandle](); if (!handle) { return this; } handle.setMode(SQLQueryResultMode.raw); return this; } values() { + if (this[_flags] & SQLQueryFlags.notTagged) { + throw this[_adapter].notTaggedCallError(); + } const handle = this[_getQueryHandle](); if (!handle) { return this; } handle.setMode(SQLQueryResultMode.values); return this; }Also applies to: 241-250
🧹 Nitpick comments (11)
src/js/internal/sql/postgres.ts (9)
409-416: Avoid optional chaining on readyConnections; keep the invariant non-null SetreadyConnections is constructed in the adapter ctor and never nulled. Optional chaining here weakens the invariant and is inconsistent with other sites (e.g., Line 848 uses .clear() directly). Prefer direct delete.
- this.adapter.readyConnections?.delete(this); + this.adapter.readyConnections.delete(this);
674-681: Drop the || [] fallback; preserve the Set invariantUsing a fallback hides bugs and loosens typing. Array.from(Set) is already safe.
- const nonReservedConnections = Array.from(this.readyConnections || []).filter( + const nonReservedConnections = Array.from(this.readyConnections).filter( c => !(c.flags & PooledConnectionFlags.preReserved) && c.queryCount < maxDistribution, );
752-755: Simplify: readyConnections is always defined- if (this.readyConnections?.size > 0) return true; + if (this.readyConnections.size > 0) return true;
775-777: Same here: remove optional chaining- if (this.readyConnections?.size > 0) { + if (this.readyConnections.size > 0) {
915-921: Redundant undefined check; maintain strict invariantreadyConnections exists; keep the check simple and typed.
- if (!this.readyConnections || this.readyConnections.size === 0) { + if (this.readyConnections.size === 0) {
987-996: Iterate the Set directly (no fallback)- for (const connection of this.readyConnections || []) { + for (const connection of this.readyConnections) {
1001-1003: Remove optional chaining on delete- this.readyConnections?.delete(connection); + this.readyConnections.delete(connection);
69-89: Unreachable code after return in onResolvePostgresQueryEverything from Line 69 to 89 is dead due to the return on Line 67. Please remove to reduce bundle size and avoid confusing maintenance.
- return; - - /// prepared statements - $assert(result instanceof SQLResultArray, "Invalid result array"); - if (typeof commandTag === "string") { - if (commandTag.length > 0) { - result.command = commandTag; - } - } else { - result.command = cmds[commandTag]; - } - - result.count = count || 0; - if (queries) { - const queriesIndex = queries.indexOf(query); - if (queriesIndex !== -1) { - queries.splice(queriesIndex, 1); - } - } - try { - query.resolve(result); - } catch {} + return;
123-139: Typo in interface: “databae” → “database”Purely cosmetic in typings but worth fixing for clarity.
- databae: string, + database: string,src/js/internal/sql/query.ts (2)
52-66: Make [_getQueryHandle] explicit and early-return on failureAdd an explicit return type and return null immediately after reject to avoid fall-through. This clarifies control flow and prevents callers from misinterpreting undefined.
- [_getQueryHandle]() { + [_getQueryHandle](): Handle | null { let handle = this[_handle]; if (!handle) { try { const [sql, values] = this[_adapter].normalizeQuery(this[_strings], this[_values]); this[_handle] = handle = this[_adapter].createQueryHandle(sql, values, this[_flags]); } catch (err) { this[_queryStatus] |= SQLQueryStatus.error | SQLQueryStatus.invalidHandle; - this.reject(err as Error); + this.reject(err as Error); + return null; } } return handle; }
121-126: Set “executed” after obtaining a valid handleMinor ordering nit: mark executed after a successful handle acquisition. This avoids “executed” being set on invalid-handle paths.
- this[_queryStatus] |= SQLQueryStatus.executed; - const handle = this[_getQueryHandle](); + const handle = this[_getQueryHandle](); if (!handle) { return this; } + this[_queryStatus] |= SQLQueryStatus.executed;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/js/internal/sql/postgres.ts(7 hunks)src/js/internal/sql/query.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
src/js/internal/sql/postgres.tssrc/js/internal/sql/query.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/internal/sql/postgres.tssrc/js/internal/sql/query.ts
🧬 Code graph analysis (1)
src/js/internal/sql/query.ts (1)
src/js/bun/sqlite.ts (1)
handle(432-434)
🔇 Additional comments (3)
src/js/internal/sql/query.ts (3)
92-95: LGTM: resolve/reject captors stored correctly
202-206: LGTM: cancel only attempts adapter cancel post-execution
272-276: Ignore this check: marking the returned promise matches project-wide convention.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/sql/query.ts (1)
130-160: Fix lint failure and a cancellation race in async path
- The line
(await 1)triggers eslint-plugin-unicorn(no-unnecessary-await). Use a real Promise to yield a microtask.- If
cancel()is called during the microtask yield, the handler still runs. Re-check after the await and bail.Apply this diff:
this[_queryStatus] |= SQLQueryStatus.executed; const handle = this.#getQueryHandle(); if (!handle) { return this; } - - (await 1) as never as Promise<void>; + // Yield to a microtask without tripping no-unnecessary-await + await Promise.resolve(); + // If cancelled while yielding, stop before invoking the handler + if (this[_queryStatus] & SQLQueryStatus.cancelled) { + handle.cancel?.(); + return; + } try { return handler(this, handle); } catch (err) { this[_queryStatus] |= SQLQueryStatus.error; this.reject(err as Error); }
🧹 Nitpick comments (4)
test/js/sql/sql-mysql.transactions.test.ts (2)
35-42: Prefer expect(...).rejects over manual try/catch for cleaner intentThis removes unreachable() and asserts the error shape directly.
- try { - await sql`BEGIN`; - expect.unreachable(); - } catch (error) { - expect(error.code).toBe("ERR_MYSQL_UNSAFE_TRANSACTION"); - } + await expect(sql`BEGIN`).rejects.toHaveProperty("code", "ERR_MYSQL_UNSAFE_TRANSACTION");
44-48: Drop superfluous return before expectReturning the expect isn’t needed here.
- return expect(error.code).toBe("ERR_MYSQL_UNSAFE_TRANSACTION"); + expect(error.code).toBe("ERR_MYSQL_UNSAFE_TRANSACTION");src/js/internal/sql/query.ts (2)
185-194: Avoid creating a handle on resolve() path
resolve()should not create a handle just to calldone(). Use the cached handle to prevent side effects if none exists.- const handle = this.#getQueryHandle(); + const handle = this[_handle];
196-211: Don’t create a handle in reject()On error paths, calling
#getQueryHandle()can instantiate a handle solely to calldone(), which is unnecessary and can cascade errors. Use the cached handle if present.- if (!(this[_queryStatus] & SQLQueryStatus.invalidHandle)) { - const handle = this.#getQueryHandle(); - - if (!handle) { - return this[_reject](x); - } - - handle.done?.(); - } + const handle = this[_handle]; + if (handle) { + handle.done?.(); + } return this[_reject](x);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
src/js/internal/sql/query.ts(10 hunks)test/js/sql/sql-mysql.transactions.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/js/sql/sql-mysql.transactions.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
test/js/sql/sql-mysql.transactions.test.tssrc/js/internal/sql/query.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/js/sql/sql-mysql.transactions.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/js/sql/sql-mysql.transactions.test.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sql-mysql.transactions.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/sql/sql-mysql.transactions.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/sql/sql-mysql.transactions.test.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/internal/sql/query.ts
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Applied to files:
test/js/sql/sql-mysql.transactions.test.ts
🧬 Code graph analysis (1)
test/js/sql/sql-mysql.transactions.test.ts (1)
src/js/internal-for-testing.ts (1)
SQL(17-17)
🪛 GitHub Check: Lint JavaScript
src/js/internal/sql/query.ts
[failure] 152-152: eslint-plugin-unicorn(no-unnecessary-await)
Unexpected await on a non-Promise value
⏰ 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 (7)
src/js/internal/sql/query.ts (7)
48-63: Lazy handle creation + error flagging looks correctCreating/caching the handle and marking
error | invalidHandleon failure avoids re-entrancy loops inreject(). This is a solid pattern.
100-128: Idempotent run gating and error handling LGTMEarly exits on already-set statuses and
notTaggedguard are correct; synchronous handler exceptions are caught and routed toreject().
246-271: Mode setters (raw()/values()) look goodLazy handle acquisition and
setMode(...)are consistent with the new accessor.
296-304: catch(): mirrors then() policy appropriatelyRunnable path is triggered and the returned promise is marked as handled. Looks good.
313-316: finally(): ensures completion path startsEagerly starting the async path here is consistent with
then()/catch()changes.
221-230: cancel() is no-op pre-run—no adapter changes needed
Thecancel()implementation only callshandle.cancel?.()when theexecutedbit is already set, so cancelling before#run()never invokes the adapter and is safe.
274-293: Verify Promise shim methods Ensure$Promise.prototype.$thenand$markPromiseAsHandledare defined in this runtime before relying on them.
…ries The tests expect that calling .catch() or .finally() on untagged SQL queries should throw an error immediately and synchronously. Previously, the error was only thrown asynchronously within #runAsync(), causing the tests to fail. This fix adds synchronous validation checks in both catch() and finally() methods to throw the appropriate "not tagged call" error before attempting to run the query asynchronously. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/internal/sql/query.ts (1)
130-160: Document the forced async tick at Line 152
Whileawait 1is valid (it’s equivalent toawait Promise.resolve(1)and defers execution to the next microtask), it’s an uncommon pattern that can confuse readers. Replace it with a clearer intent—e.g.await Promise.resolve()—or add an inline comment explaining why you’re forcing the async boundary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/js/internal/sql/query.ts(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
src/js/internal/sql/query.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/internal/sql/query.ts
🧬 Code graph analysis (1)
src/js/internal/sql/query.ts (1)
src/js/bun/sqlite.ts (1)
handle(432-434)
🪛 GitHub Check: Lint JavaScript
src/js/internal/sql/query.ts
[failure] 152-152: eslint-plugin-unicorn(no-unnecessary-await)
Unexpected await on a non-Promise value
⏰ 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 (10)
src/js/internal/sql/query.ts (10)
48-62: Excellent refactor to private method with lazy initialization.The conversion from a previous
getQueryHandle()method to#getQueryHandle()private method with proper lazy initialization and error handling is well-implemented. The method correctly caches the handle and marks the query with appropriate error states when handle creation fails.
100-128: Well-structured synchronous execution path.The
#run()method properly implements the synchronous execution path with comprehensive status checks, proper error handling, and correct state management.
183-194: Handle access correctly updated to use private method.The change from direct handle access to using
this.#getQueryHandle()is consistent with the new lazy initialization pattern.
196-211: Consistent handle access pattern with proper error state checking.The reject method correctly uses the private method and properly checks for
invalidHandlestate before attempting to access the handle.
213-230: Cancel method properly updated with consistent handle access.The cancel method correctly uses
this.#getQueryHandle()and maintains proper state management for cancelled queries.
232-244: Clear separation between sync and async execution paths.The
execute()andrun()methods now properly delegate to their respective private methods, providing a clear distinction between synchronous and asynchronous execution.
246-271: Consistent lazy handle initialization in mode setters.Both
raw()andvalues()methods correctly use the private#getQueryHandle()method for consistent lazy initialization behavior.
295-314: Consistent promise handling in catch method.The
catch()method follows the same pattern asthen()with proper async execution triggering and unhandled rejection prevention.
316-324: Finally method correctly triggers async execution.The
finally()method properly calls#runAsync()to ensure the query completes before finalization, and delegates to the parent class for standard finally behavior.
273-293: Verified: Bun’s internal promise intrinsics ($isPromise, super.$then, $markPromiseAsHandled) are declared in builtins.d.ts and implemented in StreamInternals.ts; no changes required.
The fuzzy-wuzzy test was failing on Windows with "The "callback" argument must be of type function. Received undefined" when calling process.nextTick(). The WebKit upgrade changed JSInternalPromise to JSPromise, causing more promise rejections to trigger global handlers. Under heavy load, the function arguments passed to the C++ queueNextTick functions could become corrupted or invalid. This fix adds validation to ensure the function argument is callable before queuing it to the JavaScript nextTick queue, preventing undefined callbacks from reaching the JavaScript validation layer. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/BunProcess.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,cxx,cc,hpp,hxx,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/BunProcess.cpp
src/bun.js/bindings/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.{cpp,hpp,h}: When implementing JavaScript classes in C++, create Foo, FooPrototype, and FooConstructor for public constructors
Define properties using JSC HashTableValue arrays for JS class bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for bound classes
Files:
src/bun.js/bindings/BunProcess.cpp
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/BunProcess.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/BunProcess.cpp
🧠 Learnings (4)
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/BunProcess.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
src/bun.js/bindings/BunProcess.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/BunProcess.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/BunProcess.cpp
⏰ 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
src/bun.js/bindings/BunProcess.cpp
Outdated
| JSValue function = JSValue::decode(func); | ||
|
|
||
| // Validate that the function is callable before queuing | ||
| if (function && function.isCallable()) { | ||
| process->queueNextTick(globalObject, function, JSValue::decode(arg1)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: AsyncContextFrame callbacks are now silently dropped; use templated queueNextTick and broaden guard
The new isCallable() gate ignores AsyncContextFrame, which Process::queueNextTick explicitly supports. This changes behavior and can break call sites that pass an AsyncContextFrame (especially the -arg overloads). Also, silently no-op’ing on invalid callbacks makes debugging harder.
Apply this to preserve existing semantics and support both functions and AsyncContextFrame:
- JSValue function = JSValue::decode(func);
-
- // Validate that the function is callable before queuing
- if (function && function.isCallable()) {
- process->queueNextTick(globalObject, function, JSValue::decode(arg1));
- }
+ JSValue function = JSValue::decode(func);
+ // Accept JS functions and AsyncContextFrame; otherwise, do nothing (or report error)
+ if (function && (function.isCallable() || function.inherits<AsyncContextFrame>())) {
+ process->queueNextTick<1>(globalObject, function, { JSValue::decode(arg1) });
+ }
+ // Optional: report a TypeError instead of silently ignoring.
+ // e.g., auto err = createNotAFunctionError(globalObject, function);
+ // Bun__reportUnhandledError(globalObject, JSValue::encode(err));Run this to find potential callers passing AsyncContextFrame to nextTick(1-arg) and confirm behavior:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'Bun__Process__queueNextTick1\s*\('
rg -nP -C3 'AsyncContextFrame'Length of output: 41558
Bug: AsyncContextFrame callbacks are silently dropped in Bun__Process__queueNextTick1; broaden guard and use templated overload
The isCallable() gate in this C wrapper ignores AsyncContextFrame, breaking existing call sites. It also silently no-op’s on invalid callbacks, hindering debugging.
- JSValue function = JSValue::decode(func);
-
- // Validate that the function is callable before queuing
- if (function && function.isCallable()) {
- process->queueNextTick(globalObject, function, JSValue::decode(arg1));
- }
+ JSValue function = JSValue::decode(func);
+ // Accept JS functions and AsyncContextFrame; otherwise no-op (or report error)
+ if (function && (function.isCallable() || function.inherits<AsyncContextFrame>())) {
+ process->queueNextTick<1>(globalObject, function, { JSValue::decode(arg1) });
+ }
+ // Optional: report invalid callbacks via Bun__reportUnhandledError📝 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.
| JSValue function = JSValue::decode(func); | |
| // Validate that the function is callable before queuing | |
| if (function && function.isCallable()) { | |
| process->queueNextTick(globalObject, function, JSValue::decode(arg1)); | |
| } | |
| JSValue function = JSValue::decode(func); | |
| // Accept JS functions and AsyncContextFrame; otherwise no-op (or report error) | |
| if (function && (function.isCallable() || function.inherits<AsyncContextFrame>())) { | |
| process->queueNextTick<1>(globalObject, function, { JSValue::decode(arg1) }); | |
| } | |
| // Optional: report invalid callbacks via Bun__reportUnhandledError |
src/bun.js/bindings/BunProcess.cpp
Outdated
| JSValue function = JSValue::decode(func); | ||
|
|
||
| // Validate that the function is callable before queuing | ||
| if (function && function.isCallable()) { | ||
| process->queueNextTick<2>(globalObject, function, { JSValue::decode(arg1), JSValue::decode(arg2) }); | ||
| } |
There was a problem hiding this comment.
Bug: AsyncContextFrame rejected by isCallable() in 2-arg wrapper
Same issue as above: the guard drops AsyncContextFrame even though queueNextTick<2> supports it.
- JSValue function = JSValue::decode(func);
-
- // Validate that the function is callable before queuing
- if (function && function.isCallable()) {
- process->queueNextTick<2>(globalObject, function, { JSValue::decode(arg1), JSValue::decode(arg2) });
- }
+ JSValue function = JSValue::decode(func);
+ if (function && (function.isCallable() || function.inherits<AsyncContextFrame>())) {
+ process->queueNextTick<2>(globalObject, function, { JSValue::decode(arg1), JSValue::decode(arg2) });
+ }
+ // Optional: report a TypeError via Bun__reportUnhandledError(...)📝 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.
| JSValue function = JSValue::decode(func); | |
| // Validate that the function is callable before queuing | |
| if (function && function.isCallable()) { | |
| process->queueNextTick<2>(globalObject, function, { JSValue::decode(arg1), JSValue::decode(arg2) }); | |
| } | |
| JSValue function = JSValue::decode(func); | |
| if (function && (function.isCallable() || function.inherits<AsyncContextFrame>())) { | |
| process->queueNextTick<2>(globalObject, function, { JSValue::decode(arg1), JSValue::decode(arg2) }); | |
| } | |
| // Optional: report a TypeError via Bun__reportUnhandledError(...) |
🤖 Prompt for AI Agents
In src/bun.js/bindings/BunProcess.cpp around lines 3479-3484, the current guard
rejects AsyncContextFrame values because it only checks function.isCallable(),
causing valid AsyncContextFrame instances to be dropped before queueNextTick<2>
(which supports them); update the validation to allow either callable functions
OR AsyncContextFrame values (e.g., change the condition to accept
function.isCallable() || function.isAsyncContextFrame(), or use an equivalent
helper that recognizes both) so that queueNextTick<2> receives and queues
AsyncContextFrame objects correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/web/console/console-log.test.ts (1)
121-154: Stabilize stderr assertion by replacing brittle inline snapshotIn test/js/web/console/console-log.test.ts (around the existing
expect(stderr).toMatchInlineSnapshot), remove the full inline snapshot—its code-frame excerpts and carets vary across builds—and assert only the stable messages:- expect(stderr).toMatchInlineSnapshot(` - "Warning log - warn: console.warn an error - at <file>:NN:NN - at loadAndEvaluateModule (N:NN) - - 52 | console.group("Different logs"); - 53 | console.log("Regular log"); - 54 | console.info("Info log"); - 55 | console.warn("Warning log"); - 56 | console.warn(new Error("console.warn an error")); - 57 | console.error(new Error("console.error an error")); - ^ - error: console.error an error - at <file>:NN:NN - at loadAndEvaluateModule (N:NN) - - 53 | console.log("Regular log"); - 54 | console.info("Info log"); - 55 | console.warn("Warning log"); - 56 | console.warn(new Error("console.warn an error")); - 57 | console.error(new Error("console.error an error")); - 58 | console.error(new NamedError("console.error a named error")); - ^ - NamedError: console.error a named error - at <file>:NN:NN - at loadAndEvaluateModule (N:NN) - - NamedError: console.warn a named error - at <file>:NN:NN - at loadAndEvaluateModule (N:NN) - - Error log" - `); + // Assert only stable lines to avoid platform-specific code-frame noise. + expect(stderr).toContain("Warning log"); + expect(stderr).toContain("warn: console.warn an error"); + expect(stderr).toContain("error: console.error an error"); + expect(stderr).toContain("NamedError: console.error a named error"); + expect(stderr).toContain("NamedError: console.warn a named error");
🧹 Nitpick comments (4)
test/js/web/console/console-log.test.ts (4)
6-13: Use await using with Bun.spawn for reliable cleanup on failure paths.If the test fails before awaiting exited, the subprocess can leak. using ensures disposal.
Apply this diff:
-import { file, spawn } from "bun"; +import { file } from "bun"; @@ -it("should log to console correctly", async () => { - const { stdout, stderr, exited } = spawn({ - cmd: [bunExe(), join(import.meta.dir, "console-log.js")], +it("should log to console correctly", async () => { + const consoleLogPath = join(import.meta.dir, "console-log.js"); + await using proc = Bun.spawn({ + cmd: [bunExe(), consoleLogPath], stdin: "inherit", stdout: "pipe", stderr: "pipe", env: bunEnv, }); - const exitCode = await exited; + const exitCode = await proc.exited;
15-20: Normalize path separators in outputs to reduce cross-platform diffs.stdout/expected do not normalize “\” vs “/”. Mirror the stderr normalization used elsewhere in this file.
- const out = (await stdout.text()).replaceAll("\r\n", "\n"); - const expected = (await new Response(file(join(import.meta.dir, "console-log.expected.txt"))).text()).replaceAll( - "\r\n", - "\n", - ); + const out = (await proc.stdout.text()).replaceAll("\r\n", "\n").replaceAll("\\", "/"); + const expected = ( + await new Response(file(join(import.meta.dir, "console-log.expected.txt"))).text() + ) + .replaceAll("\r\n", "\n") + .replaceAll("\\", "/");Note: If you don’t adopt the previous comment’s Bun.spawn change, replace proc.stdout with stdout in this hunk.
40-58: Prefer inline snapshot over manual string assembly for readability and maintenance.Guidelines recommend snapshots; switch to toMatchInlineSnapshot for stdout.
- expect(proc.stdout.toString("utf8")).toEqual( - "[\n" + - " 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,\n" + - " 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,\n" + - " 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,\n" + - " 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,\n" + - " ... 900 more items\n" + - "]\n" + - "", - ); + expect(proc.stdout.toString("utf8")).toMatchInlineSnapshot(` + "[ + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + ... 900 more items + ] + " + `);
60-83: Leverage normalizeBunSnapshot from harness instead of bespoke normalization.Centralizing normalization reduces maintenance and makes future stack-format changes less painful.
-import { bunEnv, bunExe } from "harness"; +import { bunEnv, bunExe, normalizeBunSnapshot } from "harness"; @@ - let stdout = proc.stdout - .toString("utf8") - .replaceAll("\r\n", "\n") - .replaceAll("\\", "/") - .trim() - .replaceAll(filepath, "<file>"); - let stderr = proc.stderr - .toString("utf8") - .replaceAll("\r\n", "\n") - .replaceAll("\\", "/") - .trim() - .replaceAll(filepath, "<file>") - // Normalize line numbers for consistency between debug and release builds - .replace(/\(\d+:\d+\)/g, "(N:NN)") - .replace(/<file>:\d+:\d+/g, "<file>:NN:NN"); + let stdout = normalizeBunSnapshot(proc.stdout.toString("utf8")) + .replaceAll(filepath, "<file>") + .trim(); + let stderr = normalizeBunSnapshot(proc.stderr.toString("utf8")) + .replaceAll(filepath, "<file>") + .trim();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/js/bun/util/fuzzy-wuzzy.test.ts(4 hunks)test/js/web/console/console-log.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/js/bun/util/fuzzy-wuzzy.test.ts
🧰 Additional context used
📓 Path-based instructions (9)
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/js/web/console/console-log.test.ts
test/js/web/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Put Web API tests under test/js/web/
Files:
test/js/web/console/console-log.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
test/js/web/console/console-log.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/js/web/console/console-log.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/js/web/console/console-log.test.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/web/console/console-log.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/web/console/console-log.test.ts
test/js/web/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Web API tests under test/js/web/, separated by category
Files:
test/js/web/console/console-log.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/web/console/console-log.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/js/web/console/console-log.test.ts
⏰ 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 (1)
test/js/web/console/console-log.test.ts (1)
157-181: LGTM: SharedArrayBuffer logging behavior is clearly asserted and normalized.No portability concerns; expectations are concise and stable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/sql/query.ts (1)
316-324: finally(): add same runPromise catch to avoid unhandled rejectionsMirror the safety used in then()/catch().
finally(_onfinally?: (() => void) | undefined | null) { if (this[_flags] & SQLQueryFlags.notTagged) { throw this[_adapter].notTaggedCallError(); } - this.#runAsync(); + const runPromise = this.#runAsync(); + if ($isPromise(runPromise) && runPromise !== this) { + runPromise.catch(() => {}); + } return super.finally.$apply(this, arguments); }
🧹 Nitpick comments (3)
src/js/internal/sql/query.ts (3)
48-62: Short-circuit on invalidHandle and normalize errors to ErrorPrevents repeated handle creation + duplicate rejections after an earlier failure; ensures
reject()always receives anError.- #getQueryHandle() { + #getQueryHandle() { + if ((this[_queryStatus] & SQLQueryStatus.invalidHandle) !== 0) { + return null; + } let handle = this[_handle]; if (!handle) { try { const [sql, values] = this[_adapter].normalizeQuery(this[_strings], this[_values]); this[_handle] = handle = this[_adapter].createQueryHandle(sql, values, this[_flags]); } catch (err) { this[_queryStatus] |= SQLQueryStatus.error | SQLQueryStatus.invalidHandle; - this.reject(err as Error); + this.reject(err instanceof Error ? err : new Error(String(err))); } } return handle; }
100-129: Return type clarity for #run()
#run()is used for side effects; consider not returningthis/handler result to avoid implying meaningful return values.
130-160: Await handler’s Promise to avoid side-channel rejectionsIf
handler()returns a Promise that rejects,#runAsync()can propagate a rejection separate from the query’s own rejection. Await it and funnel errors throughthis.reject().async #runAsync() { const { [_handler]: handler, [_queryStatus]: status } = this; @@ await Promise.$resolve(); - try { - return handler(this, handle); - } catch (err) { - this[_queryStatus] |= SQLQueryStatus.error; - this.reject(err as Error); - } + try { + const out = handler(this, handle); + if ($isPromise(out)) { + try { + await out; + } catch (err) { + this[_queryStatus] |= SQLQueryStatus.error; + this.reject(err instanceof Error ? err : new Error(String(err))); + } + return; + } + return out; + } catch (err) { + this[_queryStatus] |= SQLQueryStatus.error; + this.reject(err instanceof Error ? err : new Error(String(err))); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/js/internal/sql/query.ts(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
src/js/internal/sql/query.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/internal/sql/query.ts
🔇 Additional comments (9)
src/js/internal/sql/query.ts (9)
183-194: Resolve path looks goodCalls
done()on the handle when present; state flags are updated correctly.
196-211: Good guard against invalidHandle in reject()Skips handle access when the handle creation failed earlier.
213-230: Cancel path is safeOnly cancels when executed; null-safe on handle.
233-235: execute() correctly triggers the sync pathMatches intent to start execution without awaiting.
247-255: raw(): lazy handle + mode set LGTM
263-271: values(): lazy handle + mode set LGTM
300-308: catch(): consistent with then() (swallows runPromise rejections)
238-244: run() now async—confirm callers handle returned promise
Ensure every invocation ofSQLQuery.run()usesawaitor otherwise handles the returned Promise so errors are propagated correctly.
274-291: Confirm$applyalias availability
I seePromise.prototype.$thenis primordials-aliased, but there’s no$applyalias forFunction.prototype.applyinsrc/js/internal/primordials.js. Ensure this file imports a$applybinding (or switch to.apply) so that callingsuper.$then.$apply(...)won’t break and stays consistent with the.catch/.finallyusage below.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/js/internal/sql/query.ts (7)
48-62: Avoid recursive/double rejection from #getQueryHandleCalling this.reject() here can re-enter reject(), which then calls #getQueryHandle() again on some paths. Prefer direct resolver to avoid nested control flow and double-settlement risk.
} catch (err) { this[_queryStatus] |= SQLQueryStatus.error | SQLQueryStatus.invalidHandle; - this.reject(err as Error); + // Avoid re-entering reject(); invalidHandle ensures no handle cleanup is needed. + this[_reject](err as Error); }
100-129: Set “executed” after successfully obtaining a handleMarking executed before creating the handle can leave queries “executed” even when handle creation fails. Flip the order.
- this[_queryStatus] |= SQLQueryStatus.executed; - const handle = this.#getQueryHandle(); - - if (!handle) { - return this; - } + const handle = this.#getQueryHandle(); + if (!handle) return this; + this[_queryStatus] |= SQLQueryStatus.executed;
130-160: Mirror handle/flag ordering in async pathSame rationale as sync path: obtain handle first, then mark executed.
- this[_queryStatus] |= SQLQueryStatus.executed; - const handle = this.#getQueryHandle(); - - if (!handle) { - return this; - } + const handle = this.#getQueryHandle(); + if (!handle) return this; + this[_queryStatus] |= SQLQueryStatus.executed;
183-195: Skip handle lookup in resolve() if never executedNo need to materialize a handle just to call done() when the query wasn’t executed.
resolve(x: T) { this[_queryStatus] &= ~SQLQueryStatus.active; - const handle = this.#getQueryHandle(); + if ((this[_queryStatus] & SQLQueryStatus.executed) === 0) { + return this[_resolve](x); + } + const handle = this.#getQueryHandle();
196-211: Guard handle cleanup on “executed” and avoid creating a handle during rejectionPrevents unnecessary handle creation and aligns cleanup with actual execution state. Combined with the #getQueryHandle() change, this also avoids re-entrant reject loops on creation errors.
reject(x: Error) { this[_queryStatus] &= ~SQLQueryStatus.active; this[_queryStatus] |= SQLQueryStatus.error; - if (!(this[_queryStatus] & SQLQueryStatus.invalidHandle)) { - const handle = this.#getQueryHandle(); + const status = this[_queryStatus]; + if (!(status & SQLQueryStatus.invalidHandle) && (status & SQLQueryStatus.executed)) { + const handle = this.#getQueryHandle(); if (!handle) { return this[_reject](x); } handle.done?.(); }
298-309: Align notTagged handling across then/catch/finallycatch() throws synchronously for notTagged while then()/finally() rely on #runAsync to reject. Prefer one consistent approach.
Option A (make catch consistent with then/finally):
- catch() { - if (this[_flags] & SQLQueryFlags.notTagged) { - throw this[_adapter].notTaggedCallError(); - } - this.#runAsyncAndCatch(); + override catch<TResult = never>( + onrejected?: ((reason: any) => TResult | PromiseLike<TResult>) | null, + ): Promise<T | TResult> { + this.#runAsyncAndCatch(); const result = super.catch.$apply(this, arguments); $markPromiseAsHandled(result); return result; }Option B (pre-check everywhere): add the same pre-check to then()/finally().
311-320: Add explicit override signature; consider consistency with notTaggedMatch Promise’s signature and choose a consistent notTagged strategy (see previous comment).
- finally(_onfinally?: (() => void) | undefined | null) { + override finally(_onfinally?: (() => void) | null) { if (this[_flags] & SQLQueryFlags.notTagged) { throw this[_adapter].notTaggedCallError(); } this.#runAsyncAndCatch(); return super.finally.$apply(this, arguments); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/js/internal/sql/query.ts(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
src/js/internal/sql/query.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/internal/sql/query.ts
🧬 Code graph analysis (1)
src/js/internal/sql/query.ts (1)
src/js/bun/sqlite.ts (1)
handle(432-434)
🪛 Biome (2.1.2)
src/js/internal/sql/query.ts
[error] 284-284: Do not add then to a class.
(lint/suspicious/noThenProperty)
🔇 Additional comments (6)
src/js/internal/sql/query.ts (6)
221-227: LGTM: cancel() delegates to handle.cancel() only after executionConditional cancel is correct and won’t spuriously create a handle.
232-235: LGTM: execute() drives sync path and remains chainable
237-244: LGTM: run() awaits async path and remains chainable
246-255: LGTM: raw() uses lazy handle and mode switch
262-271: LGTM: values() uses lazy handle and mode switch
273-283: LGTM: #runAsyncAndCatch prevents unhandled rejection warningsThe guard with $isPromise and identity check looks correct.
| then() { | ||
| this.#runAsyncAndCatch(); | ||
|
|
||
| const result = super.$then.$apply(this, arguments); | ||
| $markPromiseAsHandled(result); | ||
|
|
||
| // Only mark as handled if there's a rejection handler | ||
| const hasRejectionHandler = arguments.length >= 2 && arguments[1] != null; | ||
| if (hasRejectionHandler) { | ||
| $markPromiseAsHandled(result); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix Biome error: “Do not add then to a class” and tighten types
Biome flags this as an error; CI will fail. Since Query extends Promise intentionally, add a targeted ignore and declare an explicit override signature to satisfy TS/lint while keeping runtime behavior.
- then() {
+ /* biome-ignore lint/suspicious/noThenProperty: Query extends Promise; overriding then is intentional to trigger execution on chaining. */
+ override then<TResult1 = T, TResult2 = never>(
+ onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | null,
+ onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null,
+ ): Promise<TResult1 | TResult2> {
this.#runAsyncAndCatch();
- const result = super.$then.$apply(this, arguments);
+ const result = super.$then.$apply(this, arguments);
- // Only mark as handled if there's a rejection handler
- const hasRejectionHandler = arguments.length >= 2 && arguments[1] != null;
+ // Only mark as handled if there's a rejection handler
+ const hasRejectionHandler = arguments.length >= 2 && arguments[1] != null;
if (hasRejectionHandler) {
$markPromiseAsHandled(result);
}
return result;
}Optionally add explicit override signatures for catch/finally too (see below comments).
🏁 Script executed:
#!/bin/bash
set -e
# Locate the Query class and verify it extends Promise
rg -nP 'class\s+Query\b' -C3 src/js/internal/sql/query.ts || true
# Dump the then() implementation around lines 280–320
sed -n '280,320p' src/js/internal/sql/query.ts || true
# Search for where $then is assigned or referenced in this file
rg -n '\$then' -C3 src/js/internal/sql/query.ts || trueLength of output: 1551
Suppress Biome noThenProperty and add explicit override then signature
In src/js/internal/sql/query.ts:284, prepend
/* biome-ignore lint/suspicious/noThenProperty: overriding then is intentional to trigger query execution on chaining */
override then<TResult1 = T, TResult2 = never>(
onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | null,
onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null,
): Promise<TResult1 | TResult2> { … }Optionally apply the same pattern (override + generics + targeted ignore) to the catch and finally methods.
🧰 Tools
🪛 Biome (2.1.2)
[error] 284-284: Do not add then to a class.
(lint/suspicious/noThenProperty)
🤖 Prompt for AI Agents
In src/js/internal/sql/query.ts around lines 284 to 296, the class currently
defines a plain then() property which triggers a Biome lint warning and lacks an
explicit override/typing; change it to an explicit overridden then signature
with generics and proper Promise return type and add the suggested Biome ignore
comment immediately above it: add "/* biome-ignore
lint/suspicious/noThenProperty: overriding then is intentional to trigger query
execution on chaining */" and replace the plain then() with "override
then<TResult1 = T, TResult2 = never>(onfulfilled?: ((value: T) => TResult1 |
PromiseLike<TResult1>) | null, onrejected?: ((reason: any) => TResult2 |
PromiseLike<TResult2>) | null): Promise<TResult1 | TResult2> { … }" keeping the
existing implementation body; optionally apply the same pattern (biome-ignore +
override + generics) to catch and finally methods if present.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/js/internal/streams/from.ts (1)
56-60: Good guard on cb; micro cleanups for no-op allocation and nullish coalescingKeep the guards; consider hoisting a shared no-op and using
??to avoid allocating closures each call and treating falsy-but-defined errors correctly.+const kNoop = () => {}; readable._destroy = function (error, cb) { PromisePrototypeThen.$call( close(error), - $isCallable(cb) ? () => process.nextTick(cb, error) : () => {}, // nextTick is here in case cb throws - $isCallable(cb) ? e => process.nextTick(cb, e || error) : () => {}, + $isCallable(cb) ? () => process.nextTick(cb, error) : kNoop, // nextTick is here in case cb throws + $isCallable(cb) ? e => process.nextTick(cb, e ?? error) : kNoop, ); };test/js/bun/spawn/spawn-pipe-leak.test.ts (10)
18-20: Disable noisy debug logs by default; opt-in via envDefaulting logs to on will bloat CI output and slow runs.
Apply:
- const DEBUG_LOGS = true; // turn this on to see debug logs + const DEBUG_LOGS = process.env.DEBUG_SPAWN_PIPE_LEAK === "1"; // opt-in for local debugging/CI
4-6: Fix stale docstring (32 KB → 8 MB)Comment doesn’t match the buffer size used below.
- * This test spawns processes that write about 32 KB of data to stdout + * This test spawns processes that write ~8 MB of data to stdout
23-31: Comment says ~512 KB but code writes 8 MBAlign the comment with the behavior (or reduce the allocation if that was intended).
-// Create a command that will generate ~512 KB of output +// Create a command that will generate ~8 MB of output
42-51: Useawait usingand avoid shadowing globalprocess; assert exit codeEnsures handle cleanup, avoids name shadowing, and checks for silent failures.
- async function readPipeAfterExit() { - const process = Bun.spawn({ + async function readPipeAfterExit() { + await using proc = Bun.spawn({ cmd, stdout: "pipe", stderr: "ignore", stdin: "ignore", }); - await process.exited; - await process.stdout.blob(); + await proc.exited; + await proc.stdout.blob(); + expect(proc.exitCode).toBe(0); }
53-61: Same: preferawait using, noprocessshadowing, and assert exit code- async function dontRead() { - const process = Bun.spawn({ + async function dontRead() { + await using proc = Bun.spawn({ cmd: cmd10, stdout: "pipe", stderr: "ignore", stdin: "ignore", }); - await process.exited; + await proc.exited; + expect(proc.exitCode).toBe(0); }
63-72: Same: preferawait using, avoid shadowing, assert exit code- async function readPipeBeforeExit() { - const process = Bun.spawn({ + async function readPipeBeforeExit() { + await using proc = Bun.spawn({ cmd, stdout: "pipe", stderr: "ignore", stdin: "ignore", }); - await process.stdout.blob(); - await process.exited; + await proc.stdout.blob(); + await proc.exited; + expect(proc.exitCode).toBe(0); }
125-127: Remove explicit test timeout (use Bun’s default timeouts per guidelines)- test("'pipe' stdout if read after exit should not leak memory", async () => { - await run(readPipeAfterExit); - }, 30_000); + test("'pipe' stdout if read after exit should not leak memory", async () => { + await run(readPipeAfterExit); + });
129-131: Same: drop explicit timeout- test("'pipe' stdout if not read should not leak memory", async () => { - await run(dontRead); - }, 30_000); + test("'pipe' stdout if not read should not leak memory", async () => { + await run(dontRead); + });
133-139: Same: drop explicit timeout on the Windows-gated testtest.todoIf(isWindows)( "'pipe' stdout if read before exit should not leak memory", async () => { await run(readPipeBeforeExit); }, - 30_000, );
10-17: Use skipIf instead of todoIf for ASAN CI
Replacedescribe.todoIf(isASAN && isCI)withdescribe.skipIf(isASAN && isCI)to skip this test on ASAN CI rather than marking it as TODO and losing test visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/js/internal/streams/from.ts(2 hunks)test/js/bun/spawn/spawn-pipe-leak.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
test/js/bun/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Put Bun-specific API tests under test/js/bun/
Files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
test/js/bun/spawn/spawn-pipe-leak.test.tssrc/js/internal/streams/from.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/spawn/spawn-pipe-leak.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/bun/spawn/spawn-pipe-leak.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/spawn/spawn-pipe-leak.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/bun/spawn/spawn-pipe-leak.test.ts
src/js/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)
Files:
src/js/internal/streams/from.ts
🧠 Learnings (8)
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : When spawning Bun processes, use bunExe() and bunEnv from harness
Applied to files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
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/bun/spawn/spawn-pipe-leak.test.ts
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Applied to files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Applied to files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
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/js/bun/spawn/spawn-pipe-leak.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/js/bun/spawn/spawn-pipe-leak.test.ts
🧬 Code graph analysis (2)
test/js/bun/spawn/spawn-pipe-leak.test.ts (1)
test/harness.ts (2)
isASAN(48-48)isCI(28-28)
src/js/internal/streams/from.ts (1)
src/js/internal/primordials.js (1)
PromisePrototypeThen(86-86)
🔇 Additional comments (1)
test/js/bun/spawn/spawn-pipe-leak.test.ts (1)
8-8: Harness imports LGTMisASAN and isCI exist in the harness; import set is correct and used below.
| const SymbolIterator = Symbol.iterator; | ||
| const SymbolAsyncIterator = Symbol.asyncIterator; | ||
| const PromisePrototypeThen = Promise.prototype.then; | ||
| const PromisePrototypeThen = Promise.prototype.$then; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bind PromisePrototypeThen from primordials; current binding may lack .$call and break destroy path
Grabbing Promise.prototype.$then off the global can be undefined or not carry the internal .$call brand. This will break PromisePrototypeThen.$call(...) in _destroy. Import the primordials binding instead.
Apply:
+import { PromisePrototypeThen } from "../primordials";
-
-const PromisePrototypeThen = Promise.prototype.$then;Verify the export exists and match usage elsewhere:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'export\s+(const|{[^}]*PromisePrototypeThen[^}]*})' src/js/internal/primordials.* || true
rg -nP -C2 'PromisePrototypeThen\.\$call' src/js | sed -n '1,120p'Length of output: 6980
Add missing export for PromisePrototypeThen
src/js/internal/primordials.js doesn’t export PromisePrototypeThen — importing it in from.ts will fail. Add an export (e.g.
export const PromisePrototypeThen = uncurryThis(Promise.prototype.then);or the equivalent existing binding) to primordials.js before replacing the local binding.
🤖 Prompt for AI Agents
In src/js/internal/streams/from.ts around line 3, the file references
PromisePrototypeThen from primordials but primordials.js doesn't export it; add
an exported binding in src/js/internal/primordials.js such as exporting an
uncurryThis of Promise.prototype.then (e.g. export const PromisePrototypeThen =
uncurryThis(Promise.prototype.then)); then update from.ts to import and use that
exported PromisePrototypeThen (remove or replace the local const declaration) so
the imported symbol resolves correctly.
What does this PR do?
How did you verify your code works?