js: update node:https Agent #24426

nektro · 2025-11-06T02:31:55Z

pulled out of #21809

robobun · 2025-11-06T02:32:03Z

^{Updated 6:06 PM PT - Nov 6th, 2025}

❌ @nektro, your commit 05d2de9 has 4 failures in Build #31183 (All Failures):

test/v8/v8.test.ts - 1 failing on 🐧 12 x64-asan
test/regression/issue/24399.test.ts - core dumped on 🐧 12 x64-asan
test/bake/dev/stress.test.ts - 1 failing on 🐧 12 x64-asan
scripts/build.mjs - build failed on 🍎 aarch64 - build-cpp
scripts/build.mjs - build failed on 🍎 x64 - build-cpp

🧪 To try this PR locally:

bunx bun-pr 24426

That installs a local version of the PR into your bun-24426 executable, so you can run:

bun-24426 --bun

coderabbitai

Actionable comments posted: 7

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 86a0ff4 and 8ebfd8c.

📒 Files selected for processing (13)

src/bun.js/bindings/ErrorCode.cpp (2 hunks)
src/bun.js/bindings/ErrorCode.ts (1 hunks)
src/codegen/bundle-modules.ts (1 hunks)
src/js/builtins.d.ts (2 hunks)
src/js/internal/url.ts (1 hunks)
src/js/node/_http_agent.ts (3 hunks)
src/js/node/_http_client.ts (3 hunks)
src/js/node/_http_incoming.ts (1 hunks)
src/js/node/_http_outgoing.ts (1 hunks)
src/js/node/https.ts (3 hunks)
test/js/node/test/parallel/test-https-agent-getname.js (1 hunks)
test/js/node/test/parallel/test-https-foafssl.js (1 hunks)
test/js/node/test/parallel/test-https-options-boolean-check.js (1 hunks)

🧰 Additional context used

📓 Path-based instructions (14)

src/js/node/**/*.{ts,js}

📄 CodeRabbit inference engine (src/js/CLAUDE.md)

Place Node.js compatibility modules (e.g., node:fs, node:path) under node/

Files:

src/js/node/_http_incoming.ts
src/js/node/_http_agent.ts
src/js/node/_http_client.ts
src/js/node/_http_outgoing.ts
src/js/node/https.ts

src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}

📄 CodeRabbit inference engine (src/js/CLAUDE.md)

src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)

Files:

src/js/node/_http_incoming.ts
src/js/node/_http_agent.ts
src/js/node/_http_client.ts
src/js/internal/url.ts
src/js/node/_http_outgoing.ts
src/js/node/https.ts

test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

test/js/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place JavaScript and TypeScript tests under test/js/

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

test/js/node/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

test/js/node/test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

test/js/node/test/{parallel,sequential}/*.js

📄 CodeRabbit inference engine (test/CLAUDE.md)

For test/js/node/test/{parallel,sequential}/*.js (no .test extension), run with bun bd <file> instead of bun bd test <file>

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

test/**/*.{ts,tsx,js,jsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

For large/repetitive strings, use Buffer.alloc(count, fill).toString() instead of "A".repeat(count)

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

test/js/{bun,node}/**

📄 CodeRabbit inference engine (test/CLAUDE.md)

Organize unit tests by module under /test/js/bun/ and /test/js/node/

Files:

test/js/node/test/parallel/test-https-foafssl.js
test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

**/*.{cpp,h}

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible

Files:

src/bun.js/bindings/ErrorCode.cpp

**/*.cpp

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue

Files:

src/bun.js/bindings/ErrorCode.cpp

src/bun.js/bindings/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

src/bun.js/bindings/**/*.cpp: When implementing a JavaScript class with a public constructor in C++, create Foo, FooPrototype, and FooConstructor classes
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields in JavaScriptCore bindings

Files:

src/bun.js/bindings/ErrorCode.cpp

src/js/internal/**/*.{ts,js}

📄 CodeRabbit inference engine (src/js/CLAUDE.md)

Place internal-only modules under internal/

Files:

src/js/internal/url.ts

🧠 Learnings (25)

📓 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: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: For Node.js compatibility bugs, validate expected behavior in Node.js first, then confirm failure on Bun canary

📚 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/js/node/_http_incoming.ts
src/js/node/_http_agent.ts
src/js/node/_http_client.ts
src/js/node/_http_outgoing.ts

📚 Learning: 2025-10-19T04:55:27.213Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-88
Timestamp: 2025-10-19T04:55:27.213Z
Learning: In the Bun.telemetry._node_binding API for Node.js http.createServer compatibility, there are exactly two hooks: handleIncomingRequest(req, res) called once per request (returns a request ID), and handleWriteHead(res, statusCode) called once when headers are sent (explicit or implicit). The old handleRequestFinish hook no longer exists after architectural refactoring. Both hooks are tested in test/js/bun/http/node-telemetry.test.ts.

Applied to files:

src/js/node/_http_incoming.ts

📚 Learning: 2025-10-04T09:52:49.414Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Do not use ESM import syntax; write modules as CommonJS with export default { ... }

Applied to files:

src/codegen/bundle-modules.ts
src/js/node/_http_agent.ts

📚 Learning: 2025-10-04T09:52:49.414Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Export via export default {} for modules

Applied to files:

src/codegen/bundle-modules.ts

📚 Learning: 2025-10-04T09:52:49.414Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)

Applied to files:

src/codegen/bundle-modules.ts
src/js/node/_http_agent.ts

📚 Learning: 2025-10-04T09:52:49.414Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use require() with string literals only (no dynamic requires)

Applied to files:

src/codegen/bundle-modules.ts

📚 Learning: 2025-08-30T00:09:39.100Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests

Applied to files:

src/codegen/bundle-modules.ts

📚 Learning: 2025-10-04T09:52:49.414Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/node/**/*.{ts,js} : Place Node.js compatibility modules (e.g., node:fs, node:path) under node/

Applied to files:

src/codegen/bundle-modules.ts

📚 Learning: 2025-10-19T02:52:37.412Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/tsconfig.json:1-15
Timestamp: 2025-10-19T02:52:37.412Z
Learning: In the Bun repository, packages under packages/ (e.g., bun-otel) can follow a TypeScript-first pattern where package.json exports point directly to .ts files (not compiled .js files). Bun natively runs TypeScript, so consumers import .ts sources directly and receive full type information without needing compiled .d.ts declaration files. For such packages, adding "declaration": true or "outDir" in tsconfig.json is unnecessary and would break the export structure.
<!-- [remove_learning]
ceedde95-980e-4898-a2c6-40ff73913664

Applied to files:

src/codegen/bundle-modules.ts

📚 Learning: 2025-08-30T00:11:57.076Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/{js_bindings,BunObject}.classes.ts : Add JSDoc comments to JavaScript binding class definitions

Applied to files:

src/codegen/bundle-modules.ts
src/js/node/_http_agent.ts

📚 Learning: 2025-10-04T09:52:49.414Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/internal/**/*.{ts,js} : Place internal-only modules under internal/

Applied to files:

src/codegen/bundle-modules.ts

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes

Applied to files:

src/codegen/bundle-modules.ts
src/bun.js/bindings/ErrorCode.ts

📚 Learning: 2025-10-12T02:22:34.373Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Never hardcode port numbers; use `port: 0` to get a random port

Applied to files:

src/js/node/_http_agent.ts
test/js/node/test/parallel/test-https-agent-getname.js

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions

Applied to files:

src/bun.js/bindings/ErrorCode.cpp
src/bun.js/bindings/ErrorCode.ts

📚 Learning: 2025-08-30T00:11:57.076Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions

Applied to files:

src/bun.js/bindings/ErrorCode.cpp

📚 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/bindings/ErrorCode.cpp
src/bun.js/bindings/ErrorCode.ts

📚 Learning: 2025-10-18T23:43:42.502Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23817
File: src/js/node/test.ts:282-282
Timestamp: 2025-10-18T23:43:42.502Z
Learning: In the Bun repository, the error code generation script (generate-node-errors.ts) always runs during the build process. When reviewing code that uses error code intrinsics like $ERR_TEST_FAILURE, $ERR_INVALID_ARG_TYPE, etc., do not ask to verify whether the generation script has been run or will run, as it is automatically executed.

Applied to files:

src/bun.js/bindings/ErrorCode.cpp
src/bun.js/bindings/ErrorCode.ts

📚 Learning: 2025-09-08T00:41:12.052Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add new V8 API mangled symbols (without leading underscore) to src/symbols.txt

Applied to files:

src/bun.js/bindings/ErrorCode.cpp
src/bun.js/bindings/ErrorCode.ts

📚 Learning: 2025-10-12T02:22:34.373Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Always check exit codes and error scenarios in tests (e.g., spawned processes should assert non-zero on failure)

Applied to files:

test/js/node/test/parallel/test-https-options-boolean-check.js

📚 Learning: 2025-09-08T00:41:12.052Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Follow existing patterns in similar V8 classes, add comprehensive Node.js parity tests, update all symbol files, and document any special behavior

Applied to files:

test/js/node/test/parallel/test-https-options-boolean-check.js
test/js/node/test/parallel/test-https-agent-getname.js

📚 Learning: 2025-10-03T18:29:20.173Z

Learnt from: alii
Repo: oven-sh/bun PR: 22504
File: src/bake/client/data-view.ts:4-4
Timestamp: 2025-10-03T18:29:20.173Z
Learning: In the Bun codebase, DataView<ArrayBuffer> is valid TypeScript syntax (supported since TypeScript 4.5+) and is used consistently across the bake client modules to provide type narrowing for DataView instances backed specifically by ArrayBuffer rather than SharedArrayBuffer.

Applied to files:

test/js/node/test/parallel/test-https-options-boolean-check.js

📚 Learning: 2025-10-04T09:52:49.414Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type

Applied to files:

test/js/node/test/parallel/test-https-options-boolean-check.js

📚 Learning: 2025-10-06T23:32:44.491Z

Learnt from: alii
Repo: oven-sh/bun PR: 22504
File: packages/bun-framework-react/src/client/app.ts:10-21
Timestamp: 2025-10-06T23:32:44.491Z
Learning: In TypeScript 5.7 and later (including 5.9), TypedArray types like Uint8Array, Uint16Array, etc. are generic over their underlying ArrayBuffer type. The syntax Uint8Array<ArrayBuffer> is valid and correct, not an error.

Applied to files:

test/js/node/test/parallel/test-https-options-boolean-check.js

📚 Learning: 2025-08-30T00:12:56.803Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)

Applied to files:

test/js/node/test/parallel/test-https-agent-getname.js

🧬 Code graph analysis (1)

src/js/node/https.ts (2)

test/js/node/test/parallel/test-https-agent-getname.js (2)

options (25-47)

agent (10-10)

src/js/internal/http.ts (3)

shouldUseProxy (403-443)

checkShouldUseProxy (489-489)

kEmptyObject (520-520)

🪛 Biome (2.1.2)

src/js/node/https.ts

[error] 305-305: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)

🔇 Additional comments (21)

src/js/node/_http_incoming.ts (1)

1-2: Documentation comment for module hardcoding — approved.

The added comment serves as metadata identifying this module as the hardcoded node:_http_incoming implementation. This purely documentational change introduces no functional modifications, maintains all existing behavior, and aligns with the pattern applied across the PR.

src/js/node/_http_client.ts (2)

1-1: LGTM: Informative module identifier.

The comment clearly identifies this as the hardcoded module for "node:_http_client".

914-920: LGTM: Simplified debug messages.

The debug warnings are now plain string literals, clearly indicating these methods are no-ops.

src/js/node/_http_outgoing.ts (1)

1-1: LGTM: Module identifier comment added.

The hardcoded module comment serves as metadata for the module registry system and has no runtime impact.

src/js/internal/url.ts (2)

22-24: LGTM: URL detection helper added.

The isURL helper correctly distinguishes URL objects from plain options objects by checking for href and protocol presence while ensuring auth and path are undefined (URL objects have username/password and pathname instead).

26-28: Verify and clarify the purpose of isURL export.

The new isURL helper is exported from internal/url but has no production usage. It only appears in disabled tests (test.skip and commented-out code). Production code imports only urlToHttpOptions from this module.

Either remove isURL if it's not needed, enable the tests and integrate it into production code, or document its purpose if intentionally left as a placeholder.

src/js/node/_http_agent.ts (2)

1-1: LGTM: Module identifier comment added.

Consistent with the pattern used across other Node.js compatibility modules.

151-151: Good practice: Replaced @ts-ignore with @ts-expect-error.

Using @ts-expect-error is preferred as it will fail if the type error is resolved, preventing stale suppressions.

Also applies to: 160-160, 163-163

src/codegen/bundle-modules.ts (1)

514-514: LGTM: Added type mapping for node:util/types.

The new type declaration follows the existing pattern and enables proper TypeScript support for the node:util/types module.

src/js/node/https.ts (5)

1-8: LGTM: Imports updated for proxy tunnel support.

The new imports provide the necessary utilities for implementing HTTPS proxy tunneling via the CONNECT protocol.

369-398: LGTM: Session caching implementation.

The session management methods (_getSession, _cacheSession, _evictSession) implement a simple LRU-like cache:

Respects maxCachedSessions limit

Evicts oldest entries when cache is full

Handles cache updates for existing keys

400-407: LGTM: Global agent configuration.

The global agent is configured with sensible defaults for HTTPS, consistent with the HTTP agent pattern.

409-416: LGTM: Updated export to include globalAgent as named export.

The export now includes globalAgent as a named export alongside the default export, improving API ergonomics.

15-15: Change from const to let is correct and necessary.

The variable options is reassigned within the request function on lines 19 and 21 based on conditional logic, making let the appropriate keyword. This change allows the variable to be updated after initialization, which const would prohibit.

test/js/node/test/parallel/test-https-options-boolean-check.js (1)

154-154: LGTM: Added trailing newline.

This is a standard file hygiene improvement with no functional impact.

src/bun.js/bindings/ErrorCode.ts (1)

212-212: LGTM: Added ERR_PROXY_TUNNEL error mapping.

The new error code follows the existing pattern and is used in the HTTPS proxy tunnel implementation (src/js/node/https.ts lines 132, 169, 180, 251, 264).

src/bun.js/bindings/ErrorCode.cpp (2)

2385-2394: LGTM: ERR_HTTP_CONTENT_LENGTH_MISMATCH implementation is correct.

The error constructor properly extracts two arguments, converts them to strings with exception handling, and constructs a clear, descriptive error message explaining the content-length mismatch.

2532-2533: LGTM: ERR_HTTP_TRAILER_INVALID implementation is correct.

The error constructor follows the simple fixed-message pattern used for other argumentless error codes in this file.

src/js/builtins.d.ts (2)

769-770: LGTM: Error constructor declarations are correct.

The new error constructor declarations follow the existing pattern and are consistent with their implementations:

$ERR_HTTP_CONTENT_LENGTH_MISMATCH matches the C++ implementation that takes two number arguments

$ERR_PROXY_TUNNEL follows the standard single-message pattern

842-842: LGTM: ERR_HTTP_TRAILER_INVALID declaration is correct.

The declaration correctly matches the C++ implementation, which takes no arguments and returns a fixed error message.

test/js/node/test/parallel/test-https-agent-getname.js (1)

1-54: No issues found—test assertions match Node.js behavior exactly.

Verification against Node.js confirms all three test cases produce the expected output:

Empty case and empty object both produce "localhost::::::::::::::::::::::"

Full options case produces the exact multi-segment string the test asserts

The format is not fragile or incorrect—it faithfully captures Node.js's documented output for Agent.getName().

src/js/node/_http_client.ts

src/js/node/https.ts

test/js/node/test/parallel/test-https-foafssl.js

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

cirospaciari · 2025-11-07T22:43:06Z

src/js/node/https.ts

+//
+// This function computes the tunnel configuration for HTTPS requests.
+// The handling of the tunnel connection is done in createConnection.
+function getTunnelConfigForProxiedHttps(agent, reqOptions) {


we need tests for this and for createConnection before merging

cirospaciari

the code looks ok but we need more tests, ERR_HTTP_CONTENT_LENGTH_MISMATCH, ERR_PROXY_TUNNEL, ERR_HTTP_TRAILER_INVALID, createConnection and the proxy tunnel stuff need to be tested before merging the changes

js: update node:https Agent

5f87c5c

oops

8ebfd8c

nektro marked this pull request as ready for review November 7, 2025 01:12

nektro requested a review from alii as a code owner November 7, 2025 01:12

nektro requested a review from cirospaciari November 7, 2025 01:14

This comment was marked as duplicate.

Sign in to view

coderabbitai bot reviewed Nov 7, 2025

View reviewed changes

Update test/js/node/test/parallel/test-https-foafssl.js

05d2de9

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

cirospaciari reviewed Nov 7, 2025

View reviewed changes

cirospaciari requested changes Nov 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

js: update node:https Agent #24426

js: update node:https Agent #24426

Uh oh!

nektro commented Nov 6, 2025 •

edited

Loading

Uh oh!

robobun commented Nov 6, 2025 •

edited

Loading

Uh oh!

This comment was marked as duplicate.

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cirospaciari Nov 7, 2025

Uh oh!

cirospaciari left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

js: update node:https Agent #24426

Are you sure you want to change the base?

js: update node:https Agent #24426

Uh oh!

Conversation

nektro commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as duplicate.

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cirospaciari Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

cirospaciari left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nektro commented Nov 6, 2025 •

edited

Loading

robobun commented Nov 6, 2025 •

edited

Loading