Skip to content

AOT: fixes for modules, classes and Workers, WebKit upgrade, shorter compiler docs - #47129

Open
Jarred-Sumner wants to merge 28 commits into
mainfrom
claude/compile-aot-on-check
Open

Jarred-Sumner wants to merge 28 commits into
mainfrom
claude/compile-aot-on-check

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #45631. Upgrades WebKit to oven-sh/WebKit main at f495db1d2488 (oven-sh/WebKit#930), which holds the engine's half of this work.

Most of this is behind --aot. The first two sections list what is not.

Changes that reach Bun without --aot

  • node:path: the native functions take the width of a substring's characters from its base's storage as it is now. test/js/node/path/string-forms.test.ts covers it.
  • Heap snapshots: Bun.generateHeapSnapshot(), its V8 form and the heap profiler release the profiler's snapshot once the JSON is built.
  • execve: the suspension lock is held over execve.
  • process.env on Windows: Object.preventExtensions(), Object.seal() and Object.freeze() throw a TypeError, as in Node.
  • Module loader: follows VM::synchronousModuleQueue() from the WebKit upgrade.
  • Windows: the crash handler registers with WTFSetAbortReporter().
  • Parser: a class records whether useDefineForClassFields: false turned its fields into assignments in the constructor. declare is a name on the left of for-of, for-in and for await, and before every kind of line break. Any of the language's line breaks may follow global.
  • bun check: a ring of functions without annotations resolves in linear time. Only callable built-ins join a function type. The type of a well-known symbol is symbol. --timing prints the number of queries.

bun build --compile

  • Bun.build({ compile }): import.meta.main is inlined for the entry point, as on the command line.
  • Windows: where the section table has no free slot for the payload's header, the sections' data is laid out again behind a table that has one.
  • macOS: the executable is built and signed over a private mapping of the template and written page by page, so the process never holds the whole file in memory.
  • node:fs on paths inside an executable: the asynchronous calls on Windows name each path in an error as the caller wrote it. Asynchronous statfs() answers for the file system that holds the executable.
  • --trim:
    • On macOS, stack traces and the allocator's override know the several code segments of a trimmed executable.
    • The two TLS hooks that only the WebSocket client calls go with it.
    • On Windows, the tool tells a member of an archive from a file of the same name.
    • On Windows, a build on a trim template fills holes and joins pieces over them, so it cuts what a build on Bun cuts.

The ahead-of-time compiler

  • Modules:
    • One set of __esModule marks serves both kinds of module.
    • Export tables of re-exporting files are limited like those of other files.
    • A name taken from a JSON import loads the files in between, so files that a package lists under sideEffects run.
    • A name that a library module lacks can be read off its namespace.
  • CommonJS: under "type": "module", a default import holds what the link captured. It takes the short path only where nothing stores to module.exports after the body ends.
  • Module names from lists and tables: a walk of a list (forEach, map, filter, find and so on) counts as leaving the list unchanged only under these conditions:
    • its callback is written in the call
    • the callback has no rest parameter
    • the callback cannot reach itself through arguments, a direct eval or its own name
  • Projects with a second tsconfig.json take what the application's load found: import.meta.main, file identities, erased sites and folded constants.
  • Classes:
    • An unused class is removed whole only where its heritage is proven.
    • Methods stored on a constructor function's prototype are members of its layout.
  • Workers:
    • Whether the files and texts that Workers start from form one program with the rest is decided after the front end, before the engine's first run.
    • A Worker that finds no room for its thread waits for siblings that are ending, and is tried again on a timer.
  • node:http: only what new ServerResponse() makes takes the compact form. An instance of a subclass, and an object that ServerResponse is called on, keeps its state on itself.
  • Build: a third list of the engine's sources is compiled for minimum size. Dependencies that no code of a crate uses are gone from 13 Cargo.toml files.

Docs

docs/compiler is rewritten and about 5,800 lines shorter. Each page keeps a runnable example, which test/cli/aot-docs.test.ts runs. docs/snippets/compiler-internals.jsx had no importer left and is removed. The table of libraries known not to work moves next to its test.

Tests and CI

The ahead-of-time compiler's tests run in the ordinary test-bun shards. The test-aot and test-aot-drawn steps, their path triggers, draws and shares are gone, and so is every step that was allowed to fail. Every pull request gets the same pipeline.

  • Lanes: every test of the compiler runs on the two Alpine lanes. The other lanes run the ones that ask something of the system: processes, Workers, the environment, files, sockets, memory limits, the image, native code. Windows also runs the .ir tests. The lane with AddressSanitizer runs all but a list of files that feed it nothing new, on a copy of Bun without its debug information. The lists are in scripts/runner.node.ts, and a test file that says // Runs alone: stays out of the bucket of files that run side by side.
  • Whole programs are small files under test/aot/parallel/. Each passes when its executable exits and prints as bun <file> does, on Alpine also with its types erased. The runner runs them side by side, as it does Node's tests.
  • Source to IR: files under test/aot/ir/ with // RUN: lower and // CHECK: lines. cargo test runs them in one Buildkite step. AOT_IR_UPDATE=1 writes everything below a // GENERATED: line again.
  • IR to IR, and IR to B3: .ir files under test/aot/engine-tests/, run by testaot with Bun's table of built-ins. bun test/aot/tools/update-ir-tests.ts writes a file's input from its source and its expectation from its input.
  • Servers, frameworks and packages are ordinary test files: test/aot/servers/, test/aot/frameworks/, test/aot/backend-packages/.
  • Generators moved to test/aot/fuzz/, where bun test/aot/fuzz/run.ts draws from them. No test file starts one.
  • The stored programs were cut to those that cover an edge of the compiler that no cheaper one covers, and those that came in with a fix.
  • bun run rust:test runs the Rust crates' unit tests in the same Buildkite step.

…ut of lists and Workers' programs; WebKit preview 7f21d5ed [skip size check]

- WebKit: autobuild-preview-pr-927-7f21d5ed.
- Modules: bind_modules() keeps one set of stored __esModule marks for both kinds of module and reads a changed mark from the object. limit_exports() limits the export tables of faces as it does those of files. A name taken from a JSON import asks for the files between, so files that a package lists under sideEffects run. What `delete` removes from a namespace is bound on no object.
- CommonJS: under "type": "module" a default import holds what the link captured, and takes its short way only where nothing stores to module.exports behind the end of the body. A file whose namespace may be looked at and whose text does not say all its names has its object.
- Names of modules out of lists and tables: a walk of a list (forEach, map, filter, find ..) leaves the list as written only where its function is written in the call, has no rest parameter, does not find itself through `arguments`, a direct eval or its own name, and holds its third parameter as every other holder of the list does. `instanceof` hands the list on. A table that writes its own toString or valueOf is not followed.
- Programs under a second tsconfig take what the application's load found: import.meta.main, which file stands for which, erased sites, folded constants. An imported require that is given a path from its own folder resolves from that folder. Which file of the application a file is goes by its path and its loader.
- Classes: an unused class goes whole only where its heritage is proven; otherwise it leaves its heritage and one look at it, a computed key leaves ToPropertyKey, and a static member named "prototype" keeps the class. The transpiler records which classes assign their fields under useDefineForClassFields: false and the second parse reads that, with the build's own table of defines. Methods stored on the prototype of a constructor function are members of its layout.
- Syntax: the parser says where no `;` can follow a bare `declare`, so `declare` is a name on the left of for-of, for-in and for await and in front of every kind of line break; a line break behind `global` is any of the language's.
- Types: only callable built-ins join a function type; the type of a well-known symbol is symbol.
- Workers: whether the files and texts that Workers start from are one program with the rest is decided behind the front end, ahead of the first run of the engine. The constructor tells the engine when other threads are ending.
- macOS: operator new of every piece of a trimmed executable goes to mimalloc, and the ranges of its pieces are known to stack traces.
- bun check: a ring of functions without annotations is resolved in linear time.
- The build: a third list of the engine's sources, compiled for minimum size.
- Tests: stored programs, generated tables and fuzz families for each of the above and for the engine's changes; a NaN of every pattern of bits through N-API, bun:ffi and the serializer into fields of every kind; definitions by descriptor; host objects that come back by some road; declarations that nothing uses; contextual keywords as names. Files that read how often compiled code calls the run time run on Windows. The large build of workers-from-texts-that-the-build-knows takes its turn among the others again. Program folders have short names; two tests join the fixed step; tests that pass on both Windows lanes leave the awaited list and two units join it; the engine's native tests run on Windows.
- Docs: rows of docs/compiler/compatibility.mdx follow the above.
@Jarred-Sumner
Jarred-Sumner requested a review from alii as a code owner October 28, 2026 01:08
@robobun

robobun commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 5:29 PM PT - Oct 28th, 2026

@Jarred-Sumner, your commit 5f5f865 is building: #134760

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread scripts/build/deps/webkit.ts Outdated
Comment thread src/sema/check/mod.rs
Comment thread src/jsc/web_worker.rs
Comment thread test/bundler/compile-trim-allocator.test.ts Outdated
Comment thread src/jsc/bindings/MimallocOverrideDarwin.cpp
Comment thread src/aot/load/source_provider.rs
…off an ES module, and tests for the engine's changes; WebKit preview 617ed49e [skip size check]

- WebKit: autobuild-preview-pr-927-617ed49e.
- Modules: what kind of module a file is goes by whether a top-level await is written in it, not by whether one can be reached. A CommonJS file whose getter reads, off an ES module, a name that the ES module hands on from another CommonJS file keeps its object unless the copy and the name cannot be told apart.
- The loader's fetch hook asks VM::synchronousModuleQueue().
- Strings: the natives of node:path take the width of a substring's characters from its base's storage as it is now; a string that is used as a property key takes the storage of an equal key, of either width.
- Output: a build prints neither the hint about untyped code nor the counts of property accesses; the counts end the text report of --explain.
- macOS: --aot-symbols takes a file whose table of indirect symbols has an odd number of entries.
- bun check: the calls that were resolved meanwhile are one list.
- Windows: in place, the path that an import with { type: "file" } gives has the separators of the host.
- Tests: stored programs for each of the above and for the engine's changes: module variables of files that are still running, patterns without elements, Reflect.construct, super behind a delete, closures among many entry points, counted imports under a synchronous load, a callback that stores the answer of a call that never returns into a captured variable. Own members of compiled objects that native code looks for: RegExp test and search, the first line of a stack, Bun.inspect, concat, printed class names, labels of heap snapshots. Sums over typed arrays. A family for traps that are stored on the handler after the proxy is made. Four units run the engine's flow tests under 32 other schedules of the rules. The reference run of the packages takes the reference's environment. Five more units run on every commit on Windows. The three programs that terminate a Worker inside a loop count the walks that came to their end, in place of a clock. The tests of --trim are skipped where the Bun under test cannot trim, and the one on the allocator also runs with glibc. highway-strings.test.ts lists what is wrong and calls expect() once a test: the same cases in half a second. One-loop rows for numbers that leave their slot. A way out of native code that the system refuses is left out of both runs of the N-API test. A test of lint.yml's kind runs check-unified-sources.py. Sabotages for calls that throw without a site and for bounds that are read twice. Pins of generated code that fell are restated. The pins and rows of objects of constructor functions that are not built are as they were before that step of SinkAllocations.
- Docs: a row of docs/compiler/compatibility.mdx for useDefineForClassFields under a nested tsconfig.json, and its table of platforms says what CI runs on each; a paragraph of docs/compiler/internals.mdx on calls without a site.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still open from earlier reviews (5):

  • 🔴 src/sema/check/mod.rs:3766 — Users of bun check can get diagnostics from a cycle or trial evaluation that the base branch drops and re-reports from…
  • Also unresolved: 3 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Comment thread test/aot/napi-addon/a-nan-that-native-code-hands-over.ts
@Jarred-Sumner Jarred-Sumner changed the title AOT: module binding, class fields, names of modules out of lists, programs of Workers AOT: fixes for modules, classes and Workers, WebKit upgrade, shorter compiler docs Oct 28, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked the new mapped-output path in src/exe_format/macho.rs: FileBytes::resize silently falls back from the mapping to a Vec when the signer outgrows it, while FileOutput keeps the lets_go flag captured before signing — but MachoFile::room_for reserves payload + 2×symbols + the signature of that size + two 64 KiB slacks, so the signer's resize stays inside the mapping and let_go_of_pages is only ever run over mapped pages.

Extended reasoning...

This push changes WEBKIT_VERSION to a 40-hex sha and otherwise carries the same 985-file AOT change (module binding, class fields, worker programs, Mach-O/PE image handling, checker changes). The ruled-out item concerns the new build_and_sign_into/FileOutput path in src/exe_format/macho.rs, where a stale lets_go flag would remap heap pages with MAP_FIXED if the mapping's reserved room ran short; room_for's bound covers the signer's growth. Not approved: inline findings are posted, a further verified finding is unposted, and the change is far too large and engine-adjacent for an automated sign-off.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Still open from earlier reviews (2):

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Comment thread test/js/node/process/process-execve-under-the-profiler.test.ts Outdated
Comment thread test/js/bun/util/heap-snapshot-is-dropped.test.ts Outdated
Comment thread scripts/runner.node.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 test/aot/run-parallel.ts:109 — CI loses the per-child time and memory limits for the ~9,900 compiled programs: one program that never exits now stalls the whole driver instead of failing alone. start() at test/aot/run-parallel.ts:105 calls Bun.spawn with no timeout, no killSignal and no endAtMemoryLimit, and nothing else bounds a child. Fix: give every spawned build, executable and reference run a time limit (SIGKILL, scaled for ASAN as the old 90_000/240_000 ms were) and the memory limit from ./harness/memory-limit, and report a killed child as a "run" failure naming the signal, so a hang fails only its own program.

    Why this was flagged

    A program under test/aot/parallel that hangs (a terminated Worker that never ends, a loop that is not interrupted, a deadlock) is run by start() at test/aot/run-parallel.ts:103-109, whose Bun.spawn has no timeout, killSignal or memory limit; await Promise.all(batches.map(run)) at :484 then never settles. The deleted test/aot/run-programs.ts:476-486 spawned every child with timeout (90_000 ms, 240_000 ms under ASAN or the checks) plus killSignal: "SIGKILL" and endAtMemoryLimit(proc, memoryLimitOfAKnownProgram), so a hang or runaway allocation failed that one program and the suite went on. Now the only bound is the runner's idleTimeout: 4 * 60_000 at scripts/runner.node.ts:1336, which SIGKILLs the driver only (runner.node.ts:1809), leaving the hung executable and every in-flight bun build orphaned on the agent. The memory cap is gone entirely, so a program that allocates without bound can exhaust the agent and take the other programs of its batch down with it.

    Verification: start() at test/aot/run-parallel.ts:105 is Bun.spawn({ cmd, cwd, env, stdin: "ignore", stdout: "pipe", stderr: "pipe" }) — no timeout, no killSignal, no endAtMemoryLimit; :106 awaits child.exited unconditionally. The only outer bound is the runner's idleTimeout: 4 * 60_000 (scripts/runner.node.ts:1335-1336), and subprocess.kill(9) (:1809) kills the driver alone.

  • 🟡 test/aot/run-parallel.ts:430 — nit: a Bun whose --revision lacks -canary (any release build) reports every program under test/aot/parallel as skipped, so none of them verifies that build. test/aot/run-parallel.ts:429-430 turns !isCanary into whySkipped, and :434-437 skips all files, because the executable only honours BUN_AOT_START_AT under IS_CANARY (src/standalone_graph/StandaloneModuleGraph.rs:136-139). Fix: let a release build run the programs too, e.g. build each program alone (or in groups started by a flag the executable always reads) when BUN_AOT_START_AT is unavailable, rather than skipping the whole suite.

    Why this was flagged

    Trigger: scripts/runner.node.ts:1451-1469 hands every file under test/aot/parallel to bun test/aot/run-parallel.ts on each lane; on a Bun built without a canary revision (bun --revision without -canary, which .buildkite/ci.ts:100-125 yields when ahead_by is 0) test/aot/run-parallel.ts:403 sets isCanary false. :427-433 then sets whySkipped to "only a canary build of Bun reads BUN_AOT_START_AT" and :434-437 prints { state: "skip" } for every file and exits 0; scripts/runner.node.ts:735-738 records each as skipped. The dependency is real: src/standalone_graph/StandaloneModuleGraph.rs:136-139 returns None from start_at_named_entry_point unless bun_core::env::IS_CANARY, so a multi-program executable always starts its first program. On the base branch these programs ran through test/aot/run-programs.ts, which built and ran them without any canary-only variable. The result is that the ~thousands of behavioral programs under test/aot/parallel are never executed against a non-canary build, and the runner shows it only as skips.

    Verification: Trigger: the test runner is pointed at a Bun whose bun --revision output lacks "-canary". test/aot/run-parallel.ts:427-433 turns !isCanary into whySkipped; :434-437 emits { state: "skip" } for every file. scripts/runner.node.ts:1379-1382 records each skip as an okResults entry with status "skip", so the lane is green with none of those programs compiled or compared.

  • 🟡 test/aot/run-parallel.ts:436 — nit: on a Bun that cannot build or start these programs the driver leaves its temporary folder behind. mkdtempSync at test/aot/run-parallel.ts:75 creates folder before the skip check, and the skip path at :434-437 calls process.exit(0) without the rmSync(folder, ...) that the normal path runs at :485. Fix: remove folder on every exit path (create it after the skip decision, or rmSync before process.exit(0) in the skip branch).

    Why this was flagged

    Trigger: bun test/aot/run-parallel.ts on a Bun where refusal (test/aot/run-parallel.ts:398), !isCanary (:403) or startsWhereItIsTold() (:405) makes whySkipped a string, which scripts/runner.node.ts:1333 does on every non-canary or non-AoT lane. folder is created by mkdtempSync(join(tmpdir(), "aot-parallel-")) at :75 before that decision; startsWhereItIsTold writes and removes its own files in it (:406-424) but the directory itself remains. The branch at :434-437 prints one skip line per file and calls process.exit(0), never reaching rmSync(folder, { recursive: true, force: true }) at :485. Each such run leaves an empty aot-parallel-* directory in tmpdir, which the repository's test rules (leave nothing behind on persistent CI runners) forbid; the base branch had no such driver.

    Verification: nit. The driver runs on a Bun that is not a canary, that refuses --aot, or that ignores BUN_AOT_START_AT. test/aot/run-parallel.ts:75 creates folder before the skip decision at :427-433. The skip branch at :434-437 calls process.exit(0) with no rmSync(folder, ...). The only cleanup is at :485, so every skipped run leaves an empty aot-parallel-XXXXXX directory in tmpdir.

  • 🟡 test/aot/README.md:3 — nit: maintainers following test/aot docs and comments are sent to files this PR deletes (test/aot/run-programs.ts, parts.mts, parts.test.ts). sweep:run-programs\.ts|parts\.test\.ts|parts\.mts|TEST_PART= test/aot/README.md:3 links run-programs.ts as the place that says how a program is written and judged, and README.md:92 gives TEST_PART=... bun test ./test/aot/parts.test.ts as the command to run a group; test/tsconfig.json:52 explains the aot/programs exclude by run-programs.ts. Fix: point every such mention at test/aot/run-parallel.ts and the bun test/aot/run-parallel.ts <file> command, or drop the sentence where the mechanism no longer exists.

    Why this was flagged

    The PR deletes test/aot/run-programs.ts, test/aot/parts.mts and test/aot/parts.test.ts and replaces them with test/aot/run-parallel.ts driven by scripts/runner.node.ts. test/aot/README.md is edited in the same diff but line 3 still reads "The top of run-programs.ts says how a program under programs/ is written and judged", and line 92 still tells a maintainer to run AOT_SHARE=all TEST_PART=aot/joined-programs-validated bun test ./test/aot/parts.test.ts. test/tsconfig.json:52, also edited here, keeps the comment "run-programs.ts builds each with their own tsconfig.json". Running the documented command fails because the file is gone, and the link resolves to nothing. On the base branch those files existed and the commands worked. About thirty more comment lines under test/aot (for example test/aot/builtins/differential-cases.ts:11, test/aot/perf/pinned-applications.ts:10, test/aot/generate.ts:2) repeat the same stale names; the sweep expression finds them.

    Verification: nit. Triggering condition: a maintainer follows test/aot/README.md or the tsconfig comment. test/aot/run-programs.ts, parts.mts and parts.test.ts are deleted at HEAD. test/aot/README.md:3 still links run-programs.ts, a dead link; line 92 still gives bun test ./test/aot/parts.test.ts, which cannot run; test/tsconfig.json:52 still cites run-programs.ts. Documentation-only harm; nothing executable breaks.

Comment thread scripts/runner.node.ts Outdated
Comment thread src/aot/compile.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Still open from earlier reviews (1):

  • 🔴 scripts/runner.node.ts:2657 — Maintainers lose CI coverage: AOT suites that ran as units of parts.mts now have no runner at all. scripts/runner.node.…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 test/aot/run-parallel.ts:433 — Maintainers can get a green shard while none of the ~9,900 programs under test/aot/parallel ran: a canary Bun whose probe executable crashes or errors at start is reported as "this Bun does not read BUN_AOT_START_AT" and every program is skipped. startsWhereItIsTold() at test/aot/run-parallel.ts:423-425 only compares the probe's stdout with "second"; its exit code, signal and stderr are never looked at, so a startup crash of compiled executables or a broken BUN_AOT_START_AT lookup ("NoProgramToStartAt") is indistinguishable from a locked environment. Fix: skip only when the probe exits 0 with empty stderr and prints "first", and otherwise print a fail verdict for every file, or exit non-zero, so the runner reports the regression.

    Why this was flagged

    The driver test/aot/run-parallel.ts runs on a canary Bun (isCanary true at :403) and reaches startsWhereItIsTold() at :431. The probe is built at :408-422 with --aot-separate-programs; the build's exit code is not checked. At :423 the probe is run with BUN_AOT_START_AT=second.js and only its stdout is read; :425 returns said.trim() === "second". If the executable crashes at start, prints an error such as "BUN_AOT_START_AT names no entry point that begins a program of its own" (src/standalone_graph/error.rs:23) and exits 1, or prints nothing, the function returns false and :433 sets whySkipped to "this Bun does not read BUN_AOT_START_AT". :434-437 then print { state: "skip" } for every file and exit 0. scripts/runner.node.ts:1379-1381 records each skip in okResults, so the shard is green and no program ran. On the base branch the corpus ran through test/aot/run-programs.ts without such a probe, so a startup regression failed the programs instead of skipping them.

    Verification: In the new file test/aot/run-parallel.ts, :423 reads only .stdout of the probe run; :425 return said.trim() === "second". Exit code, signal and stderr are never inspected. At :427-433 a false becomes whySkipped = "this Bun does not read BUN_AOT_START_AT", and :434-437 emits {state: "skip"} for every file and process.exit(0). scripts/runner.node.ts:1379-1382 pushes it with ok: true into okResults.

…, the program named at start on Windows; WebKit preview-pr-930-3fdfc9b1 [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no new issues

No new issues were found in this update; 3 findings from earlier reviews are still open above.

Still open from earlier reviews (3):

  • 🔴 scripts/runner.node.ts:2657 — Maintainers lose CI coverage: AOT suites that ran as units of parts.mts now have no runner at all. scripts/runner.node.…
  • Also unresolved: 2 minor or pre-existing.

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Jarred-Sumner and others added 2 commits October 28, 2026 02:18
…named without its extension; typed rows of a query; WebKit preview-pr-930-bf1133d2 [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Still open from earlier reviews (1):

  • 🔴 scripts/runner.node.ts:2691 — Maintainers lose CI coverage: AOT suites that ran as units of parts.mts now have no runner at all. scripts/runner.node.…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Comment thread .buildkite/ci.ts
Comment thread src/exe_format/macho.rs Outdated
Comment thread .buildkite/ci.ts
…each executable behind its runs; each CI step names the build it tests; the Mach-O writer hands out owned parts of a mapping; bun check keeps the members of a named union; WebKit preview-pr-930-0cb47ca7 [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread scripts/runner.node.ts
Comment thread scripts/rust-test.ts Outdated
…un without debug information that the build step uploads beside bun-profile; test/aot/fuzz and its test as in 5319f52 [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no new issues

No new issues were found in this update; 2 findings from earlier reviews are still open above.

Still open from earlier reviews (2):

  • 🔴 scripts/runner.node.ts:2434 — Maintainers can see slow test/aot files killed by the runner's file wall on CI lanes where they previously finished. Dr…
  • Also unresolved: 1 minor or pre-existing.

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

… all modules run in the cargo lane; every lane builds the light programs alone; under ASAN the compiled programs run on a Bun without debug information; a following round is asked for only where it keeps more; a frame names a function made under a symbol key by its own name; the generators of the fuzzer are sources of run.ts; WebKit preview-pr-930-430afb3a [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

… traps; the printer is loaded for a printed value only where a class that needs it may have an instance; start-up functions are handed over by need; internal/fs/streams no longer loads node:fs/promises; a list that grows by push is checked in linear time; the tests build a trimmed and a whole executable in one build [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

…at it imports; the shards are dealt whole builds; long test files run beside one another under ASAN and on Windows; the texts of the library read the members of Object, Buffer, Reflect and Bun in one order; a list that grows by push is checked in linear time whatever lies between the pushes [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

…o ask for; the exports of a module of the library are handed to nobody new as the receiver of a member call; pushes with branches between them are checked in linear time; tables that only a build reads are made at first use; one compile writes the whole and the trimmed executable in two test files [skip size check]
…that cost the plain files their form [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/jsc/bindings/BunProcess.cpp Outdated
…described by their paths; a star or dynamic import of a library module brings what its getters load; a call of what is no function throws wherever it stands; a name out of a list that was changed in place counts as unknown; the long test files run two at a time on the Linux lanes; every test of the compiler runs on Alpine, the other lanes run those that ask the system, and the lane with a sanitizer leaves out those that compile one text again; an executable built on a PE template chooses its cuts anew; dependencies that no code of a crate uses are gone [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

Comment thread scripts/runner.node.ts
…hem; three programs run half their turns; a test file that says it runs alone stays out of the bucket [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

Comment thread test/aot/files-written-while-bundling.test.ts
…piler run on a copy of Bun without its debug information, and what fails is tried again on the whole file; bun check --timing prints its queries, and the test of long rings counts them [skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Still open from earlier reviews (2):

  • Unresolved: 2 minor or pre-existing.

This review covers commit 8a44e04, which is no longer the latest commit on this pull request; later commits are not covered by it.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

Still open from earlier reviews (2):

  • Unresolved: 2 minor or pre-existing.

"classes with a method of one name that each call it on an instance of any of them" has had its growth in instructions and in two counts in budgets.linux-x64.json. Its growth in memory stands beside them now: macOS judges it, where Linux leaves it unjudged below 96 MiB.

[skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Still open from earlier reviews (2):

  • Unresolved: 2 minor or pre-existing.

Comment thread scripts/runner.node.ts
src/exe_format/macho.rs: the loop that hashes and writes pages calls `SHA256::hash(page, hash)`, as on main.

[skip size check]
…ative plugins run on every lane

- readMembersThatMakingStandardErrorReads(): the two lambdas are static functions with a ThrowScope of their own, and each call site checks or releases.
- scripts/runner.node.ts: the three test files that compile a plugin with the system's compiler and load it are among the tests that ask the system, so they run on every lane.
- test/aot/files-written-while-bundling.test.ts says why its plugins wait.

[skip size check]

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the latest push and found no new bugs; given the size of this change (compiler, engine bindings, CI runner), a human look is still worthwhile before merge.

What was reviewed:

  • Commit 021911d addresses the three open threads: BunProcess.cpp readMembers/readMembersOfGlobal are now static functions with their own ThrowScope and every call site checks or releases; the three native-plugin test files are listed in aotTestsThatAskTheSystem; the 500 ms waits in files-written-while-bundling.test.ts now state why no observable signal exists.
  • Checked the deletion of the frameworks executable tests (test/aot/frameworks/run-framework.ts) — a coverage reduction, not a correctness issue, and the remaining frameworks still build in one form.
Extended reasoning...

The latest commit since the previous review touches only scripts/runner.node.ts, src/jsc/bindings/BunProcess.cpp and test/aot/files-written-while-bundling.test.ts, each a direct response to an earlier open inline nit. The full PR remains a very large change (tens of thousands of files across the AOT compiler, sema checker, JSC bindings, exe_format, and the CI runner) that no automated review can certify end to end. No authentication, crypto or permission surface is touched; the exception-check rework in BunProcess.cpp is the only engine-facing code in this push and now follows the no-check-macros-in-lambdas rule. Size and the native-code scope decided defer over approve.

On macOS, an executable made by `bun build --compile` started a detached
thread at boot that loaded the system trust store when a flag in the module
graph said the program might ask for it. Under `--aot` the flag was set by
reachability; otherwise it was set by searching the output for the text
`getCACertificates`.

This removes the thread, the flag, both places that set it, and the test for
the flag. The system trust store is loaded on first use again, on the thread
that asks for it.

[skip size check]
They checked that Security.framework was loaded before the program asked for
the system certificates, which no longer happens. Also updates a comment in
node:tls that described the prefetch.

[skip size check]

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants