[robobun]

Fixes #32103

Problem

• This kills a release build:

  Array.of.call(require("util").types.isDate, 1, 2, 3);

  Release 1.4.3: panic(main thread): Segmentation fault at address 0xA. Asserts build: ASSERTION FAILED: isCell(), JSCJSValue.h:766.
• Five registration sites install a call function as the native [[Construct]] handler, so a construct reads a primitive as an object. The widest is putNativeFn (src/jsc/modules/_NativeModule.h), 81 exports.
• The same functions print as classes: [class hostname] (OS Module os["class"] gives wrong output #32103).

Fix

• The sites pass no constructor, so new throws TypeError: putNativeFn, four node:buffer functions, utf-8-validate, node:sqlite backup, and JSFFIFunction::create and createForFFI (node:os, bun:ffi).
• fix(node:module): don't register native helpers as their own constructors #31393 and bun:test: fix SIGSEGV when constructing an expect.extend() matcher #31219 applied the same remedy.
• Self-reviewed: 7 concerns raised, 7 addressed.
• Verified: spawned tests in test-util-types.test.js and os.test.js exit 134 with src/ at main. Also ffi, bun-jsc, buffer.

Background

• A native [[Construct]] handler must return an object, which JSC does not check.
• Array.of, Array.from and Symbol.species construct the callee.
• Considered constructing natives like ordinary JS functions. That matches Node and needs a prototype on each.

Downsides

• Needs a maintainer decision. new on these functions returned the call's result on main and now throws. fix(buffer): use correct constructor for buffer.isAscii #22480 kept the isAscii constructor as the less breaking choice. Five return an object under new on main and in Node, for example os.loadavg.
• bun:ffi CString is not changed. Array.from.call(CString, [1, 2, 3]) still aborts. Its constructor returns a string by contract.
• Speed and size: no change. An isDate call takes 639.0 instructions and .text is 58,143,989 bytes on both builds.

Notes

What changed in this revision. This pull request also carried a construct handler for bun:test mocks and the mock.instances change. #41398 covers both, so they are removed here. The removed commit is kept on the branch robobun/e9fbc481/mock-construct-half (13cae76). Array.of.call(mock(), 1, 2, 3) keeps its segfault until a mock change lands.

The decision to make. On main new f() on these functions runs the call function and evaluates to its result: new os.hostname() is a string, new buffer.isAscii(buf) is a boolean, new os.loadavg() is the array. #22480 noted that removal of the isAscii and isUtf8 constructors "would be more breaking than correcting the current bug" and added expect(new isAscii(...)).toBeFalse() to test/js/node/buffer.test.js. This pull request changes that line to expect a TypeError. #31393 made the opposite choice for module._compile, Module._resolveFilename, Module.runMain and performance.now. The five functions that return an object under new on main and in Node v26.3.0 are os.loadavg, os.userInfo, os.networkInterfaces, buffer.transcode and sqlite.backup.

Node v26.3.0, which functions new accepts. Checked with Reflect.construct(Object, [], f):

	Node	main	this branch
os.freemem, release, totalmem, version	not a constructor	constructor	not a constructor
the other 8 native os functions	constructor	constructor	not a constructor
buffer.isUtf8, isAscii, transcode, resolveObjectURL	constructor	constructor	not a constructor
sqlite.backup	constructor	constructor	not a constructor
util.types.*	14 of 43	44 of 44	0 of 44

Node writes those 27 functions in JS, so they are ordinary functions there. In Node new os.hostname() returns an empty wrappedFn object and Array.of.call(os.hostname, 1, 2, 3) returns that object with 0, 1, 2 and length set. On main the first returns the string and the second aborts.

bun:ffi CString. docs/runtime/ffi.mdx says new CString() returns a normal string, and constructFFICString (src/jsc/bindings/JSFFICString.cpp) returns one. So it cannot drop its constructor, and this pull request does not change it. A release build of main (367d939d9a) aborts on Array.from.call(CString, [1, 2, 3]) and on a Symbol.species filter. A debug build also fails the asObject assertion on Reflect.construct(CString, [ptr(buffer)]). The release build returns the string there. Array.of.call(CString, 1, 2, 3) segfaults at address 0x3 for a different reason: Array.of constructs with the length 3, and CString reads 3 as a pointer.

How the numbers were measured. Release builds of main 36cd1514ec and of this branch 6c4214a189, same machine. perf and valgrind are not available in the build container (perf_event_open returns EPERM), so instructions were counted with a ptrace single-step counter: it runs the process at full speed to a marker syscall, single-steps the main thread to a second marker, and counts the steps. BUN_JSC_useJIT=0, 500 warm-up calls, Bun.gc(true), then the window. A per-call figure is the difference between windows of 1000 and 3000 calls, divided by 2000.

	main	this branch
util.types.isDate(date), window of 1000 / 3000 calls	670,760 / 1,948,760	670,760 / 1,948,760
buffer.isUtf8(buf), window of 1000 / 3000 calls	686,760 / 1,996,760	686,760 / 1,996,760
size: text, data, bss	80,661,004 / 110,424 / 1,822,992	80,661,004 / 110,424 / 1,822,992

Other open pull requests. #43802 rewrites createForFFI and passes trampoline as the constructor again. The cc() test that this pull request adds to ffi.test.js fails on that line.

Local test runs on a debug ASAN build, on a machine with a load average above 400. test-util-types 55 pass, os 56 pass, bun-jsc 42 pass, utf-8-validate 2 pass, buffer-resolveObjectURL 4 pass, buffer 683 pass. node-sqlite has 4 GC stress tests that time out, and ffi has 10 integer identities tests that time out at 5 s. GC stress > TagStore LRU churn times out the same way with src/ at main. The three tests added to ffi.test.js pass.

---

no test proof · iteration 24 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/sqlite/node-sqlite.test.ts, test/js/node/os/os.test.js, test/js/node/buffer.test.js, test/js/bun/jsc/bun-jsc.test.ts, test/js/bun/ffi/ffi.test.js

[robobun]

^{Updated 12:59 PM PT - Sep 28th, 2026}

❌ @robobun, your commit 6383f7a has 1 failures in Build #121328 (All Failures):

• test/js/bun/s3/s3.test.ts - code 1 on 🐧 13 x64-asan
• test/js/bun/s3/s3.test.ts - code 1 on 🐧 13 x64
• test/js/bun/s3/s3.test.ts - code 1 on 🐧 25.04 x64
• test/js/bun/s3/s3.test.ts - code 1 on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 34575

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

bun-34575 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Changes

The PR updates native constructor wiring, adds JavaScript-compatible construction and instance tracking for mock functions, removes redundant native function creation arguments, and adds coverage for non-constructible native functions and function inspection.

Constructor and host-function semantics

Layer / File(s)	Summary
Native host-function constructor wiring src/jsc/bindings/JSFFIFunction.cpp	FFI host functions now receive the supplied native constructor and use callHostFunctionAsConstructor for construction dispatch.
Mock call and construct behavior src/jsc/bindings/JSMockFunction.cpp, test/js/bun/test/mock-fn.test.js	Mocks use a dedicated construct handler, record thisValue in instances, apply constructor return rules, and support prototype and Reflect.construct behavior.
Native export registration and validation src/jsc/modules/NodeBufferModule.h, src/jsc/modules/NodeSqliteModule.h, src/jsc/modules/UTF8ValidateModule.h, src/jsc/modules/_NativeModule.h, test/js/bun/ffi/ffi.test.js, test/js/bun/jsc/bun-jsc.test.ts, test/js/first_party/utf-8-validate/utf-8-validate.test.ts, test/js/node/buffer-resolveObjectURL.test.ts, test/js/node/buffer.test.js, test/js/node/os/os.test.js, test/js/node/sqlite/node-sqlite.test.ts, test/js/node/util/test-util-types.test.js	Native registrations remove redundant creation arguments. Tests verify that native exports reject construction and use function-style inspection output. The non-Windows username assertion accepts an unset USER environment variable.

Suggested reviewers: dylan-conway

Merge Risk: 🔵 Low · up to 8ab8a

The constructor fixes are low risk, but the child-process regression test can hide unexpected diagnostics when it exits successfully. Tightening the stderr assertion would provide bounded follow-up before merge.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing native functions from being registered as their own constructors.
Description check	✅ Passed	The description provides detailed problem, fix, verification, background, compatibility considerations, and test results. It does not use the template headings exactly, but it contains the required in…

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 1006-1015: Update the fallback construction in the mock
constructor path around newTarget and thisObject so a primitive
newTarget.prototype resolves to newTarget’s realm-aware %Object.prototype%,
rather than lexicalGlobalObject’s prototype. Preserve the explicit
object-prototype path, and add a cross-realm test covering a primitive
newTarget.prototype.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4fa5656d-f25c-42d0-8664-9fc5cf5bf3fe

📥 Commits

Reviewing files that changed from the base of the PR and between f5b73a0 and 8b85bb6.

📒 Files selected for processing (8)

• src/jsc/bindings/JSFFIFunction.cpp
• src/jsc/bindings/JSMockFunction.cpp
• src/jsc/modules/NodeBufferModule.h
• test/js/bun/ffi/ffi.test.js
• test/js/bun/test/mock-fn.test.js
• test/js/node/buffer-resolveObjectURL.test.ts
• test/js/node/buffer.test.js
• test/js/node/os/os.test.js

[claude]

Beyond the inline nit, I also checked: JSFFIFunction::create's only caller (Bun__CreateFFIFunctionWithData) omits nativeConstructor so it picks up the header default callHostFunctionAsConstructor — the os.freemem test exercises exactly that path. The new instances push in jsMockFunctionCallImpl doesn't break the pre-existing stolen.instances / mockClear assertions in the same file. jsMockFunctionConstruct has RETURN_IF_EXCEPTION after each throwing call and thisObject is rooted via the instances/contexts arrays before user JS runs.

Extended reasoning...

This is well-executed and thoroughly tested, but it's non-trivial native JSC binding work with observable API-surface changes (mock.instances now populated on every call to match Jest; every Rust-created host function via JSFFIFunction::create becomes non-constructable), so deferring to a human. The only finding is a nit about an unrelated drive-by test change.

[claude]

Both prior nits are addressed (PR description now justifies the os.test.js $USER fallback, and the FFI subprocess test asserts stderr). I didn't find anything new, but this adds a new native [[Construct]] handler in JSC bindings and changes user-visible semantics (mock.instances is now populated on every call; FFI/node:os/node:buffer natives are no longer constructable), so a maintainer should sign off.

What was reviewed:

• jsMockFunctionConstruct: getFunctionRealm + createSubclassStructure matches the NapiClass/ObjectConstructor pattern; exception checks after each throwing call; thisObject is stack-rooted and stored in instances/contexts before user JS runs.
• JSFFIFunction::create now honors its nativeConstructor param — the only caller omits it and picks up the header default callHostFunctionAsConstructor, covered by the freemem test.
• mock.instances push records thisValue (the created instance, not an object returned by the impl), matching Jest.

Extended reasoning...

Overview

Fixes a fuzzer-found debug assertion / release UB where three native construct handlers could return primitives to asObject(). JSMockFunction gets a dedicated construct handler implementing ordinary-function [[Construct]] semantics; JSFFIFunction::createForFFI and ::create now route construct through callHostFunctionAsConstructor; isUtf8/isAscii/resolveObjectURL drop their redundant nativeConstructor argument. Tests cover all three sites plus mock.instances semantics, cross-realm newTarget.prototype fallback, and function-vs-class inspection.

Security risks

None identified. No untrusted-input parsing, no auth/crypto/permission surface. The construct handler runs user JS via jsMockFunctionCallImpl, but that path already existed for the call handler and the new code adds proper exception propagation around it.

Level of scrutiny

High — this is native C++ in src/jsc/bindings/, the most-blocked category per REVIEW.md. The new jsMockFunctionConstruct interacts with JSC's realm resolution and structure caching, and jsMockFunctionCallImpl now records thisValue into mock.instances on every invocation (a Jest-compat behavior change). JSFFIFunction::create now actually uses its nativeConstructor parameter, which flips every function created through Bun__CreateFFIFunctionWithData (node:os natives, etc.) from "constructable, returns primitive through asObject()" to "throws TypeError on new" — a correct fix, but a behavioral change a maintainer should be aware of.

Other factors

All three prior review threads (CodeRabbit's cross-realm fallback, my os.test.js justification note, my FFI stderr assertion nit) are resolved on the current head (a852cf8). The PR description includes a scope audit of every remaining nativeConstructor site in the tree. Test coverage is thorough — every fixed site has both new X() and Reflect.construct variants, and the mock construct block covers primitive/object returns, newTarget.prototype, throwing impls, spies, and cross-realm. No bugs surfaced in this review pass. Deferring rather than approving because new native construct-handler logic and user-visible semantic changes in bun:test mocks warrant a maintainer glance.

[claude]

All four of my earlier findings are addressed (cross-realm fallback prototype, putNativeFn / utf-8-validate, the stderr assertion, and node:sqlite backup in 1e8bc91) and this pass found nothing new. I'm not auto-approving because the change spans six JSC binding files, adds a new native [[Construct]] handler with realm/prototype logic, and changes mock.instances semantics to match Jest — worth a maintainer's eyes.

What was reviewed:

• jsMockFunctionConstruct: exception scopes after getFunctionRealm/createSubclassStructure/the call impl are all present; object-vs-primitive return matches ordinary [[Construct]].
• JSFFIFunction::create's nativeConstructor param defaults to callHostFunctionAsConstructor in the header and its only caller omits it, so the freemem test covers that path.
• Re-checked for remaining self-as-nativeConstructor JSFunction::create sites after 1e8bc91 — none found beyond those the Scope section already lists as safe.

Extended reasoning...

Overview

The PR eliminates a class of debug-assertion / release-UB bugs where a native [[Construct]] handler returns a primitive to Interpreter::executeConstruct's unchecked asObject(). It touches six C++ files in src/jsc/ (JSFFIFunction.cpp, JSMockFunction.cpp, NodeBufferModule.h, NodeSqliteModule.h, UTF8ValidateModule.h, _NativeModule.h) and nine test files. The mechanical fixes drop the trailing self-as-nativeConstructor argument so those functions become non-constructable. JSMockFunction gets a new dedicated jsMockFunctionConstruct that mirrors ordinary JS constructor semantics via getFunctionRealm + InternalFunction::createSubclassStructure, and jsMockFunctionCallImpl now populates mock.instances on every invocation.

Security risks

None identified. This is a correctness/UB fix in the JSC binding layer; no auth, crypto, permissions, or untrusted-input parsing surface is touched. The change removes a UB path reachable from user JS.

Level of scrutiny

High. Per REVIEW.md, native-code memory safety is "the most-blocked category", and JSMockFunction.cpp adds a new construct handler with exception-scope, realm-resolution, and GC-object-creation logic. The PR also went through three rounds of completeness fixes during review (cross-realm prototype, putNativeFn/utf-8-validate, node:sqlite backup), each catching a real missed sibling — that history alone argues for a maintainer sign-off rather than bot approval.

Other factors

• All four of my prior inline findings have been addressed with code + tests; the last one (NodeSqliteModule.h) landed in 1e8bc91 with a Reflect.construct/Bun.inspect assertion in node-sqlite.test.ts.
• The mock.instances change is a user-visible behavior change (previously never populated; now records thisValue on every call). It matches Jest, is well-tested (instances is pushed on every invocation), and is arguably a bugfix, but it's an API-semantics decision a maintainer should confirm.
• I verified JSFFIFunction.h:72 defaults nativeConstructor to callHostFunctionAsConstructor and the only JSFFIFunction::create caller (Bun__CreateFFIFunctionWithData) omits the argument, so the fix on line 155 does not depend on any caller passing it explicitly.
• Test coverage is thorough: every fixed export has a Reflect.construct throws-TypeError + Bun.inspect assertion, and mock-fn.test.js covers primitive/object returns, newTarget.prototype, cross-realm fallback, throwing implementations, and spy construction.

[robobun]

CI status (build 75341): 285/286 jobs passed. The one red is test-worker-message-port-transfer-terminate.js on debian 13 x64-asan, which ci:errors flags as [pre-existing] (it fails on current main too; an !exception() assertion in JSObject::getOwnPropertyDescriptor via worker termination, unrelated to anything this diff touches). The same test was the only red on build 75189 as well. All nine test files this PR touches passed on every lane.

All bot review findings are addressed as of 1e8bc91:

• CodeRabbit: cross-realm fallback prototype (d9fffe6).
• putNativeFn / UTF8ValidateModule.h sibling sites my first sweep missed (a81c157).
• node:sqlite backup, missed by the second sweep's non-greedy regex (1e8bc91).
• os.test.js $USER drive-by justification and the FFI subprocess stderr assertion.

The final balanced-paren sweep of every 8-arg JSFunction::create in src/ lists 8 remaining sites, all verified safe in the Scope section. Ready for review.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/node/buffer-resolveObjectURL.test.ts`:
- Line 52: Remove the standalone bug-context comment from the test body near the
resolveObjectURL setup; leave the test’s setup, action, and assertions
unchanged.

In `@test/js/node/os/os.test.js`:
- Line 120: Update the username and shell environment fallback assertions in the
relevant OS tests to use nullish coalescing (`?? "unknown"`) instead of
truthiness fallback, preserving explicitly empty values. Add coverage for both
unset and empty USER and SHELL environment values.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f4303628-2670-4edc-9001-dba9a2aab80b

📥 Commits

Reviewing files that changed from the base of the PR and between 9e36e9f and 6947a57.

📒 Files selected for processing (15)

• src/jsc/bindings/JSFFIFunction.cpp
• src/jsc/bindings/JSMockFunction.cpp
• src/jsc/modules/NodeBufferModule.h
• src/jsc/modules/NodeSqliteModule.h
• src/jsc/modules/UTF8ValidateModule.h
• src/jsc/modules/_NativeModule.h
• test/js/bun/ffi/ffi.test.js
• test/js/bun/jsc/bun-jsc.test.ts
• test/js/bun/test/mock-fn.test.js
• test/js/first_party/utf-8-validate/utf-8-validate.test.ts
• test/js/node/buffer-resolveObjectURL.test.ts
• test/js/node/buffer.test.js
• test/js/node/os/os.test.js
• test/js/node/sqlite/node-sqlite.test.ts
• test/js/node/util/test-util-types.test.js

[robobun]

CI status, build 89069: 195/196 jobs passed. The one red lane (debian 13 x64-asan) is an LSan leak report in the RSA keygen threadpool job (EVP_PKEY_keygen via KeyPairJobCtx::runTask, node_crypto_binding.rs:85), attributed to AsyncLocalStorage-tracking.test.ts because that file was running at exit. Nothing in this diff touches crypto, the threadpool, or async_hooks; the leak is handed off to main-break triage. The other ten entries in the annotation all passed on retry. Every file this PR touches is green on every lane.

Current head 42a91f6: rebased over the engine-native FFI rework (#35246), all CodeRabbit and claude findings addressed, #36497 consolidated into this PR. Ready for review.

[claude]

No new findings — all prior review comments (missing putNativeFn/utf-8-validate/node:sqlite sibling sites, stderr assertion, comment length) are addressed on the current head, and the comment-cop flag is resolved by 80e069b. This still touches JSC bindings and changes user-visible mock.instances semantics, so leaving the merge decision to a human.

What was reviewed:

• jsMockFunctionConstruct: exception checks after getFunctionRealm/createSubclassStructure/the impl call are present; result.isObject() gate matches ordinary-function [[Construct]].
• JSFFIFunction::create now honors its nativeConstructor param — checked its sole caller (Bun__CreateFFIFunctionWithData) already defaults to callHostFunctionAsConstructor via the header.
• Re-swept the remaining self-as-nativeConstructor sites listed in the Scope section; each is always-throws or always-object as claimed.
• os.test.js ?? fallback matches node_os.rs's unwrap_or(b"unknown") (unset ≠ empty).

Extended reasoning...

Overview

The PR fixes a bug class where native [[Construct]] handlers returned primitives, which JSC::Interpreter::executeConstruct hands to an unchecked asObject() — an assertion in debug and UB in release. Six sites are touched: JSMockFunction gets a real construct handler implementing ordinary-function [[Construct]] semantics; JSFFIFunction::createForFFI, node:buffer's isUtf8/isAscii/resolveObjectURL, the shared putNativeFn helper, utf-8-validate, and node:sqlite's backup all drop their self-as-nativeConstructor argument so they become non-constructable. Nine test files add coverage. The PR also changes mock.instances to be populated on every invocation (matching Jest), not only on construct calls.

Security risks

None. This closes a fuzzer-found UB path rather than opening one. No auth, crypto, or permissions code is touched. The new construct handler does not introduce untrusted-input parsing.

Level of scrutiny

High. src/jsc/bindings/ C++ is memory-safety-critical (REVIEW.md's most-blocked category), and jsMockFunctionConstruct is new native code that runs user JS via jsMockFunctionCallImpl. The exception-scope discipline looks correct (RETURN_IF_EXCEPTION after each fallible call, no clearException), and the getFunctionRealm + createSubclassStructure pattern matches NapiClass and JSC's ObjectConstructor. But the mock.instances semantics change (now pushed on every call with thisValue, so a plain fn() records undefined and obj.m() records obj) is a user-visible behavior change to bun:test that a maintainer should confirm is the intended Jest-parity direction — Jest does behave this way, but existing Bun tests that read mock.instances may see different lengths.

Other factors

The PR has been through three rounds of completeness review (my earlier findings caught the missing putNativeFn, utf-8-validate, and node:sqlite siblings; all fixed). CI was 285/286 with the one red flagged pre-existing on main. Test coverage is thorough — every fixed site has both a Reflect.construct throws-TypeError assertion and a Bun.inspect [Function: ...] assertion, plus the mock construct block covers primitive/object returns, newTarget.prototype, cross-realm, and throwing implementations. Given the scope (native construct semantics + a bun:test behavior change), I'm deferring rather than approving.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 test/js/bun/ffi/ffi.test.js:1531-1542 — This new cc() test is the only in-process TinyCC compilation in the tree not gated .skipIf(isASAN) — every one of the eight cc() blocks in cc.test.ts (lines 19, 31, 240, 399, 468, 581, 630, 1090) carries that gate with "TinyCC's setjmp/longjmp error handling conflicts with ASan", including the trivial include-free add(a,b) case. FFI_FIXTURE_PATH gates on the system compiler (compileFixture), which is orthogonal to bundled TinyCC and ASAN; ffi-test.c also #includes <stdio.h>/<stdlib.h> (cc.test.ts:16-17 notes CI can't find those), and the result is never .close()'d in a file whose sibling linkSymbols test three tests up runs in a subprocess specifically to keep unfinalized native handles off the ASAN leak checker. Gate this it.skipIf(isASAN)(...), or run it in a subprocess with a minimal inline source (e.g. bool returns_true(void){return 1;}) and .close() the library — matching the pattern the earlier review comment asked for ("skipIf'd on availability, as the description already claims").

  Extended reasoning...

  What's off

  Commit 5d0f5dc added "cc()-compiled functions are not constructors" at ffi.test.js:1531-1542, inside describe.skipIf(!FFI_FIXTURE_PATH)("engine-native FFI (single implementation)"). It calls cc({ source: import.meta.dir + "/ffi-test.c", ... }) in-process, does not .close() the result, and is not gated on isASAN. ffi.test.js doesn't import isASAN at all.

  Why this violates the harness convention

  Every other in-process cc() compilation in the repo lives in test/js/bun/ffi/cc.test.ts and is .skipIf(isASAN)-gated:

  Line	Block	Gate
  19	can run a .c file	it.todoIf(isWindows || isASAN)
  31	given an add(a, b) function (no system includes)	describe.skipIf(isASAN)
  240	threadsafe JSCallback from foreign thread	describe.skipIf(isWindows || isASAN)
  399	GC liveness of compiled symbols	describe.skipIf(isASAN)
  468	variadic functions inside cc()	describe.skipIf(isASAN)
  581	long double varargs	describe.skipIf(isASAN || ...)
  630	thread-local storage	describe.skipIf(isASAN)
  1090	compiler runtime header directory	describe.skipIf(isASAN)

  Each carries the identical rationale comment: "TinyCC's setjmp/longjmp error handling conflicts with ASan." The convention is blanket — even the trivial int add(int a, int b) { return a + b; } block at line 31, which has no system includes and can't fail compilation, is gated. The one un-gated describe in cc.test.ts ("double <-> JSValue conversions" at line 650) runs its cc() calls inside spawned subprocesses, not in the test process.

  REVIEW.md, Tests reviewers reject: "Copy harness conventions exactly."

  Why the existing gate doesn't help

  FFI_FIXTURE_PATH is set when compileFixture(import.meta.dir + "/ffi-test.c") succeeds (ffi.test.js:24) — i.e., when which('cc') || which('clang') || which('gcc') finds a system C compiler for dlopen. That's orthogonal to bundled TinyCC and to ASAN: on the ASAN CI lane with a system toolchain installed, FFI_FIXTURE_PATH is truthy and this test runs.

  The pre-existing cc() call in this same describe block (ffi.test.js:1564) doesn't count: it passes args: ["ptr", "buffer_length"], which ffi_body.rs rejects at argument-type validation (buffer_length is only supported for ... not in cc()) before TinyCC ever runs. So the new test is genuinely the only in-process TinyCC compilation in ffi.test.js — and the only unguarded one in the tree.

  Step-by-step: two concrete failure modes on the ASAN lane

  Mode 1 — missing system headers → longjmp under ASAN:

  1. ffi-test.c lines 3-4: #include <stdio.h> / #include <stdlib.h>.
  2. cc.test.ts:16-17 (the repo's own comment): "TODO: we need to install build-essential and Apple SDK in CI. It can't find includes." Bundled TinyCC only ships its own builtin headers; the system toolchain that satisfied compileFixture knows SDK paths TinyCC does not.
  3. If TinyCC fails to locate <stdio.h>, it longjmps to its internal error handler — the exact interaction every isASAN gate in cc.test.ts documents as ASAN-incompatible. That takes down the whole ffi.test.js file, not just this test.

  Mode 2 — unfinalized native handle → LSan leak report:

  1. The test destructures { symbols: { returns_true } } from cc(...) and never calls .close().
  2. ffi.test.js:1063-1065 — the sibling linkSymbols() constructor test three tests above — runs in a subprocess with the explicit rationale: "the linked library's native handle is not finalized on bun test's exit path, which the ASan lane's leak checker reports against this file."
  3. A cc()-compiled library has the same lifecycle: an unfinalized handle at process exit is attributed to ffi.test.js by LSan.

  Impact

  Commit 5d0f5dc (2026-08-16) postdates the last CI build referenced in the PR timeline (build 89069, 2026-08-05), so this test has not yet been exercised on the ASAN lane. If either failure mode fires, the whole of ffi.test.js goes red on that lane — but CI would catch it immediately on the next push, so this isn't silently shipping a runtime bug.

  Fix

  Match the convention. Either:

  import { bunEnv, bunExe, compileFixture, isASAN, isGlibcVersionAtLeast, isWindows, tempDir } from "harness";
  // ...
  it.skipIf(isASAN)("cc()-compiled functions are not constructors", () => {
    const lib = cc({
      source: import.meta.dir + "/ffi-test.c",
      symbols: { returns_true: { args: [], returns: "bool" } },
    });
    const { returns_true } = lib.symbols;
    expect(returns_true()).toBe(true);
    expect(() => new returns_true()).toThrow(TypeError);
    expect(() => Reflect.construct(returns_true, [])).toThrow(TypeError);
    expect(Bun.inspect(returns_true)).toBe("[Function: returns_true]");
    lib.close();
  });

  or, to run on the ASAN lane too, follow the linkSymbols sibling and spawn a subprocess with a minimal include-free source (bool returns_true(void){return 1;}) written via tempDir, closing the library before exit — which is what the PR description's "TinyCC-backed test … run in a subprocess and gated on cc availability" already claims this test does.

[robobun]

On the cc() test ASan finding: the in-process version it describes (5d0f5dc) had already been replaced by a subprocess in ccb307c, but the finding was still right about the source file. That version compiled ffi-test.c, which includes <stdio.h>/<stdlib.h>, so on the ASan lane a header-resolution failure would have longjmped inside the child. de99310 switches it to an include-free one-liner written to a tempDir, which is the same shape as the ungated subprocess cc() test in cc.test.ts, and drops the Windows skip since nothing is left that can fail to compile. The child calls close(), and running its body alone under the local ASan debug build exits 0 with no LSan report. The createForFFI discrimination still holds since the body goes through cc().

[claude]

I reviewed this PR and found no bugs on this pass — all seven prior inline findings have been addressed as of de99310. Because it adds a new C++ [[Construct]] handler in JSMockFunction.cpp, changes bun:test mock semantics (mock.instances/mock.contexts now populated on every invocation), and makes a large set of native exports non-constructable, a human sign-off is still warranted.

What was reviewed:

• jsMockFunctionConstruct: exception checks after getFunctionRealm/createSubclassStructure/jsMockFunctionCallImpl, object-return-wins fallback — matches the NapiClass/ObjectConstructor pattern.
• mock.instances push in jsMockFunctionCallImpl mirrors the sibling calls/contexts blocks and is covered by the new "instances is pushed on every invocation" test; checked existing mockClear/mockReset tests still hold.
• JSFFIFunction::create now honoring nativeConstructor (default callHostFunctionAsConstructor in the header) and createForFFI — the cc() subprocess test now discriminates the createForFFI hunk.
• All seven JSFunction::create self-as-constructor drops (NodeBufferModule, _NativeModule putNativeFn, UTF8ValidateModule, NodeSqliteModule) each have a Reflect.construct-throws test.

Extended reasoning...

Overview

Fixes a fuzzer-found debug assertion / release UB where native [[Construct]] handlers returned primitives through asObject(). Seven call sites across JSMockFunction, JSFFIFunction, and five native-module headers are fixed; JSMockFunction gets a proper ordinary-function [[Construct]] implementation, and the rest become non-constructable via callHostFunctionAsConstructor or by dropping the redundant nativeConstructor argument. Nine test files gain coverage; jsMockFunctionCall is refactored into an impl that accepts thisValue and now populates mock.instances on every call (Jest parity).

Security risks

None identified. The change tightens behavior (functions that previously had UB on new now throw TypeError), does not touch auth/crypto/permissions, and does not parse untrusted input. The new C++ code follows the ThrowScope discipline (RETURN_IF_EXCEPTION after each fallible call) and the instances push mirrors the existing calls/contexts pattern verbatim.

Level of scrutiny

High. This is C++ in src/jsc/bindings/ — REVIEW.md's most-blocked category — and it changes user-visible bun:test API behavior (mock.instances, mock.contexts, non-constructability of ~80 native exports including all putNativeFn consumers). The jsMockFunctionConstruct implementation looks correct and matches the NapiClass/ObjectConstructor pattern, but an API-surface change of this breadth deserves a maintainer's eyes rather than bot-only approval.

Other factors

• All seven of my prior inline findings (missed sibling sites in _NativeModule.h/UTF8ValidateModule.h/NodeSqliteModule.h, the linkSymbols test not covering createForFFI, the missing default .prototype, the multi-line comment, the os.test.js drive-by, the missing stderr assertion) have been addressed in commits through de99310.
• The mock.instances-on-every-call change is a Jest-compat fix, not a break, but it is a behavior change nonetheless — pre-existing tests that read mock.instances after plain calls would now see entries.
• The author explicitly deferred adding a default .prototype to mocks as a separate API change; the current behavior is pinned by an isBun-gated test.
• Test coverage is thorough: each fixed factory has a discriminating test, and the mock construct block covers primitives, object passthrough, mockReturnValue, explicit newTarget, cross-realm fallback, throwing implementations, and spies.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[robobun]

Rebased onto main (e375701) and corrected the description. Three things changed.

A correction. The description said these functions are not constructable "like Node's". That holds for only part of them. Node v26.3.0 writes 27 of them in JS, so they are ordinary functions there and new works:

	Node v26.3.0	main	this branch
os.freemem, release, totalmem, version	not a constructor	constructor	not a constructor
the other 8 native os functions	constructor	constructor	not a constructor
buffer.isUtf8, isAscii, transcode, resolveObjectURL	constructor	constructor	not a constructor
sqlite.backup	constructor	constructor	not a constructor
util.types.*	14 of 43	44 of 44	0 of 44

So new os.loadavg() returns the array in Node and on main, and throws TypeError on this branch. new os.hostname() returns an empty object in Node and returns the string on main. Array.of.call(os.hostname, 1, 2, 3) is the call that aborts on main. The description now lists this under Downsides. A comment in os.test.js that made the same claim is corrected too.

mock.instances records every call. m(); o.m(); m.call(5) gives [undefined, o, 5] in jest-mock 30.5.2, in @vitest/spy 5.0.2 and on this branch. The doc sentence in jest, and the one bun copied, says "instances that have been instantiated", which is narrower than what jest records. docs/test/mocks.mdx and the JSDoc in test.d.ts now say what the array holds.

Cost, measured on release builds of main and of this branch.

call	main	this branch	delta
mock(() => 1)()	1705.7 instructions	1806.8	+101.1 (+5.9%)
spied method	1757.8	1861.8	+104.0 (+5.9%)
new on a mock	1779.8	2184.4	+404.6 (+22.7%)
util.types.isDate, buffer.isUtf8, os.freemem	639.0, 655.0, 2411.8	the same	0
heap kept per recorded mock call	138.29 bytes	148.85	+10.56
.text	58,299,253 bytes	58,299,765	+512

The method is in the description's notes. In short: main-thread instructions between two marker syscalls, counted by single-step, with the JIT off so the loop has a fixed cost. Wall-clock could not resolve the difference on this machine.

Tests added. One case for each primitive a constructed mock can return (string, symbol, bigint, number, boolean, null, undefined), through new and Reflect.construct. Also a spy on a function that returns a symbol, and a spy on a property that is not a function. With src/ at main, 20 of the 22 tests in the constructing a mock block fail.

Edited: an earlier version of this comment said that new os.hostname() aborts on main. It returns the string. The mock changes and the measurements for them no longer apply to this pull request, see the comment below.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[robobun]

This pull request now covers native functions only. A self-review of the whole diff found that the two halves share no code and have different risks, and that the mock half was the one that held it back.

Removed: the bun:test mock changes (JSMockFunction.cpp, mock-fn.test.js, the docs row and the JSDoc).

• bun:test: mock functions match Jest for Proxy receivers, new, and mock.instances #41398 carries the same construct handler and the same mock.instances change, and adds a default prototype and Proxy receivers.
• The handler calls InternalFunction::createSubclassStructure on every construct. A maintainer objected to that call in fix mock function crash when Reflect.construct returns non-object #28315 (fix mock function crash when Reflect.construct returns non-object #28315 (comment)), and the objection was not answered here.
• The mock half was the only source of per-call cost: 101 more instructions and 10.6 more bytes for each mock call.
• The removed commit is on the branch robobun/e9fbc481/mock-construct-half (13cae76), so nothing is lost. Array.of.call(mock(), 1, 2, 3) keeps its segfault until a mock change lands.

Kept: 5 source files, +10 and -11 lines, and 8 test files. The remedy is the one #31393 and #31219 used: pass no native constructor, so new throws TypeError.

One decision is needed from a maintainer. On main new f() on these functions runs the call function and evaluates to its result. #22480 kept the isAscii and isUtf8 constructors because removal "would be more breaking than correcting the current bug", and it added expect(new isAscii(...)).toBeFalse(). This pull request changes that line to expect a TypeError. Five of the functions return an object under new on main and in Node v26.3.0: os.loadavg, os.userInfo, os.networkInterfaces, buffer.transcode and sqlite.backup. If those five must stay constructable, say so and I will keep a constructor on them. Four of them return an object or throw on every path, so they cannot reach the crash. sqlite.backup has one path that returns undefined, when the VM is terminating (NodeSqlite.cpp, the NeedTermination check).

One case stays open. bun:ffi CString returns a string from its constructor by contract (docs/runtime/ffi.mdx), so it cannot use this remedy. Array.of.call(CString, 1, 2, 3) still segfaults on main and on this branch. That is a separate change.

Cost of what remains, measured on release builds of main (36cd151) and of this branch: an isDate call takes 639.0 instructions on both, an isUtf8 call 655.0 on both, and size reports the same text, data and bss.

With src/ at main, the two spawned tests fail with exit code 134 in each child: ASSERTION FAILED: isCell() for the functions that return a number or undefined, and ASSERTION FAILED: cell->isObjectSlow() for the ones that return a string.

[robobun]

One correction to the open CString case in the comment above.

Array.of.call(CString, 1, 2, 3) does crash, but for a different reason. Array.of constructs its callee with the length 3. CString reads 3 as a pointer, and strlen faults. A bad pointer that crashes FFI is expected.

The repro for the construct contract is:

import { CString, ptr } from "bun:ffi";
Reflect.construct(CString, [ptr(Buffer.from("hi\0"))]);

On a debug build this fails ASSERTION FAILED: cell->isObjectSlow() in JSC::asObject. constructFFICString (src/jsc/bindings/JSFFICString.cpp:54) returns a string from a native construct handler. new CString(ptr) works because the bytecode path does not check the result.

This pull request does not change CString. Its documented contract is that new CString(ptr) returns a string, so it needs a separate decision.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[robobun]

CI status. The diff is green. CI is red on one file that this pull request does not touch, for a reason on the CI hosts.

build	head	jobs passed	failed
121328	6383f7a (rerun, no code change)	178 of 182	4, all test/js/bun/s3/s3.test.ts
121044	6c4214a	177 of 181	4, all test/js/bun/s3/s3.test.ts

s3.test.ts fails before any test runs, on Linux x64 lanes whose agent has no cached MinIO image:

Failed to start service minio: ... failed to resolve reference "quay.io/minio/minio:latest":
unexpected status from HEAD request to https://quay.io/v2/minio/minio/manifests/latest: 401 Unauthorized

The same failure is on main builds 120892, 120896 and 120898. It is reported for both builds above. Nothing in this diff reaches S3 or Docker.

Every test file that this pull request touches passed on every lane in both builds: ffi, bun-jsc, utf-8-validate, buffer-resolveObjectURL, buffer, os, node-sqlite and test-util-types. The other entries in the annotations passed on retry.

I will not push again to rerun CI. The pull request is mergeable, has no open review thread, and waits on the decision in the description.

[robobun]

CString is the case that this PR leaves open. It has a draft of its own now: #44226, with oven-sh/WebKit#739 for the engine side.

CString cannot lose its construct handler, because new CString(ptr) must return a string (#25231, #25257). The draft keeps new CString(ptr) and makes CString not a constructor to everything that asks IsConstructor first.

Both PRs need the same decision from a maintainer: may a construct form that returns a value today start to throw a TypeError?

• Here the forms are new os.hostname(), new buffer.isAscii(buf) and the like.
• For CString they are Reflect.construct(CString, [ptr]), new (CString.bind(null))(ptr) and new (new Proxy(CString, {}))(ptr).

With #44226 alone, Array.from.call(f, [1, 2, 3]) still aborts for 79 other native functions of the same kind. I did not check how many of the 79 are in this PR.

[Jarred-Sumner]

This is Claude (an AI assistant) commenting on Jarred's behalf.

This fix went in with #46021, which is merged. Closing.