feat(tls): add https.globalAgent.options fallback for fetch, node:https, and WebSocket #25910
+2,085
−77
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Updated 11:44 AM PT - Jan 9th, 2026
❌ @cirospaciari, your commit 3960596 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 25910That installs a local version of the PR into your bun-25910 --bun |
cirospaciari
changed the title
cirospaciari
changed the title
Contributor
|
@cirospaciari: I'll review the changes in this PR. I notice there's a build failure on macOS aarch64 (build-zig) that you'll want to address - you can check the details in the Build #34298 logs. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
|
@cirospaciari: Understood! I'll proceed with reviewing the code changes and ignore the stale build failure. Let me review the current state of the PR. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
WalkthroughThe PR adds support for falling back to Node.js https.globalAgent for TLS settings (rejectUnauthorized, CA, certificates) and proxy configuration in Bun's fetch, WebSocket, and HTTPS request implementations. New bindings expose globalAgent, helper functions extract TLS/proxy options from agents, and Undici classes are updated to store options for compatibility. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.test.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
🧠 Learnings (21)📓 Common learnings📚 Learning: 2025-10-19T04:55:33.099ZApplied to files:
📚 Learning: 2025-10-18T05:23:24.403ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-09-03T01:30:58.001ZApplied to files:
📚 Learning: 2025-09-20T00:57:56.685ZApplied to files:
📚 Learning: 2025-09-20T00:58:38.042ZApplied to files:
📚 Learning: 2025-10-20T01:38:02.660ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2025-10-19T02:44:46.354ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2025-10-25T17:20:19.041ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-10-30T03:48:10.513ZApplied to files:
📚 Learning: 2025-10-08T13:48:02.430ZApplied to files:
🧬 Code graph analysis (3)test/js/node/tls/globalAgent-fetch.test.ts (1)
test/js/node/tls/globalAgent-undici.test.ts (1)
test/js/node/tls/globalAgent-https-request.test.ts (1)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/bun.js/webcore/fetch.zig:
- Around line 952-996: The TLS extraction block duplicates logic already
encapsulated by getAgentTLSOptions(); replace the manual connectOpts/connect
probing in the current_agent branch with a call to
getAgentTLSOptions(current_agent, vm, globalThis) (or the same signature used
earlier), then apply the returned values to reject_unauthorized,
check_server_identity, and ssl_config (allocating SSLConfig on success) and
preserve the existing error path (set is_error and return .zero on failure),
ensuring you reference current_agent, getAgentTLSOptions(), reject_unauthorized,
check_server_identity, and ssl_config so the behavior matches the earlier TLS
fallback block.
In @src/js/node/_http_client.ts:
- Around line 733-841: The TLS option fallback chain (agent.options →
agent.connectOpts → agent.connect) is duplicated across many blocks; extract a
helper like getAgentTlsOption(agent, key) and use it in each branch (e.g., when
resolving agentRejectUnauthorized, agentCa, agentCert, agentKey,
agentPassphrase, agentCiphers, agentServername, agentSecureOptions) so the
repeated three-step lookup is centralized, then keep the existing validations
(isValidTLSArray, typeof checks) and assign via this._ensureTls() as before;
update places referencing the inline fallback logic to call the new helper to
reduce duplication.
In @src/js/thirdparty/undici.js:
- Around line 340-344: The RetryAgent constructor currently ignores the
dispatcher parameter which should be the wrapped agent; update the constructor
in class RetryAgent to store the passed-in dispatcher (e.g., assign it to
this.dispatcher) so future methods can delegate to it, and optionally validate
its type (instanceof Dispatcher) or add a comment/docstring noting it's
intentionally stubbed if you don't want to use it yet; ensure references to
dispatcher in other RetryAgent methods use this.dispatcher.
- Around line 274-326: Dispatcher and DispatcherBase duplicate the same
constructor logic for storing options and options.connect; remove the
duplication by extracting that logic into a single reusable place—either create
a small helper function (e.g., storeConnectOptions(options)) and call it from
both Dispatcher and DispatcherBase constructors, or make one class the true base
(e.g., have Dispatcher extend DispatcherBase or vice versa) and keep the
constructor logic only in the base; update the constructors of Dispatcher,
DispatcherBase (and any subclasses like Agent, Pool, Client, BalancedPool if
needed) to call the shared helper or the single base constructor so the
duplicated block is eliminated.
In @test/js/node/tls/globalAgent-tls-options.test.ts:
- Around line 1121-1242: The property-storage assertions for undici classes
(Agent, Dispatcher, Pool, Client, ProxyAgent, EnvHttpProxyAgent, RetryAgent)
belong to a structural/unit test, not the TLS integration suite; move the test
"undici.Agent stores options and connect properties correctly" into its own
describe block or a separate test file (e.g., undici-agent-properties.test.ts),
keeping the same assertions and process spawning logic but removing it from the
TLS/globalAgent-tls-options.test.ts integration file so integration vs.
structural tests are clearly separated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/webcore/fetch.zigsrc/js/node/_http_client.tssrc/js/thirdparty/undici.jstest/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/webcore/fetch.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/webcore/fetch.zig
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/node/_http_client.tssrc/js/thirdparty/undici.js
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/node/_http_client.ts
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/bindings.cpp
🧠 Learnings (36)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/js/thirdparty/undici.jssrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/js/thirdparty/undici.jssrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use private globals and methods with `$` prefix (e.g., `$Array`, `map.$set()`) instead of public JavaScript globals
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/first_party/undici/undici.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
src/bun.js/webcore/fetch.zigsrc/js/thirdparty/undici.jstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Create test functions in test/v8/v8-module/main.cpp that take FunctionCallbackInfo<Value> parameter, use the test V8 API, print results for comparison with Node.js, and return Undefined
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
🧬 Code graph analysis (2)
src/js/node/_http_client.ts (1)
src/js/internal/tls.ts (1)
isValidTLSArray(53-53)
test/js/node/tls/globalAgent-tls-options.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (10)
src/bun.js/webcore/fetch.zig (3)
176-198: Asymmetric TLS property checking betweenoptionsandconnectOpts/connect.For
agent.options, the code checkshasTLSProperties()before returning it (line 180-181), but forconnectOptsandconnect, it returns them if they're just objects without verifying they contain TLS properties.This asymmetry seems intentional (options may contain non-TLS data while connectOpts/connect are TLS-specific by convention), but it could lead to unexpected behavior if someone passes an empty object as
connectOpts.Is this asymmetry intentional? If
connectOptsorconnectis an empty object{}, it will be returned and potentially passed toSSLConfig.fromJS(), which should handle it gracefully. Please confirm this is the expected behavior.
523-606: TLS fallback logic is well-structured.The fallback chain correctly prioritizes:
- Per-request
agentoption (node-fetch compatibility)- Per-request
dispatcheroption (undici compatibility)https.globalAgentThe extraction of
rejectUnauthorized,checkServerIdentity, andSSLConfigfrom the found options object follows the same pattern as the per-request TLS handling, maintaining consistency.
159-173: ThehasException()check is redundant.The
try obj.get(globalThis, prop)already propagates any exceptions via the error union. SinceisUndefinedOrNull()is a pure function that only reads the enum value and cannot throw, the explicithasException()check on lines 168-170 will never catch anything the try block didn't already handle. This pattern is also inconsistent with similar code in the codebase (e.g.,parse_args.zig,SocketAddress.zig), which rely on try's error propagation without explicithasException()checks. Remove the redundant check.src/js/thirdparty/undici.js (2)
12-18: Fetch wrapper correctly uses globalDispatcher fallback.The wrapper properly checks if a globalDispatcher is set and no per-request dispatcher/agent is provided before injecting the globalDispatcher. The spread operator preserves other init options correctly.
One minor note: Per the coding guidelines, consider using
$isCallable()or similar intrinsics if available, though for this context the simple property check is sufficient.
462-471: Global dispatcher management looks correct.The lazy initialization pattern in
getGlobalDispatcher()(line 470) is appropriate for the JavaScript single-threaded context. TheglobalDispatcherbeing a module-level variable is consistent with undici's API.src/bun.js/bindings/bindings.cpp (1)
6124-6144: HTTPS globalAgent binding correctly wraps internal registryThe binding cleanly pulls
node:httpsfrom the internal module registry, returnshttps.globalAgentwhen present, and safely falls back toundefinedon missing module, non-object exports, or JS exceptions viaRETURN_IF_EXCEPTION. This matches the Zig wrapper’s contract and fits existing error-handling patterns in this file.src/bun.js/bindings/JSGlobalObject.zig (1)
803-808: JSGlobalObject wrapper for https.globalAgent is minimal and consistentThe new extern and
getHttpsGlobalAgentwrapper are correctly wired to the C++ binding, have the expected non-throwingJSValuesignature, and the doc comment matches the C++ behavior of returningundefinedwhen the module or property is unavailable.test/js/first_party/undici/undici.test.ts (1)
159-282: Tests correctly verify Bun's simplified undici dispatcher TLS configuration storageThe tests appropriately verify that TLS options passed via the
connectproperty in dispatcher options are stored and accessible on the instance. This is correct for Bun's implementation.The original concern about shadowing an upstream undici
.connect()method does not apply here: Bun's undici.js is a minimal shim that stores TLS configuration without implementing the full upstream undici API. Bun does not expose a.connect()method on these dispatcher classes, so there is no conflict. The tests validate exactly what Bun intends: that connect options are stored on the instance for use by Bun's native fetch implementation.Likely an incorrect or invalid review comment.
test/js/node/tls/globalAgent-tls-options.test.ts (2)
1-12: LGTM! Clean test setup.The imports and fixture loading are well-organized. Loading certificates at module level is efficient and appropriate for these integration tests.
13-1244: Comprehensive test coverage with proper isolation.The test suite effectively validates TLS option fallback behavior across https.request, fetch, and undici integration. Test patterns are correct:
- Uses
port: 0for dynamic port allocation ✓- Uses
tempDirfrom harness withusingfor automatic cleanup ✓- Uses
await usingwithBun.spawnfor proper process management ✓- No flaky patterns (setTimeout, hardcoded delays) ✓
- Proper test isolation with concurrent execution ✓
Contributor
|
@cirospaciari: Understood! I'll proceed with reviewing the code changes and ignore the stale build failure. Let me review the current state of the PR. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/bun.js/webcore/fetch.zig:
- Around line 176-198: getAgentTLSOptions currently only verifies TLS properties
via hasTLSProperties for agent.options but returns agent.connectOpts and
agent.connect if they are objects without checking for TLS fields; update
getAgentTLSOptions to call hasTLSProperties(globalThis, connect_opts) and
hasTLSProperties(globalThis, connect) (using the same pattern as for opts)
before returning them so only objects with TLS properties are accepted; keep the
existing null return when no TLS-bearing object is found.
- Around line 952-981: Duplicate TLS extraction code should be moved into a
shared helper: implement a function (e.g., extractAgentTLSOptions or
getTLSSettingsFromAgent) that accepts the VM, globalThis, the connect_opts value
(from getAgentTLSOptions), and references/outputs for reject_unauthorized,
check_server_identity, and ssl_config; inside it perform the current logic that
reads "rejectUnauthorized", "checkServerIdentity", and calls SSLConfig.fromJS
(preserving the existing error handling behavior and OOM allocation pattern),
then replace the duplicated blocks (the block using getAgentTLSOptions in both
the main TLS fallback and the proxy-from-agent branch) with calls to this helper
to update those three variables.
In @src/js/node/_http_client.ts:
- Around line 752-760: The error text is misleading because
getAgentTlsOption(agent, "ca") may return a value from agent.options.ca,
agent.connectOpts.ca, or agent.connect.ca; update the throw in the block that
handles agentCa (and analogous blocks for cert, key, passphrase, ciphers,
servername, secureOptions) to use a generic or source-aware message: detect the
source via getAgentTlsOption or include the option name and agent property path
(e.g., "agent.{options|connectOpts|connect}.ca") in the message; locate the
checks around getAgentTlsOption(agent, "ca") and isValidTLSArray and replace the
hardcoded "agent.options.ca must be..." text with a descriptive message that
references getAgentTlsOption, agentCa, and this._ensureTls().ca so all similar
TLS option validations are updated consistently.
In @src/js/thirdparty/undici.js:
- Around line 294-311: Constructors currently accept but do not assign their
inputs; update Pool, Client, and BalancedPool constructors to store the given
parameters on the instance so downstream code can read them (set this.origin =
url inside Pool's constructor to support pool.origin, set this.url = url inside
Client's constructor to support client.url, and set this.upstreams = upstreams
inside BalancedPool's constructor to support balancedPool.upstreams), keeping
the existing super(options) call and preserving any existing behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/bun.js/webcore/fetch.zigsrc/js/node/_http_client.tssrc/js/thirdparty/undici.jstest/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/node/_http_client.tssrc/js/thirdparty/undici.js
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/node/_http_client.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/fetch.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zig
🧠 Learnings (29)
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tssrc/js/thirdparty/undici.jssrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tssrc/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zig
🧬 Code graph analysis (3)
src/js/node/_http_client.ts (1)
src/js/internal/tls.ts (1)
isValidTLSArray(53-53)
test/js/node/tls/globalAgent-tls-options.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
test/js/first_party/undici/undici-properties.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (14)
src/js/node/_http_client.ts (2)
63-70: LGTM! Clean helper function for TLS option fallback chain.The
getAgentTlsOptionhelper correctly implements the fallback chain (agent.options → agent.connectOpts → agent.connect) as documented. This centralizes the logic and improves maintainability.
740-833: Approve the TLS fallback implementation with a minor observation.The fallback logic is correctly structured: user-provided options take precedence, then agent-derived options are applied. Type validation is consistently applied to both sources. The pattern is clean and follows best practices for Node.js compatibility.
One observation: the blocks for each TLS option share a similar structure. If more options need to be added in the future, consider extracting this into a helper to reduce duplication.
src/bun.js/webcore/fetch.zig (3)
159-173: Approve TLS property detection helper.The
hasTLSPropertiesfunction correctly checks for the presence of TLS-related properties. Usinginline foris appropriate here for compile-time unrolling. The early return on finding any property is efficient.
523-606: Approve TLS fallback implementation with minor observation.The fallback logic correctly prioritizes per-request agent/dispatcher over globalAgent, and properly extracts
rejectUnauthorized,checkServerIdentity, and SSL config. Exception handling after each JS property access is thorough.
860-906: Approve proxy fallback implementation.The proxy fallback logic correctly checks for per-request agent/dispatcher proxy before falling back to globalAgent.proxy. The tuple return of
agent_and_proxyto capture both the agent and proxy value is a clean pattern for subsequent TLS extraction.src/js/thirdparty/undici.js (3)
9-18: Approve fetch wrapper with globalDispatcher support.The fetch wrapper correctly routes through the globalDispatcher when set and no per-request dispatcher/agent is provided. The spread operator properly merges user-provided init with the dispatcher option.
274-286: Approve Dispatcher constructor with options/connect storage.The constructor correctly stores
optionsand wiresoptions.connecttothis.connect. This enables TLS option extraction in Bun's fetch via thegetAgentTLSOptionshelper.
330-335: Approve RetryAgent implementation.The RetryAgent correctly stores both the dispatcher and options. This aligns with undici's API where RetryAgent wraps another dispatcher.
test/js/first_party/undici/undici-properties.test.ts (1)
1-131: Approve comprehensive property validation test.The test correctly validates that all undici classes store
optionsandconnectproperties when constructed with appropriate arguments. The test structure follows best practices:
- Uses
tempDirfrom harness for test file creation- Uses
Bun.spawnfor subprocess execution- Expects stdout before exit code for better error messages
- Tests both populated and empty constructor cases
test/js/node/tls/globalAgent-tls-options.test.ts (5)
1-12: Approve test setup with TLS fixtures.The test properly loads server certificates and CA from the fixtures directory. Using
readFileSyncat module level for test fixtures is appropriate.
13-265: Approve https.request TLS fallback tests.Comprehensive test coverage for https.request TLS fallback scenarios:
- globalAgent.options.rejectUnauthorized
- Per-request override of globalAgent.options
- agent.connectOpts fallback (https-proxy-agent compatibility)
- agent.connect fallback (undici.Agent compatibility)
All tests correctly use port 0 for dynamic port assignment.
267-494: Approve fetch TLS fallback tests.Good coverage for fetch-specific TLS fallback scenarios:
- globalAgent.options.rejectUnauthorized for fetch
- globalAgent.options.ca with checkServerIdentity
- Per-request tls options override
- globalAgent.connectOpts fallback
496-688: Approve agent/dispatcher option tests for fetch.Tests cover the per-request agent and dispatcher options:
- agent.connectOpts takes precedence over globalAgent.options
- dispatcher option for TLS fallback
- dispatcher.connect for undici.Agent compatibility
690-1121: Approve undici module integration tests.Comprehensive undici integration tests covering:
- undici.Agent with connect options
- undici.Pool with connect options
- undici.Client with connect options
- undici.ProxyAgent with connect options
- undici.fetch with globalAgent.options fallback
- undici.fetch with dispatcher
- undici.setGlobalDispatcher affecting fetch TLS
This provides excellent end-to-end test coverage for the TLS option propagation feature.
Contributor
|
@cirospaciari: Understood! I'll proceed with reviewing the code changes and ignore the stale build failure. Let me review the current state of the PR. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/bun.js/webcore/fetch.zig:
- Around line 175-202: The checks in getAgentTLSOptions redundantly combine
isObject() with !isUndefinedOrNull() when validating the results of agent.get;
remove the unnecessary !isUndefinedOrNull() checks and only assert
opts.isObject(), connect_opts.isObject(), and connect.isObject() before calling
hasTLSProperties(globalThis, ...). Update the three branches that inspect
"options", "connectOpts", and "connect" to simplify the conditionals to just
isObject() while preserving the try agent.get(...) and subsequent
hasTLSProperties calls.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/bun.js/webcore/fetch.zigsrc/js/node/_http_client.tssrc/js/thirdparty/undici.js
🧰 Additional context used
📓 Path-based instructions (4)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/node/_http_client.tssrc/js/thirdparty/undici.js
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/node/_http_client.ts
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/fetch.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zig
🧠 Learnings (10)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/js/node/_http_client.tssrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
src/js/thirdparty/undici.jssrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/js/thirdparty/undici.jssrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zig
🧬 Code graph analysis (1)
src/js/node/_http_client.ts (1)
src/js/internal/tls.ts (1)
isValidTLSArray(53-53)
🔇 Additional comments (9)
src/js/node/_http_client.ts (2)
63-70: Clean helper with well-documented fallback chain.The helper correctly implements the fallback order:
agent.options→agent.connectOpts(for https-proxy-agent) →agent.connect(for undici.Agent). This aligns with the TLS extraction logic infetch.zig.
740-745: Consistent TLS option fallback implementation.The pattern is consistently applied across all TLS options with proper type validation for agent-derived values. Error messages clearly indicate these are "agent TLS" options, helping users distinguish between direct option issues and agent configuration issues.
Also applies to: 752-761, 768-777, 784-793, 797-803, 807-813, 817-823, 825-834
src/bun.js/webcore/fetch.zig (4)
204-246: Well-structured TLS settings extraction helper.The
TLSSettingsstruct andextractTLSSettingsfunction provide a clean abstraction for extracting TLS settings, reducing duplication between the two fallback sites (TLS config and proxy agent TLS).
891-1008: Proxy fallback with TLS extraction from agent.The proxy fallback correctly reuses
getAgentTLSOptionsandextractTLSSettingswhen extracting TLS settings from the agent that provides the proxy. This ensures consistent TLS configuration when using proxy agents.One observation: Lines 973-977 handle URL type detection for file/blob URLs, but proxy URLs should typically be HTTP(S) URLs. This inherited logic from the main proxy extraction block is safe but slightly misleading in this context.
159-173: Redundant exception check aftertryoperations.Line 168-170 checks
globalThis.hasException()after thetryin line 163 already handles exceptions. While defensive, this pattern is redundant since thetrywill propagate any JS exception viaerror.JSError.♻️ Suggested simplification
inline for (props) |prop| { if (try obj.get(globalThis, prop)) |val| { if (!val.isUndefinedOrNull()) { return true; } } - if (globalThis.hasException()) { - return error.JSError; - } }⛔ Skipped due to learnings
Learnt from: pfgithub Repo: oven-sh/bun PR: 24273 File: src/bun.js/bindings/JSValue.zig:545-586 Timestamp: 2025-11-03T20:40:59.655Z Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
571-637: TLS fallback integration is correct.The fallback order is properly implemented: per-request
agent→ per-requestdispatcher→https.globalAgent. The integration withextractTLSSettingsavoids code duplication.
getHttpsGlobalAgentis properly exposed onJSGlobalObjectas a public method that wraps the C++ extern bindingJSC__JSGlobalObject__getHttpsGlobalAgent, with documentation indicating it retrieveshttps.globalAgentfrom the node:https module.src/js/thirdparty/undici.js (3)
9-18: Fetch wrapper enables globalDispatcher TLS fallback.The wrapper correctly routes through
globalDispatcheronly when no explicitdispatcheroragentis provided. This enables the TLS fallback chain infetch.zigto extract TLS settings from the global dispatcher.
274-286: Dispatcher stores options and connect for TLS extraction.Storing both
optionsandconnectseparately enablesfetch.zig'sgetAgentTLSOptionsto properly traverse the fallback chain (options→connectOpts→connect).
288-338: Undici classes consistently propagate options to superclass.All undici class constructors (
Agent,Pool,BalancedPool,Client,ProxyAgent,EnvHttpProxyAgent,RetryAgent) now properly pass options to their superclass, ensuring TLS settings are accessible via the inheritance chain.Minor observation:
RetryAgentstores thedispatcherreference (line 336), which allowsfetch.zigto potentially traverse through to the underlying dispatcher's TLS settings if needed.
cirospaciari
requested review from
a team and
dylan-conway
and removed request for
a team
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/bun.js/webcore/fetch.zig:
- Around line 878-979: The checks "isObject() and !isUndefinedOrNull()" in the
agent/dispatcher probe are redundant; update the agent/dispatcher validation
inside the objects_to_try loop (the block that inspects "agent" and "dispatcher"
properties on objects_to_try) to use a single, clear predicate (e.g., only
isObject() or only !isUndefinedOrNull()) when deciding to access .get(...,
"proxy"), removing the duplicate check to simplify the condition while retaining
the same null/undefined safety.
- Around line 568-624: Summary: Remove redundant null/undefined checks where
isObject() already excludes null/undefined. In the fallback_to_agent block,
replace the combined checks "agent.isObject() and !agent.isUndefinedOrNull()"
and the equivalent for "dispatcher" with a single "agent.isObject()" /
"dispatcher.isObject()" check (these occur around the calls that then call
getAgentTLSOptions), leaving all other logic unchanged (including the calls to
getAgentTLSOptions and subsequent extractTLSSettings usage). Ensure the three
occurrences that currently combine isObject() with !isUndefinedOrNull() are
simplified to just isObject().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/webcore/fetch.zig
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/fetch.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zig
🧠 Learnings (9)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zig
🔇 Additional comments (4)
src/bun.js/webcore/fetch.zig (4)
159-170: LGTM!The helper function correctly iterates over TLS-related properties and returns early when a non-null/undefined value is found. The use of
inline foris appropriate for the compile-time known property list.
172-199: LGTM!The fallback chain (
options→connectOpts→connect) is implemented correctly and matches the PR description for compatibility with https-proxy-agent and undici.Agent patterns.
201-206: Verify GC safety forcheck_server_identityJSValue.The
check_server_identityfield stores a rawJSValuewithout GC protection. While this appears safe since the value is extracted and used immediately within the same function call (and the original JS object remains rooted), please verify that no asynchronous operations or allocations occur between extraction and use that could trigger a GC cycle.
208-243: LGTM!The extraction logic correctly handles the different types for
rejectUnauthorized(boolean and number) and validates thatcheckServerIdentityis callable. The SSLConfig allocation is properly managed by the caller's defer block at lines 345-349.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/bun.js/webcore/fetch.zig:
- Around line 201-211: The GC-safety comment above TLSSettings is overly
verbose; replace the multi-line justification with a concise one-line note
stating that check_server_identity stores an unprotected raw JSValue but this is
safe because the source agent_opts remain live on the JS stack for the
synchronous Bun__fetch_ call, so no GC can run before the value is promoted to a
Strong reference; keep the TLSSettings struct and symbol names
(reject_unauthorized, check_server_identity, ssl_config) unchanged.
- Around line 883-984: Add a short explanatory comment before the TLS extraction
block inside the fallback_to_agent_proxy logic: explain that when proxy is
sourced from an agent/dispatcher (current_agent) we must also inherit
TLS/connect options from that agent via getAgentTLSOptions and
extractTLSSettings so per-agent TLS overrides (reject_unauthorized,
check_server_identity, ssl_config) are respected when ssl_config is not already
set; place the comment immediately above the if (ssl_config == null) { ... }
block that calls getAgentTLSOptions/extractTLSSettings in fetch.zig (within the
fallback_to_agent_proxy scope).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/webcore/fetch.zig
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/fetch.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zig
🧠 Learnings (9)
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zig
🔇 Additional comments (3)
src/bun.js/webcore/fetch.zig (3)
159-170: LGTM! Clean property detection helper.The implementation correctly checks for TLS-related properties using proper error propagation. The inline loop and early return optimize the check.
172-199: LGTM! Well-structured agent option extraction.The fallback chain (options → connectOpts → connect) correctly handles multiple agent patterns from different libraries. The unprotected JSValue return is safe here since the agent remains on the stack during the synchronous extraction.
573-629: LGTM! Clean TLS fallback implementation.The logic correctly prioritizes per-request agents over the global HTTPS agent, with proper error handling and memory ownership. The block structure and early exits are well-organized.
Add support for https.globalAgent.options as default TLS options across:
- node:https requests (via _http_client.ts)
- fetch() API (via fetch.zig)
TLS options that now support agent fallback:
- ca, cert, key (with type validation)
- passphrase, ciphers, servername, secureOptions
- rejectUnauthorized (already had fallback, now also for fetch)
- checkServerIdentity (for fetch)
Also supports agent.connectOpts fallback for compatibility with
https-proxy-agent and similar libraries.
Example usage:
```js
const https = require('node:https');
https.globalAgent.options.ca = customCA;
https.globalAgent.options.rejectUnauthorized = true;
// Both now use the global TLS options:
await fetch('https://example.com');
https.get('https://example.com');
```
…lity
Add support for extracting TLS options from agent/dispatcher objects in
fetch() and node:https, enabling compatibility with libraries like
https-proxy-agent and undici.
fetch() now checks for TLS options in this order:
1. Per-request tls option
2. options.agent.options/connectOpts/connect (node-fetch style)
3. options.dispatcher.options/connectOpts/connect (undici style)
4. https.globalAgent.options/connectOpts/connect
node:https now also checks agent.connect for TLS properties, matching
undici.Agent's API which uses connect: { rejectUnauthorized, ca, ... }
Supported properties: rejectUnauthorized, ca, cert, key, passphrase,
ciphers, servername, secureOptions, checkServerIdentity
Also adds proxy extraction from agent.proxy/dispatcher.proxy for future
proxy support.
- Move hasTLSProperties and getAgentTLSOptions from anonymous structs inside Bun__fetch_ to standalone private functions at file scope - Remove function pointer parameter from getAgentTLSOptions, call hasTLSProperties directly - Remove unused getHttpsGlobalAgentOptions from bindings.cpp and JSGlobalObject.zig (dead code that was superseded by getHttpsGlobalAgent)
- Update Dispatcher class to store options and connect properties from constructor - Update Agent, Pool, BalancedPool, Client to pass options to parent Dispatcher - Update DispatcherBase, ProxyAgent, EnvHttpProxyAgent, RetryAgent to store options - Wrap undici.fetch to automatically use globalDispatcher when set via setGlobalDispatcher() - Add tests for dispatcher TLS options storage in undici.test.ts - Add comprehensive undici module integration tests in globalAgent-tls-options.test.ts This enables Bun's fetch to read TLS settings (rejectUnauthorized, ca, etc.) from dispatcher.connect when using undici Agent/Dispatcher classes.
- fetch.zig: Use getAgentTLSOptions() helper instead of manual connectOpts/connect probing in current_agent branch - undici.js: Make DispatcherBase extend Dispatcher (matches real undici), store dispatcher in RetryAgent - _http_client.ts: Extract getAgentTlsOption() helper for 3-step fallback chain (agent.options -> connectOpts -> connect) - Move undici property tests to separate file (undici-properties.test.ts)
- Add hasTLSProperties validation to connectOpts and connect fallback
paths in getAgentTLSOptions to ensure only objects with actual TLS
properties are returned
- Extract duplicate TLS extraction code into shared extractTLSSettings
helper function with TLSSettings struct
- Update agent TLS error messages to be generic ("agent TLS <option>
option") since values can come from options, connectOpts, or connect
- Store constructor parameters in undici Pool, BalancedPool, and Client
classes (origin, upstreams, url respectively)
isObject() already implies the value is not undefined or null, so the additional checks are unnecessary.
Remove 6 redundant globalThis.hasException() checks that were incompatible with strict exception checks. These checks were unnecessary because all preceding operations use 'try' which already propagates exceptions as error.JSError.
…ions Add fallback to https.globalAgent.options for TLS settings in the WebSocket constructor, similar to what was implemented in fetch.zig. When no explicit TLS config or proxy is provided, fall back to the global agent's settings. Changes: - Add applyGlobalAgentFallback() helper function to extract proxy and TLS options from https.globalAgent - Update constructJSWebSocket1 and constructJSWebSocket2 to use the helper and the full WebSocket::create overload with TLS support - Update constructJSWebSocket3 to reuse the helper instead of duplicating the fallback logic - Add 6 new tests for globalAgent fallback behavior
cirospaciari
force-pushed
the
ciro/globalAgent-tls-options
branch
from
75d0170 to
2d29b31
Compare
Contributor
|
@cirospaciari: I'll review the current code changes in this PR, focusing on the implementation and ignoring the previous build errors. 🧠 Learnings used✅ Actions performedReview triggered.
|
cirospaciari
changed the title
cirospaciari
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In @src/bun.js/bindings/bindings.cpp:
- Around line 6125-6144: The function JSC__JSGlobalObject__getHttpsGlobalAgent
should be exported as Zig-callable and nothrow by adding the
[[ZIG_EXPORT(nothrow)]] attribute, and should use a catch scope instead of
DECLARE_THROW_SCOPE so exceptions are properly suppressed; replace
DECLARE_THROW_SCOPE and RETURN_IF_EXCEPTION calls with DECLARE_CATCH_SCOPE(vm),
after each operation that may throw check if (catchScope.exception()) {
catchScope.clearException(); return JSValue::encode(jsUndefined()); } (e.g.,
after requireId and get calls) so the VM has no pending exceptions and the
function returns undefined on error.
In @src/bun.js/bindings/JSGlobalObject.zig:
- Around line 803-808: The new cross-language wrapper getHttpsGlobalAgent does
not call jsc.markBinding(@src()), so add a call to jsc.markBinding(@src()) at
the start of pub fn getHttpsGlobalAgent(this: *JSGlobalObject) before invoking
JSC__JSGlobalObject__getHttpsGlobalAgent(this) so that this binding shows in
trace output; ensure you import/qualify jsc.markBinding as used elsewhere in
other JSGlobalObject-boundary helpers if needed.
In @src/bun.js/bindings/webcore/JSWebSocket.cpp:
- Around line 152-259: The fallback in applyGlobalAgentFallback currently checks
globalAgent.options then globalAgent.connectOpts for TLS settings but omits the
undici-style globalAgent.connect; update the logic in applyGlobalAgentFallback
to also check for a "connect" property (after checking "options" and
before/alongside "connectOpts") and use it as tlsSourceValue when it's an object
and not undefined/null, so that rejectUnauthorized and the filtered TLS options
(ca/cert/key/passphrase) are extracted from globalAgent.connect the same way
they are from options/connectOpts; adjust the sequence around the existing
optionsValue/connectOptsValue handling to include reading
Bun::getOwnPropertyIfExists(..., "connect"_s) and fallback accordingly.
In @src/bun.js/webcore/fetch.zig:
- Around line 879-981: The agent/dispatcher lookup is duplicated; extract it
into a helper (e.g., fn findAgent(globalThis: *JSGlobalObject, options_object:
?JSValue, request_init_object: ?JSValue) ?JSValue) that encapsulates the logic
of checking options_object/request_init_object for "agent" or "dispatcher" and
falling back to globalThis.getHttpsGlobalAgent(), then replace the inline blk in
the proxy fallback (agent_and_proxy) and the earlier TLS fallback block to call
findAgent and then query the returned object's "proxy" or TLS options as before.
In @src/js/node/_http_client.ts:
- Around line 63-70: Replace the fallback chain in getAgentTlsOption with
nullish-coalescing to make intent clearer: instead of sequentially checking
agent?.options?.[key], agent?.connectOpts?.[key], agent?.connect?.[key] with if
(value === undefined) checks, return agent?.options?.[key] ??
agent?.connectOpts?.[key] ?? agent?.connect?.[key]; preserve the same
undefined/null handling and keep the function name getAgentTlsOption and the
same property order.
- Around line 740-834: The code treats falsy but valid values (e.g.,
secureOptions: 0) as “not provided” and doesn't validate rejectUnauthorized as a
boolean; update all option checks to use strict undefined checks (e.g., if
(options.secureOptions !== undefined) rather than if (options.secureOptions)) so
explicit falsy values are honored, and add boolean validation for
options.rejectUnauthorized and agent-derived rejectUnauthorized via
getAgentTlsOption; keep existing type checks for
ca/cert/key/passphrase/ciphers/servername/secureOptions for both request and
agent fallbacks and ensure agent-derived values are validated before assigning
via _ensureTls().
In @src/js/thirdparty/undici.js:
- Around line 274-339: The Dispatcher currently assigns this.options and
this.connect making them public enumerable properties, which diverges from
upstream undici; change this to store TLS fallback data non-enumerably and
document the divergence: in the Dispatcher constructor (affecting Dispatcher,
Agent, Pool, Client, and DispatcherBase) replace direct assignments with
Object.defineProperty-style non-enumerable properties for options and connect,
add a short comment explaining this is a deliberate Bun TLS fallback shim (so
future maintainers know why it differs from upstream), and ensure no other code
relies on those properties being enumerable to avoid introspection/compatibility
breaking changes.
In @test/js/first_party/undici/undici-properties.test.ts:
- Around line 5-130: The test currently awaits stdout, stderr and proc.exited
together which makes the exitCode assertion run before you can inspect outputs;
change the ordering so you first await proc.stdout.text() and proc.stderr.text()
(e.g. await Promise.all([proc.stdout.text(), proc.stderr.text()]) to get stdout
and stderr), then separately await proc.exited to get exitCode, and then run
assertions in the order: check stderr/stdout for diagnostics first and only then
assert exitCode; also consider relaxing the strict stderr === "" check to allow
purely-whitespace output (e.g. use a trim or regex) if flaky.
In @test/js/first_party/undici/undici.test.ts:
- Around line 158-282: Replace hard-coded "http://localhost" in tests for Pool,
BalancedPool, Client with the existing hostUrl variable so constructors don't
accidentally hit a local service; hoist const { Agent, Dispatcher, Pool,
BalancedPool, Client, ProxyAgent, EnvHttpProxyAgent, RetryAgent } =
require("undici") to the top of the describe block and create a small
table-driven helper that iterates over entries for each class (Agent,
Dispatcher, Pool, BalancedPool, Client, ProxyAgent, EnvHttpProxyAgent,
RetryAgent) to assert options/connect presence and rejectUnauthorized/ca values,
so TLS assertions are written once and reused instead of duplicated across the
8+ it blocks (refer to symbols Agent, Dispatcher, Pool, BalancedPool, Client,
ProxyAgent, EnvHttpProxyAgent, RetryAgent and hostUrl).
In @test/js/node/tls/globalAgent-tls-options.test.ts:
- Around line 69-74: The test currently asserts expect(stderr).toBe("") which is
brittle; update each occurrence of expect(stderr).toBe("") (e.g., in the test
blocks that also assert stdout.trim() === "Hello" and exitCode === 0) to avoid
failing on benign platform warnings: either remove the stderr assertion entirely
and rely on expect(stdout.trim()).toBe("...") and expect(exitCode).toBe(0), or
replace it with a negative-match that only fails on real errors (e.g.,
expect(stderr).not.toMatch(/(Error|Exception|Traceback)/)). Apply this change to
every test instance that uses expect(stderr).toBe("").
- Around line 13-1121: The test suite uses describe.concurrent for several
suites (instances of describe.concurrent such as the ones titled "https.request
uses globalAgent.options", "fetch uses globalAgent.options as fallback", "fetch
uses agent/dispatcher option for TLS fallback", and "undici module
integration"); change each describe.concurrent to plain describe so the
expensive TLS + Bun.spawn tests run serially to avoid CI flakiness and resource
contention.
In @test/js/web/websocket/websocket-proxy.test.ts:
- Around line 913-929: The test replaces https.globalAgent.options with a cloned
object which can leak state and also fails to restore the new fallback fields;
capture originalConnectOpts and originalConnect in beforeEach, and in afterEach
restore the options object by mutating it (e.g.,
Object.assign(https.globalAgent.options, originalOptions)) rather than replacing
it, and restore https.globalAgent.connectOpts and https.globalAgent.connect by
reassigning them from the saved originals; also add snapshots/assertions for
https.globalAgent.connectOpts and https.globalAgent.connect (e.g.,
expect(...).toMatchSnapshot()) so the new fallback surface is validated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/webcore/fetch.zigsrc/js/node/_http_client.tssrc/js/thirdparty/undici.jstest/js/first_party/undici/undici-properties.test.tstest/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/web/websocket/websocket-proxy.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cpp
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/thirdparty/undici.jssrc/js/node/_http_client.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/node/_http_client.ts
🧠 Learnings (48)
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.tssrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.tssrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tssrc/bun.js/webcore/fetch.zigsrc/js/thirdparty/undici.js
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tssrc/js/thirdparty/undici.js
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cppsrc/js/thirdparty/undici.jssrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/bindings.cppsrc/js/thirdparty/undici.jssrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/web/websocket/websocket-proxy.test.tstest/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Prefer async/await over callbacks in tests. When callbacks must be used for a single callback, use `Promise.withResolvers` to create a promise that can be resolved or rejected from a callback.
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/webcore/fetch.zigsrc/js/node/_http_client.ts
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/webcore/fetch.zigsrc/js/thirdparty/undici.js
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-12-23T06:50:41.142Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-18T20:59:45.579Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:458-475
Timestamp: 2025-10-18T20:59:45.579Z
Learning: In src/bun.js/telemetry.zig, the RequestId (u64) to JavaScript number (f64) conversion in jsRequestId() is intentionally allowed to lose precision beyond 2^53-1. This is acceptable because: (1) at 1M requests/sec it takes ~285 years to overflow, (2) the counter resets per-process, and (3) these are observability IDs, not critical distributed IDs. Precision loss is an acceptable trade-off for this use case.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/js/thirdparty/undici.jssrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use private globals and methods with `$` prefix (e.g., `$Array`, `map.$set()`) instead of public JavaScript globals
Applied to files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts : Builtin functions must include `this` parameter typing in TypeScript to enable direct method binding in C++
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cpp
🧬 Code graph analysis (5)
test/js/node/tls/globalAgent-tls-options.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
test/js/web/websocket/websocket-proxy.test.ts (2)
test/js/bun/http/bun-websocket-cpu-fixture.js (1)
ws(25-25)src/bun.js/bindings/webcore/WebSocket.cpp (2)
WebSocket(180-188)WebSocket(190-224)
test/js/first_party/undici/undici-properties.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
src/bun.js/bindings/webcore/JSWebSocket.cpp (3)
src/bun.js/bindings/webcore/WebSocket.h (2)
sslConfig(181-184)url(135-164)src/bun.js/bindings/ObjectBindings.cpp (2)
getOwnPropertyIfExists(95-108)getOwnPropertyIfExists(95-95)src/bun.js/bindings/webcore/WebSocket.cpp (18)
url(1100-1103)url(1100-1100)create(294-297)create(294-294)create(299-302)create(299-299)create(304-319)create(304-304)create(320-336)create(320-320)create(338-355)create(338-338)create(357-375)create(357-357)create(377-380)create(377-377)protocol(1115-1118)protocol(1115-1115)
src/js/node/_http_client.ts (1)
src/js/internal/tls.ts (1)
isValidTLSArray(53-53)
🔇 Additional comments (6)
src/bun.js/bindings/webcore/JSWebSocket.cpp (1)
278-289: LGTM: fallback application points are correct (constructor overloads get proxy + TLS consistently).Calling the fallback before
WebSocket::create(...)in the 1-arg/2-arg constructors and as a final fallback in the options-object constructor is the right shape to keep precedence rules intact.Also applies to: 319-333, 575-581
src/js/thirdparty/undici.js (1)
9-18: Movelet globalDispatcherabove the wrapper to avoid any TDZ/init-order surprises.Proposed change
const kEmptyObject = ObjectCreate(null); -const bunFetch = Bun.fetch; - -// Wrapper fetch that uses globalDispatcher when set -function fetch(input, init) { +let globalDispatcher; + +const bunFetch = Bun.fetch; + +// Wrapper fetch that uses globalDispatcher when set +function fetch(input, init) { // If no dispatcher/agent is provided and globalDispatcher is set, use it if (globalDispatcher && init?.dispatcher === undefined && init?.agent === undefined) { return bunFetch(input, { ...init, dispatcher: globalDispatcher }); } return bunFetch(input, init); } ... -let globalDispatcher; - // Add missing dispatcher functions function setGlobalDispatcher(dispatcher) { globalDispatcher = dispatcher; }Also applies to: 456-465
⛔ Skipped due to learnings
Learnt from: theshadow27 Repo: oven-sh/bun PR: 23798 File: packages/bun-otel/BunFetchInstrumentation.ts:126-131 Timestamp: 2025-10-20T01:38:02.660Z Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.src/bun.js/webcore/fetch.zig (4)
159-170: LGTM: Clean TLS property detection.The function correctly checks for TLS-related properties and handles errors appropriately. The early return optimization is good.
172-199: LGTM: Agent TLS options fallback logic is sound.The function correctly implements the fallback chain (options → connectOpts → connect) for compatibility with different agent implementations (standard, https-proxy-agent, undici).
201-244: LGTM: TLS settings extraction is well-structured.The extractTLSSettings function properly handles type coercion for
rejectUnauthorized(boolean or number) and validatescheckServerIdentityis callable. The comment aboutcheck_server_identityholding a raw JSValue is correct—it's promoted to a Strong reference at line 1652 before being passed to FetchTasklet, so there's no GC risk during the synchronous call.Memory management for
ssl_configis properly handled via the defer at lines 346-350.
569-625: LGTM: TLS fallback implements correct precedence.The fallback logic properly prioritizes per-request
agent/dispatcheroptions overhttps.globalAgent, and only applies when no explicittlsconfig was provided. The dual check for bothagent(node-fetch style) anddispatcher(undici style) ensures broad compatibility.
- bindings.cpp: Use catch scope with nothrow export for getHttpsGlobalAgent - JSGlobalObject.zig: Add markBinding call for proper binding tracking - JSWebSocket.cpp: Add 'connect' property check for undici.Agent compat - fetch.zig: Extract findAgentOrDispatcher and findAgentWithProxy helpers - _http_client.ts: Use nullish-coalescing, add boolean validation for rejectUnauthorized, fix secureOptions to use strict undefined check - undici.js: Make options/connect properties non-enumerable with docs - Test fixes: Fix assertion ordering, use hostUrl variable, remove describe.concurrent, relax stderr assertions, fix state restoration
Contributor
|
@cirospaciari: I'll review the current code changes in this PR, focusing on the implementation quality and ignoring the previous build errors as requested. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/first_party/undici/undici.test.ts (1)
120-132: RemovesetTimeoutusage in tests—useAbortSignal.timeout()instead.Line 123 uses
setTimeoutto schedule the abort, which violates the coding guidelines and can introduce flakiness. Replace withAbortSignal.timeout(500)to create an abort signal that fires after 500ms without relying on setTimeout. This pattern is used consistently throughout the codebase.src/js/node/_http_client.ts (1)
737-839: Fix option presence checks: use!== undefinedto prevent unintended fallback to agent options.The code uses truthy checks (
if (options.ca),if (options.cert),if (options.key),if (options.passphrase),if (options.ciphers),if (options.servername)) which incorrectly treat falsy user-provided values (empty strings,false,0) as absent, causing them to unexpectedly fall through to agent/global options. Replace with explicit presence checks (!== undefined).Affected lines and fix
- if (options.ca) { + if (options.ca !== undefined) { ... } - if (options.cert) { + if (options.cert !== undefined) { ... } - if (options.key) { + if (options.key !== undefined) { ... } - if (options.passphrase) { + if (options.passphrase !== undefined) { ... } - if (options.ciphers) { + if (options.ciphers !== undefined) { ... } - if (options.servername) { + if (options.servername !== undefined) { ... }Note:
rejectUnauthorizedandsecureOptionsalready use!== undefinedcorrectly.
🤖 Fix all issues with AI agents
In @src/bun.js/bindings/bindings.cpp:
- Around line 6125-6150: In JSC__JSGlobalObject__getHttpsGlobalAgent replace
unconditional catchScope.clearException() calls with the variant that preserves
termination exceptions (e.g., clearExceptionExceptTermination or the project
equivalent) so shutdown/termination exceptions aren’t swallowed, and avoid
constructing the "globalAgent" Identifier twice by creating a local Identifier
(e.g., auto globalAgentId = Identifier::fromString(vm, "globalAgent"_s)) and
reusing it when calling httpsModule.getObject()->get(globalObject,
globalAgentId) and any other places in this function; keep existing
null/undefined checks and return behavior unchanged.
In @src/bun.js/bindings/webcore/JSWebSocket.cpp:
- Around line 152-266: The gating for inheriting TLS fields incorrectly requires
both rejectUnauthorized == -1 and !sslConfig; update applyGlobalAgentFallback so
the two concerns are handled separately: first, if tlsSourceValue is an object,
extract rejectUnauthorized into rejectUnauthorized only when rejectUnauthorized
== -1 (using the existing rejectValue logic), and then independently, if
(!sslConfig) build filteredTlsOpts from tlsSourceObj (ca, cert, key,
passphrase), call Bun__WebSocket__parseSSLConfig and set sslConfig; keep the
existing RETURN_IF_EXCEPTION checks and reuse tlsSourceValue/tlsSourceObj logic
but split the condition that currently reads if (rejectUnauthorized == -1 &&
!sslConfig) into separate paths for the two responsibilities.
In @src/js/node/_http_client.ts:
- Around line 63-67: getAgentTlsOption currently stops the fallback chain if an
agent explicitly sets options[key] = null; to treat null as "unset" normalize
null to undefined inside getAgentTlsOption so the chain continues (i.e., when
reading each candidate value from agent.options, agent.connectOpts, and
agent.connect treat value === null as undefined), or alternatively change the
function to check for value != null for each source before returning; update the
getAgentTlsOption implementation accordingly so callers get undefined for unset
keys instead of null.
In @src/js/thirdparty/undici.js:
- Around line 274-349: The Dispatcher constructor currently calls
Object.defineProperty directly (in the "options" and "connect" branches) which
is user-mutable; bind and use the primordials version instead. Import/bind the
primordial defineProperty once at the top of the module (e.g.,
ObjectDefineProperty or Object_defineProperty from the project's primordials
entry) and replace all direct Object.defineProperty calls in Dispatcher (and any
other places in this file that call Object.defineProperty) with that primordial
binding to prevent userland tampering; ensure the property descriptor and
configurable/writable/enumerable flags remain identical.
In @test/js/first_party/undici/undici.test.ts:
- Around line 158-282: Tests duplicate require("undici") and mostly only assert
rejectUnauthorized; import undici exports once (e.g., const { Agent, Dispatcher,
Pool, BalancedPool, Client, ProxyAgent, EnvHttpProxyAgent, RetryAgent } =
require("undici")) at top of the describe and reuse them, consolidate the
repeated connect object into a shared const connectOpts = { rejectUnauthorized:
false, ca: "test-ca" } and pass/modify as needed; add an extra TLS assertion
(e.g., expect(...connect.ca).toBe("test-ca")) for 1–2 representative classes
beyond Agent (such as Pool and Client) to guard non-boolean fields; and confirm
whether Dispatcher is intended to be constructed in this environment—if not,
remove or adapt the Dispatcher-specific test to use the supported public API
(refer to Dispatcher and RetryAgent usage) so tests match the shim's public
surface.
In @test/js/node/tls/globalAgent-tls-options.test.ts:
- Around line 1-1121: Remove the brittle stderr regex assertions (the lines
calling expect(stderr).not.toMatch(/(Error|Exception|Traceback)/i)) and instead
rely on the existing exitCode and stdout checks; keep the Bun.spawn/tempDir
pattern. Update assertions that validate stdout/exitCode (references: stdout,
stderr, exitCode, and the expect(...) calls that check stdout.trim() and
exitCode) to fail with stderr included in the failure context—e.g., drop the
stderr regex check and ensure test failures surface stderr by including the
stderr variable in the test assertion failure message or by adding an assertion
that prints/stores stderr on failure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/webcore/fetch.zigsrc/js/node/_http_client.tssrc/js/thirdparty/undici.jstest/js/first_party/undici/undici-properties.test.tstest/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/web/websocket/websocket-proxy.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/node/_http_client.tssrc/js/thirdparty/undici.js
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/node/_http_client.ts
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cpp
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
🧠 Learnings (65)
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.tssrc/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/first_party/undici/undici.test.tstest/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.tssrc/bun.js/webcore/fetch.zigsrc/js/thirdparty/undici.js
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/js/node/_http_client.tssrc/bun.js/webcore/fetch.zigsrc/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Always check exit codes and test error scenarios when spawning processes in tests.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cppsrc/js/thirdparty/undici.jssrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/bindings.cppsrc/js/thirdparty/undici.jssrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cppsrc/js/thirdparty/undici.jssrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-02T18:09:21.647Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:79-85
Timestamp: 2025-09-02T18:09:21.647Z
Learning: In Bun's Zig codebase, prefer simple, direct error handling patterns. When an error type has only one variant (like FreeError with only NotAllocated), use a switch statement rather than generic catch blocks to leverage compile-time knowledge and avoid unnecessary complexity. Avoid duplicating function calls across error paths when a cleaner structure is possible.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2026-01-05T16:32:15.175Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:15.175Z
Learning: In Bun's Zig codebase, std.posix.sigaction returns void (not an error union) and should be called without error handling. The Zig standard library treats sigaction failures as programmer errors (unreachable) since they only occur with invalid signals like SIGKILL/SIGSTOP. This pattern is used consistently across crash_handler.zig, main.zig, filter_run.zig, and process.zig.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-23T06:50:41.142Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
test/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.tssrc/bun.js/webcore/fetch.zigsrc/js/thirdparty/undici.js
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/web/websocket/websocket-proxy.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Prefer async/await over callbacks in tests. When callbacks must be used for a single callback, use `Promise.withResolvers` to create a promise that can be resolved or rejected from a callback.
Applied to files:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Prefer concurrent tests over sequential tests. Use `test.concurrent` or `describe.concurrent` when multiple tests spawn processes or write files, unless it is very difficult to make them concurrent.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For single-file tests, prefer `-e` flag over `tempDir`
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tssrc/js/thirdparty/undici.js
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/webcore/fetch.zigsrc/js/thirdparty/undici.js
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-18T20:59:45.579Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:458-475
Timestamp: 2025-10-18T20:59:45.579Z
Learning: In src/bun.js/telemetry.zig, the RequestId (u64) to JavaScript number (f64) conversion in jsRequestId() is intentionally allowed to lose precision beyond 2^53-1. This is acceptable because: (1) at 1M requests/sec it takes ~285 years to overflow, (2) the counter resets per-process, and (3) these are observability IDs, not critical distributed IDs. Precision loss is an acceptable trade-off for this use case.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zigsrc/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/js/thirdparty/undici.js
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use private globals and methods with `$` prefix (e.g., `$Array`, `map.$set()`) instead of public JavaScript globals
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use JSC::WriteBarrier for heap-allocated references in V8 objects and implement visitChildren() for custom heap objects to support garbage collection
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
🧬 Code graph analysis (5)
test/js/first_party/undici/undici.test.ts (1)
src/js/node/_http_client.ts (2)
agent(1049-1051)agent(1053-1055)
src/js/node/_http_client.ts (1)
src/js/internal/tls.ts (1)
isValidTLSArray(53-53)
test/js/first_party/undici/undici-properties.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
test/js/web/websocket/websocket-proxy.test.ts (1)
src/bun.js/bindings/webcore/WebSocket.cpp (2)
WebSocket(180-188)WebSocket(190-224)
test/js/node/tls/globalAgent-tls-options.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (10)
src/bun.js/bindings/JSGlobalObject.zig (1)
803-809: Binding wrapper looks correct; confirm the extern never throws.
getHttpsGlobalAgent()is consistent (marks binding, delegates). IfJSC__JSGlobalObject__getHttpsGlobalAgent()can ever raise a JS exception, this wrapper should usebun.jsc.fromJSHostCall(...)to ensure proper propagation/handling instead of returning an arbitraryJSValue.test/js/web/websocket/websocket-proxy.test.ts (2)
913-936: Global agent state restore is correct (preserves object identity).Good call mutating
https.globalAgent.optionsinstead of replacing it, which avoids breaking consumers holding the original reference.
1007-1037: Potential flake: TLS failure may surface as close-only in some implementations.The “fails without globalAgent TLS options” case assumes an
errorevent must fire. If this flakes on a platform/engine path, consider allowing either:
- error event, or
- close without open (and no messages), with a stronger assertion that
onopennever fired.src/bun.js/bindings/webcore/JSWebSocket.cpp (1)
272-306: Wiring fallback into ctor paths is good; keep precedence consistent with 3-arg path.Calling
applyGlobalAgentFallback()in the 1/2-arg constructors closes the feature gap nicely, and doing it in the 3-arg path after parsing explicit options matches the intended precedence. Once the TLS gating issue above is fixed, this should behave consistently across all constructor overloads.Also applies to: 313-350, 582-599
src/js/thirdparty/undici.js (1)
9-18: Global-dispatcher fetch wrapper is reasonable; verifynullhandling matches undici.You only apply
globalDispatcherwheninit.dispatcher === undefined && init.agent === undefined. If callers passdispatcher: null(oragent: null), they won’t get the global fallback. Please confirm that matches the intended undici compatibility expectations (undefined vs null).src/bun.js/webcore/fetch.zig (5)
159-170: LGTM! Lightweight TLS property detection.The function appropriately checks for presence of TLS properties without deep type validation. Type validation is handled downstream in
SSLConfig.fromJS, keeping this helper fast and focused.
271-314: Excellent documentation and proper memory management.The detailed comment on line 272 about
check_server_identitybeing safe as an unprotected JSValue is exemplary documentation. The explanation is correct—the value is used synchronously and immediately promoted to aStrongreference at line 1645.Memory management is sound:
ssl_configis heap-allocated only whenSSLConfig.fromJSsucceeds- Caller owns cleanup via the
deferblock at lines 416-420- No leak paths exist
639-660: TLS fallback logic correctly implements the resolution priority.The fallback runs only when no explicit TLS configuration is provided, properly checking for
nullbefore proceeding. Error handling is correct, and all extracted settings are applied conditionally based on their presence.Resolution order: per-request
tlsoption → agent TLS options → global agent TLS options.
914-974: Proxy fallback with nested TLS extraction is correctly implemented.The logic properly handles both string and object proxy formats, and the nested TLS extraction (lines 956-972) correctly runs only when:
- No explicit proxy was provided (
proxy == nullat line 916)- An agent/dispatcher with a proxy is found
- No TLS config has been set yet (
ssl_config == nullat line 956)This ensures per-agent TLS settings are inherited when using an agent's proxy, maintaining the correct resolution priority: per-request options → agent-specific settings → global agent settings.
Memory management is sound: old
url_proxy_bufferis freed (line 951) before the new buffer is assigned (line 952).
172-269: Well-structured agent resolution with proper fallback chain.The priority order (
agent.options→agent.connectOpts→agent.connect→https.globalAgent) correctly implements the resolution strategy. TheconnectOptsfallback ensures compatibility with libraries likehttps-proxy-agent. ThegetHttpsGlobalAgent()binding is properly exported fromJSGlobalObject.zigand integrated across the codebase.
… feedback - Use clearExceptionExceptTermination() to preserve termination exceptions - Reuse Identifier for globalAgent lookup instead of constructing twice - Pass full TLS source object to SSLConfig.fromJS instead of building filtered object - the parser extracts only properties it needs - Split combined condition (rejectUnauthorized == -1 && !sslConfig) into separate paths so each can be extracted independently - Treat null as undefined in getAgentTlsOption fallback chain - Use primordial ObjectDefineProperty in undici.js - Hoist undici imports to describe level and share connectOpts in tests - Add ca assertions for Pool and Client tests - Remove brittle stderr regex assertions from TLS tests - Convert undici-properties test to use -e flag instead of tempDir
Contributor
|
@cirospaciari: I'll review the current code changes in this PR, focusing on the implementation details. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/_http_client.ts (1)
744-846: Fix option “presence” checks to avoid treating empty strings as “unset”.Several TLS options use truthiness checks (
if (options.ca),if (options.passphrase),if (options.servername), etc.). This will skip intentionally-provided empty strings and then incorrectly fall back to agent-derived values.Proposed fix
- if (options.ca) { + if (options.ca !== undefined) { if (!isValidTLSArray(options.ca)) throw new TypeError( "ca argument must be an string, Buffer, TypedArray, BunFile or an array containing string, Buffer, TypedArray or BunFile", ); this._ensureTls().ca = options.ca; } else { const agentCa = getAgentTlsOption(agent, "ca"); if (agentCa !== undefined) { if (!isValidTLSArray(agentCa)) throw new TypeError( "agent TLS ca option must be a string, Buffer, TypedArray, BunFile or an array containing string, Buffer, TypedArray or BunFile", ); this._ensureTls().ca = agentCa; } } - if (options.cert) { + if (options.cert !== undefined) { if (!isValidTLSArray(options.cert)) throw new TypeError( "cert argument must be an string, Buffer, TypedArray, BunFile or an array containing string, Buffer, TypedArray or BunFile", ); this._ensureTls().cert = options.cert; } else { const agentCert = getAgentTlsOption(agent, "cert"); if (agentCert !== undefined) { if (!isValidTLSArray(agentCert)) throw new TypeError( "agent TLS cert option must be a string, Buffer, TypedArray, BunFile or an array containing string, Buffer, TypedArray or BunFile", ); this._ensureTls().cert = agentCert; } } - if (options.key) { + if (options.key !== undefined) { if (!isValidTLSArray(options.key)) throw new TypeError( "key argument must be an string, Buffer, TypedArray, BunFile or an array containing string, Buffer, TypedArray or BunFile", ); this._ensureTls().key = options.key; } else { const agentKey = getAgentTlsOption(agent, "key"); if (agentKey !== undefined) { if (!isValidTLSArray(agentKey)) throw new TypeError( "agent TLS key option must be a string, Buffer, TypedArray, BunFile or an array containing string, Buffer, TypedArray or BunFile", ); this._ensureTls().key = agentKey; } } - if (options.passphrase) { + if (options.passphrase !== undefined) { if (typeof options.passphrase !== "string") throw new TypeError("passphrase argument must be a string"); this._ensureTls().passphrase = options.passphrase; } else { const agentPassphrase = getAgentTlsOption(agent, "passphrase"); if (agentPassphrase !== undefined) { if (typeof agentPassphrase !== "string") throw new TypeError("agent TLS passphrase option must be a string"); this._ensureTls().passphrase = agentPassphrase; } } - if (options.ciphers) { + if (options.ciphers !== undefined) { if (typeof options.ciphers !== "string") throw new TypeError("ciphers argument must be a string"); this._ensureTls().ciphers = options.ciphers; } else { const agentCiphers = getAgentTlsOption(agent, "ciphers"); if (agentCiphers !== undefined) { if (typeof agentCiphers !== "string") throw new TypeError("agent TLS ciphers option must be a string"); this._ensureTls().ciphers = agentCiphers; } } - if (options.servername) { + if (options.servername !== undefined) { if (typeof options.servername !== "string") throw new TypeError("servername argument must be a string"); this._ensureTls().servername = options.servername; } else { const agentServername = getAgentTlsOption(agent, "servername"); if (agentServername !== undefined) { if (typeof agentServername !== "string") throw new TypeError("agent TLS servername option must be a string"); this._ensureTls().servername = agentServername; } }
🤖 Fix all issues with AI agents
In @src/bun.js/bindings/bindings.cpp:
- Around line 6125-6150: In JSC__JSGlobalObject__getHttpsGlobalAgent ensure you
return undefined immediately when a non-termination exception was cleared so you
never use JSValues created under an exception: after the requireId(...) call, if
catchScope.exception() and clearExceptionExceptTermination() returns true,
immediately return JSValue::encode(jsUndefined()); do the same after the
get(...) call (i.e. check catchScope.exception() and return jsUndefined() if
clearExceptionExceptTermination() cleared a non-termination exception) so
httpsModule or globalAgent are never used when an exception was pending.
In @test/js/first_party/undici/undici-properties.test.ts:
- Around line 114-125: Add a stderr diagnostic when the spawned process fails:
after awaiting proc.stdout, proc.stderr, and proc.exited (variables stdout,
stderr, exitCode) insert a conditional like if (exitCode !== 0)
console.error("Test stderr:", stderr); before the existing
expect(exitCode).toBe(0) so test failures print the child process stderr for
easier debugging.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/webcore/JSWebSocket.cppsrc/js/node/_http_client.tssrc/js/thirdparty/undici.jstest/js/first_party/undici/undici-properties.test.tstest/js/first_party/undici/undici.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/node/_http_client.tssrc/js/thirdparty/undici.js
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/node/_http_client.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/bindings.cpp
🧠 Learnings (63)
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/js/node/_http_client.tssrc/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.tssrc/js/thirdparty/undici.js
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Always check exit codes and test error scenarios when spawning processes in tests.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For single-file tests, prefer `-e` flag over `tempDir`
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Use `-e` flag for single-file tests when spawning Bun processes with `Bun.spawn()`.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/js/first_party/undici/undici-properties.test.tstest/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/first_party/undici/undici-properties.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Prefer concurrent tests over sequential tests. Use `test.concurrent` or `describe.concurrent` when multiple tests spawn processes or write files, unless it is very difficult to make them concurrent.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tstest/js/first_party/undici/undici.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tssrc/js/thirdparty/undici.js
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/js/node/tls/globalAgent-tls-options.test.tssrc/js/thirdparty/undici.js
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Prefer async/await over callbacks in tests. When callbacks must be used for a single callback, use `Promise.withResolvers` to create a promise that can be resolved or rejected from a callback.
Applied to files:
test/js/first_party/undici/undici.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/js/thirdparty/undici.jssrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts : Builtin functions must include `this` parameter typing in TypeScript to enable direct method binding in C++
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-09-03T22:12:26.870Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22378
File: src/sql/mysql/MySQLConnection.zig:0-0
Timestamp: 2025-09-03T22:12:26.870Z
Learning: In Bun's MySQL SSL implementation, the .require SSL mode is designed to behave the same as .prefer for postgres.js compatibility, allowing graceful fallback to non-SSL connections when the server doesn't support SSL. Only .verify_ca and .verify_full modes will fail when SSL is not available.
Applied to files:
src/bun.js/bindings/webcore/JSWebSocket.cpp
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/js/thirdparty/undici.jssrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use private globals and methods with `$` prefix (e.g., `$Array`, `map.$set()`) instead of public JavaScript globals
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `.$call()` and `.$apply()` instead of `.call()` and `.apply()` to prevent user tampering with function invocation
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/js/thirdparty/undici.jssrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
src/js/thirdparty/undici.js
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-02T18:09:21.647Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:79-85
Timestamp: 2025-09-02T18:09:21.647Z
Learning: In Bun's Zig codebase, prefer simple, direct error handling patterns. When an error type has only one variant (like FreeError with only NotAllocated), use a switch statement rather than generic catch blocks to leverage compile-time knowledge and avoid unnecessary complexity. Avoid duplicating function calls across error paths when a cleaner structure is possible.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2026-01-05T16:32:15.175Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:15.175Z
Learning: In Bun's Zig codebase, std.posix.sigaction returns void (not an error union) and should be called without error handling. The Zig standard library treats sigaction failures as programmer errors (unreachable) since they only occur with invalid signals like SIGKILL/SIGSTOP. This pattern is used consistently across crash_handler.zig, main.zig, filter_run.zig, and process.zig.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-23T06:50:41.142Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-18T20:59:45.579Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:458-475
Timestamp: 2025-10-18T20:59:45.579Z
Learning: In src/bun.js/telemetry.zig, the RequestId (u64) to JavaScript number (f64) conversion in jsRequestId() is intentionally allowed to lose precision beyond 2^53-1. This is acceptable because: (1) at 1M requests/sec it takes ~285 years to overflow, (2) the counter resets per-process, and (3) these are observability IDs, not critical distributed IDs. Precision loss is an acceptable trade-off for this use case.
Applied to files:
src/bun.js/bindings/bindings.cpp
🧬 Code graph analysis (5)
src/js/node/_http_client.ts (1)
src/js/internal/tls.ts (1)
isValidTLSArray(53-53)
test/js/node/tls/globalAgent-tls-options.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
test/js/first_party/undici/undici.test.ts (1)
src/js/node/_http_client.ts (2)
agent(1056-1058)agent(1060-1062)
src/bun.js/bindings/webcore/JSWebSocket.cpp (3)
src/bun.js/bindings/webcore/WebSocket.h (2)
sslConfig(181-184)url(135-164)src/bun.js/bindings/ObjectBindings.cpp (2)
getOwnPropertyIfExists(95-108)getOwnPropertyIfExists(95-95)src/bun.js/bindings/webcore/WebSocket.cpp (18)
url(1100-1103)url(1100-1100)create(294-297)create(294-294)create(299-302)create(299-299)create(304-319)create(304-304)create(320-336)create(320-320)create(338-355)create(338-338)create(357-375)create(357-357)create(377-380)create(377-377)protocol(1115-1118)protocol(1115-1115)
src/js/thirdparty/undici.js (1)
test/js/web/websocket/websocket-proxy.test.ts (2)
fetch(40-45)fetch(65-70)
🔇 Additional comments (13)
src/bun.js/bindings/webcore/JSWebSocket.cpp (5)
70-72: LGTM: Extern declaration for Zig binding.The extern "C" declaration correctly exposes the Zig-side
getHttpsGlobalAgentfunction for use in C++.
152-238: Well-structured helper for globalAgent fallback.The
applyGlobalAgentFallbackfunction properly:
- Checks exception state after each property access via
RETURN_IF_EXCEPTION- Handles proxy URL extraction from both string and URL objects (via
.href)- Follows correct TLS source priority:
options→connectOpts→connect- Only modifies
rejectUnauthorizedandsslConfigwhen not already set (respecting per-request overrides)The sentinel value
-1forrejectUnauthorizedcleanly distinguishes "not set" from explicitfalse(0) ortrue(1).
240-279: LGTM: constructJSWebSocket1 with globalAgent fallback.The 1-argument constructor path now correctly applies globalAgent fallback for proxy and TLS options. The conditional
WebSocket::createcall properly handles the tri-staterejectUnauthorizedvalue.
281-323: LGTM: constructJSWebSocket2 with globalAgent fallback.The 2-argument constructor (URL + single protocol string) correctly converts the protocol to a
Vector<String>and applies the globalAgent fallback, maintaining consistency with the other constructor paths.
491-526: LGTM: Proper fallback ordering in constructJSWebSocket3.The 3-argument constructor correctly:
- First extracts TLS options from explicit
tlsoption- Then checks
agent.connectOpts/agent.optionsif proxy wasn't explicitly set- Finally falls back to
https.globalAgentviaapplyGlobalAgentFallbackThis ensures per-request options take precedence over agent options, which take precedence over globalAgent options.
test/js/first_party/undici/undici.test.ts (1)
159-276: Comprehensive test coverage for undici TLS option storage.The tests thoroughly verify that:
- All dispatcher types (Agent, Dispatcher, Pool, BalancedPool, Client, ProxyAgent, EnvHttpProxyAgent, RetryAgent) correctly store
optionsandconnectproperties- Edge cases are covered: Agent without options has
undefinedfor both, Agent with options but noconnectonly hasoptionsThe test structure follows best practices by importing undici components at the describe level and using shared
connectOptsfor consistency.test/js/node/tls/globalAgent-tls-options.test.ts (3)
1-12: Excellent test organization and setup.The test file correctly:
- Uses
tempDirfrom harness for multi-file test scenarios (per coding guidelines)- Loads TLS fixtures at module level for reuse across tests
- Organizes tests into logical describe blocks by feature area
15-73: Well-designed baseline test for globalAgent.options.rejectUnauthorized.The test properly:
- Uses
port: 0for dynamic port allocation- Uses
await usingfor automatic process cleanup- Awaits stdout before checking exit code (better error messages)
- Sets up a self-signed cert server and verifies globalAgent fallback works
679-1102: Comprehensive undici integration tests.The tests thoroughly cover:
undici.Agent,Pool,Client,ProxyAgentwith Bun's fetch viadispatcheroptionundici.fetchusinghttps.globalAgent.optionsas fallbackundici.fetchwith explicit dispatcher overriding globalAgentsetGlobalDispatcheraffecting subsequentundici.fetchcallsThis provides excellent coverage for the TLS option propagation through the undici compatibility layer.
src/js/thirdparty/undici.js (3)
10-19: Verify fetch wrapper handles all edge cases correctly.The wrapper correctly falls back to
globalDispatcherwhen:
globalDispatcheris set- No explicit
dispatcherin init- No explicit
agentin initHowever, consider edge case: if
initisundefinedornull, accessinginit?.dispatcheris safe, but spreading{ ...init, dispatcher: globalDispatcher }withinit = undefinedis also safe in JS.One minor observation: the wrapper uses standard
Bun.fetchreference which is correct, but per coding guidelines forsrc/js/thirdparty/, consider ifbunFetch.$call(undefined, ...)pattern is needed to prevent user tampering. Given this is a thirdparty shim and fetch is a global, the current approach is acceptable.
275-298: Well-designed non-enumerable storage for TLS options.Using
Object.definePropertywithenumerable: falseis the correct approach to:
- Store options for Bun's fetch to extract TLS settings
- Avoid polluting object enumeration (e.g.,
Object.keys(),for...in)- Allow the properties to be writable and configurable for flexibility
The comment clearly explains this differs from upstream undici, which is helpful for maintainability.
345-350: RetryAgent correctly stores dispatcher reference.The
RetryAgentproperly:
- Calls
super(options)to store options/connect via Dispatcher- Stores the wrapped
dispatcherreference for delegationThis matches the test expectations in
undici-properties.test.tsthat verifyretryAgent.dispatcher === dispatcher.src/js/node/_http_client.ts (1)
63-74: The fallback chain is sound and follows established patterns in the codebase.The implementation correctly handles multiple agent types (standard HTTP Agent, https-proxy-agent, undici). The optional chaining (
?.connect?.[key]) safely handles cases where intermediate properties don't exist, and type validation occurs after retrieval for each TLS option. This mirrors the pattern already used insrc/js/thirdparty/ws.js(line 118), confirming it's an established convention. The defensive design means even if agent structures vary, the code gracefully falls back toundefinedrather than throwing errors.
cirospaciari
commented
Jan 9, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/bun.js/bindings/bindings.cpp:
- Around line 6125-6150: The function JSC__JSGlobalObject__getHttpsGlobalAgent
can clear a pre-existing exception via
catchScope.clearExceptionExceptTermination(); capture the VM's existing
exception before your operations (e.g., auto* prevException = vm.exception() or
save vm.exception()) and only call clearExceptionExceptTermination() (or
otherwise clear the catchScope) when there was no pre-existing exception; i.e.,
replace unconditional clearExceptionExceptTermination() calls with logic that
preserves prevException and only clears the scope if prevException is null,
ensuring you don't clobber an upstream pending exception in
JSC__JSGlobalObject__getHttpsGlobalAgent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/bindings.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/bindings.cpp
🧠 Learnings (21)
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-02T18:09:21.647Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:79-85
Timestamp: 2025-09-02T18:09:21.647Z
Learning: In Bun's Zig codebase, prefer simple, direct error handling patterns. When an error type has only one variant (like FreeError with only NotAllocated), use a switch statement rather than generic catch blocks to leverage compile-time knowledge and avoid unnecessary complexity. Avoid duplicating function calls across error paths when a cleaner structure is possible.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2026-01-05T16:32:15.175Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:15.175Z
Learning: In Bun's Zig codebase, std.posix.sigaction returns void (not an error union) and should be called without error handling. The Zig standard library treats sigaction failures as programmer errors (unreachable) since they only occur with invalid signals like SIGKILL/SIGSTOP. This pattern is used consistently across crash_handler.zig, main.zig, filter_run.zig, and process.zig.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-23T06:50:41.142Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-18T20:59:45.579Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:458-475
Timestamp: 2025-10-18T20:59:45.579Z
Learning: In src/bun.js/telemetry.zig, the RequestId (u64) to JavaScript number (f64) conversion in jsRequestId() is intentionally allowed to lose precision beyond 2^53-1. This is acceptable because: (1) at 1M requests/sec it takes ~285 years to overflow, (2) the counter resets per-process, and (3) these are observability IDs, not critical distributed IDs. Precision loss is an acceptable trade-off for this use case.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.zig : In Zig code, be careful with allocators and use defer for cleanup
Applied to files:
src/bun.js/bindings/bindings.cpp
- Add array support to ALPNProtocols union for compatibility with HttpsProxyAgent which sets ALPNProtocols to ['http/1.1'] - Convert array strings to ALPN wire format (length-prefixed protocols) - Use stack fallback allocator to avoid heap allocations for typical cases - Use toUTF8() for proper Windows UTF-16 string handling - Fix exception handling in getHttpsGlobalAgent to not clobber pre-existing exceptions from upstream code
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/bun.js/bindings/bindings.cpp:
- Around line 6125-6154: The binding currently swallows any pre-existing
exception by returning undefined; change the initial check in
JSC__JSGlobalObject__getHttpsGlobalAgent (the if (catchScope.exception()) block)
to propagate the pending exception back to Zig by returning the encoded
exception (i.e., return JSValue::encode(catchScope.exception())) or otherwise
rethrowing it so the Zig caller sees the JSError rather than losing it. Also
update the comment near the identifier creation for globalAgent ("(reuse
identifier)") to accurately reflect that a fresh Identifier is created each call
(remove or rephrase the comment) or implement caching if reuse is intended.
In @test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts:
- Line 16: The THRESHOLD of 60 makes the lower-bound assertion ineffective for
small timeout values; update the test in fetch-tls-abortsignal-timeout.test.ts
to compute the lower bound as Math.max(0, timeout - THRESHOLD) (or remove tiny
timeout cases) before asserting diff >= lowerBound, so the check is meaningful
for timeouts like 0, 1, and 10; locate the THRESHOLD constant and the assertion
that compares diff against timeout - THRESHOLD and replace the literal
subtraction with the conditional Math.max approach (or prune the test timeouts
to start at a larger minimum such as 100ms).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/api/server/SSLConfig.bindv2.tssrc/bun.js/api/server/SSLConfig.zigsrc/bun.js/bindings/bindings.cpptest/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/bindings.cpp
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/api/server/SSLConfig.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/api/server/SSLConfig.zig
🧠 Learnings (32)
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/api/server/SSLConfig.bindv2.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/api/server/SSLConfig.bindv2.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-02T18:09:21.647Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:79-85
Timestamp: 2025-09-02T18:09:21.647Z
Learning: In Bun's Zig codebase, prefer simple, direct error handling patterns. When an error type has only one variant (like FreeError with only NotAllocated), use a switch statement rather than generic catch blocks to leverage compile-time knowledge and avoid unnecessary complexity. Avoid duplicating function calls across error paths when a cleaner structure is possible.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2026-01-05T16:32:15.175Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:15.175Z
Learning: In Bun's Zig codebase, std.posix.sigaction returns void (not an error union) and should be called without error handling. The Zig standard library treats sigaction failures as programmer errors (unreachable) since they only occur with invalid signals like SIGKILL/SIGSTOP. This pattern is used consistently across crash_handler.zig, main.zig, filter_run.zig, and process.zig.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use JSC::WriteBarrier for heap-allocated references in V8 objects and implement visitChildren() for custom heap objects to support garbage collection
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.zig : In Zig code, be careful with allocators and use defer for cleanup
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-12-23T06:50:41.142Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-10-18T01:49:31.037Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/SocketConfig.bindv2.ts:58-58
Timestamp: 2025-10-18T01:49:31.037Z
Learning: In Bun's bindgenv2 TypeScript bindings (e.g., src/bun.js/api/bun/socket/SocketConfig.bindv2.ts), the pattern `b.String.loose.nullable.loose` is intentional and not a duplicate. The first `.loose` applies to the String type (loose string conversion), while the second `.loose` applies to the nullable (loose nullable, treating all falsy values as null rather than just null/undefined).
Applied to files:
src/bun.js/api/server/SSLConfig.bindv2.ts
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/api/server/SSLConfig.bindv2.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
src/bun.js/api/server/SSLConfig.bindv2.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
src/bun.js/api/server/SSLConfig.bindv2.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met
Applied to files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts
📚 Learning: 2025-09-02T17:07:29.377Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:873-879
Timestamp: 2025-09-02T17:07:29.377Z
Learning: In Zig's bun codebase, in functions like `unpackNullable` that return `?Allocator`, anonymous method calls like `.get()` are preferred over fully qualified calls like `Nullable(Allocator).get(allocator)` because the compiler can infer the correct type from the return type context. This follows Zig's anonymous literal syntax conventions.
Applied to files:
src/bun.js/api/server/SSLConfig.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/api/server/SSLConfig.zig
🔇 Additional comments (3)
src/bun.js/api/server/SSLConfig.bindv2.ts (1)
21-21: LGTM!The array variant addition is correct and follows the established pattern used in
SSLConfigFile. This allows users to specify ALPN protocols as an array of strings.src/bun.js/api/server/SSLConfig.zig (2)
327-342: ALPN wire format construction is correct.The buffer allocation and wire format construction properly implements the ALPN protocol format (length-prefixed strings). The
@intCastat line 335 is safe due to the prior length validation, and memory management uses appropriate cleanup patterns.Note: This code will work correctly once the cleanup bug in lines 304-325 is fixed.
302-303: Good use of stack fallback allocator.The stack allocation of 8 slots for
ZigString.Sliceis well-sized for typical ALPN usage (1-3 protocols), with automatic heap fallback for edge cases.
…ance Split globalAgent-tls-options.test.ts (19 tests) into 3 smaller files: - globalAgent-https-request.test.ts (4 tests) - globalAgent-fetch.test.ts (7 tests) - globalAgent-undici.test.ts (8 tests) Changes: - Use bunRun (spawnSync) instead of async Bun.spawn for simpler code - Add describe.concurrent for parallel test execution within files - Reduces ASAN build timeout issues by splitting subprocess spawns
| } | ||
|
|
||
| // Allocate buffer with null terminator | ||
| const buf = try bun.default_allocator.allocSentinel(u8, total_len, 0); |
Collaborator
There was a problem hiding this comment.
Where does this memory get freed?
Member
Author
There was a problem hiding this comment.
when SSLConfig deinit we free protos
Collaborator
|
Closing per IRL discussion |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support for using
https.globalAgent.optionsas default TLS settings across fetch, node:https, and WebSocket APIs. When applications set TLS properties on the global agent, those settings now automatically apply unless overridden per-request.Features
https.globalAgentagent.options,agent.connectOpts(https-proxy-agent), andagent.connect(undici.Agent)https.globalAgent.proxywhen no explicit proxy is setca,cert,key,passphrase,rejectUnauthorizedExample Usage
Changes
Core implementation:
src/bun.js/webcore/fetch.zig- TLS fallback logic for fetchsrc/bun.js/bindings/webcore/JSWebSocket.cpp- TLS and proxy fallback for WebSocketsrc/js/node/_http_client.ts- TLS fallback for node:httpsInfrastructure:
src/bun.js/bindings/JSGlobalObject.zig- Zig binding to access global agentsrc/bun.js/bindings/bindings.cpp- C++ wrapper for agent retrievalsrc/js/thirdparty/undici.js- Store dispatcher/agent TLS optionsTests:
test/js/node/tls/globalAgent-tls-options.test.ts- Integration tests for all APIstest/js/web/websocket/websocket-proxy.test.ts- WebSocket globalAgent teststest/js/first_party/undici/undici-properties.test.ts- Property storage validation