Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: fix ready for review. Reproduced the stale-codegen behavior and verified the new input lists locally (transcripts in the PR description); review threads are resolved. CI: every lane that ran is green on builds 94713 and 95707. The only failures are unrelated flaky tests that passed on retry, plus the macOS aarch64 test lanes, which never got an agent in either build. Needs a maintainer to merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — additive build-graph fix that lists three previously-untracked codegen inputs on their ninja edges.
What was reviewed:
- Verified each newly-listed file is actually read by the codegen scripts (
internal-module-registry-scanner.ts:38,bundle-functions.ts:799,replacements.ts:2,generate-classes.ts:4) and that none is covered by the existingsources.js/jsCodegen/zigGeneratedClassesglobs. - Confirmed the two pre-existing extra inputs (
ErrorCode.ts,InternalModuleRegistry.cpp) are preserved in the newextraInputsarray and that generate-classes'argscommand line is unchanged (js_classes.ts is a dep only). - Test follows the established
test/internal/build-*.test.tspattern (mock toolchain +resolveConfig+ parseNinja#toString()) and asserts existence, edge membership, and that globbed inputs remain.
Extended reasoning...
Overview
This PR fixes stale-codegen incremental builds by adding three file paths to the input lists of two ninja edges in scripts/build/codegen.ts. emitJsModules gains NativeModuleList.h, BunBuiltinNames.h, and js_classes.ts; emitGeneratedClasses gains js_classes.ts. A shared jsClassesTable(cfg) helper avoids duplicating that path. The two emitters are now exported so a new test/internal/build-codegen-extra-inputs.test.ts can drive them directly against a mock toolchain. One documentation sentence is added to scripts/build/CLAUDE.md.
Security risks
None. This touches only the configure-time ninja-graph generator and adds source-tree file paths to dependency lists. No runtime code, no user input, no network, no crypto.
Level of scrutiny
Low. The change is strictly additive to ninja edge inputs — the existing extraInput/errorCodeInput are preserved verbatim inside the new extraInputs array, and args for generate-classes is untouched (verified by the test's edge.vars.args assertion). Worst case is an unnecessary codegen re-run, which restat = 1 + writeIfNotChanged already prune downstream. I confirmed all five listed files exist on disk and that the four claimed reads occur at the cited lines in src/codegen/. The globs in scripts/glob-sources.ts (src/js/**/*.{js,ts}, src/codegen/*.ts, src/**/*.classes.ts) do not cover any of the three new files.
Other factors
The test file follows the same shape as sibling tests (build-post-link-ordering.test.ts, macos-cross-config.test.ts): fake Toolchain, resolveConfig with an explicit target and linuxSysroot pointed at a tempdir, then inspect Ninja#toString(). The edgeProducing parser inverts ninjaEscapePath ($ , $:, $$) and the build-line continuation exactly as ninja.ts emits them. The test also asserts exists: true for each extra input, so a future rename of any of these files will fail the test rather than silently pointing the edge at a nonexistent path. The comment on InternalModuleRegistry.cpp was corrected (the script does not read it) but the input itself is kept, which is the conservative choice.
|
Updated 12:36 PM PT - Aug 16th, 2026
✅ @robobun, your commit dde4235ba69b4c41abde7c11a05eafea857d81d0 passed in 🧪 To try this PR locally: bunx bun-pr 38049That installs a local version of the PR into your bun-38049 --bun |
There was a problem hiding this comment.
Beyond the inline nit (description-only), I also checked: the four claimed reads (NativeModuleList.h in internal-module-registry-scanner.ts:38, BunBuiltinNames.h in bundle-functions.ts:799, js_classes.ts in replacements.ts/bundle-modules.ts/generate-classes.ts) all exist as stated; the glob-sources.ts refactor is a verbatim extraction of the loop body into globSourceList() with globAllSources() now delegating per field, so behavior is preserved; and the test's mockToolchain/resolveConfig shape matches the existing pattern in test/internal/macos-cross-config.test.ts.
Extended reasoning...
The core build fix in codegen.ts is mechanical and verified correct against the actual codegen scripts. The one finding is a description-only nit. I'm not auto-approving because the new test (289 lines) introduces novel infrastructure — a ninja build-line parser (edgeProducing) and an import-closure walker (importClosure) — that, while it follows the established test/internal/build-*.test.ts pattern for the config/toolchain mocking, is substantial enough to warrant a human glance.
There was a problem hiding this comment.
LGTM — my earlier note about the description/diff mismatch was addressed (the description now says InternalModuleRegistry.cpp is dropped), and nothing else has changed since.
What was reviewed:
emitJsModules/emitGeneratedClassesinput-list changes: purely additive dependency tracking (plus one dead input removed); command lines are unchanged, so the codegen scripts run identically.glob-sources.ts: verified the extractedglobSourceListbody is byte-identical to the old loop body;globAllSources()behavior is unchanged.- The new test follows the existing
test/internal/build-*.test.tspattern (samemockToolchain+resolveConfigshape asbuild-debug-info-flags.test.ts), targets linux-x64 so no host-specificexecSyncpaths inresolveConfigare hit, and confirmed all four newly-listed input files exist on disk.
Extended reasoning...
Overview
This PR adjusts the ninja input lists on two codegen edges in scripts/build/codegen.ts so that files the underlying scripts actually read (NativeModuleList.h, BunBuiltinNames.h, js_classes.ts, and for generate-classes the src/codegen/*.ts helpers) trigger a re-run when edited, and drops one input (InternalModuleRegistry.cpp) that no script reads. It extracts globSourceList() from globAllSources() (pure refactor, body unchanged) so the new test can glob one field cheaply, adds test/internal/build-codegen-extra-inputs.test.ts which derives the expected input set from each script's static import closure, and updates scripts/build/CLAUDE.md.
Security risks
None. This touches only the build-graph description (which files ninja stats before deciding to re-run a codegen step). No runtime code, no user-facing surface, no data handling.
Level of scrutiny
Low. The functional change is confined to inputs: [...] arrays on two n.build() calls — command lines and outputs are untouched, so the codegen scripts execute exactly as before; only when they re-run changes. Adding inputs can only cause more rebuilds (bounded by restat = 1 + writeIfNotChanged), and the one removed input was verified unused. The glob-sources.ts change is a mechanical extract-method with no logic drift. The two new export keywords are inert.
Other factors
- My previous review's only finding (stale PR description about
InternalModuleRegistry.cpp) was addressed by the author and the thread is resolved; the code did not need to change and hasn't since (e1dba26eis a CI retrigger). - The new test reuses the established
mockToolchain()/resolveConfig()pattern from siblingtest/internal/build-*.test.tsfiles, usestempDirfrom harness, and pins the linux-x64 target soresolveConfig's darwin/androidexecSyncbranches aren't reached on any host. - Verified the four hand-listed input paths exist on disk, and that
Ninjaexposes thebuildDir/rel()/toString()members the test's edge parser relies on. - The PR description includes before/after
ninja -ntranscripts demonstrating the fix and that a warm tree stays no-op.
…e their source globs bundle-modules.ts reads src/jsc/modules/NativeModuleList.h (native module ids), src/js/builtins/BunBuiltinNames.h (BunBuiltinNames+extras.h) and src/jsc/bindings/js_classes.ts ($inherits indices); generate-classes.ts reads js_classes.ts as well. None of them matched the globs those edges were built from, so editing one of them alone did not re-run the step and the generated ids stayed stale until a globbed file was touched. Add them to the edges' inputs.
…ts in the test The test now walks each script's static import closure and checks the edge in both directions: everything imported or opened has to be listed, and every hand-listed input has to be imported or opened. That turned up InternalModuleRegistry.cpp on the bundle-modules edge, which no script reads (it came over from the CMake input list), so it is dropped, and class-definitions.ts / helpers.ts missing from the generate-classes edge, which now lists the src/codegen sources like bundle-modules and cppbind do. glob-sources.ts gains globSourceList(field) so the test can use the real src/codegen list without globbing every pattern.
e1dba26 to
dde4235
Compare
There was a problem hiding this comment.
LGTM — the description update from the earlier review round is in place, and no further issues found.
Verified the added inputs against src/codegen/: NativeModuleList.h is read at internal-module-registry-scanner.ts:38, BunBuiltinNames.h at bundle-functions.ts:786, and js_classes.ts is imported by replacements.ts, bundle-modules.ts, and generate-classes.ts; InternalModuleRegistry.cpp appears only in comments. The globSourceList extraction is the loop body moved verbatim, and the generate-classes command line is unchanged (pinned by the third test case). The new test follows the same mock-toolchain / resolveConfig / Ninja pattern as build-codegen-declared-outputs.test.ts.
Extended reasoning...
Overview
This PR touches four files: scripts/build/codegen.ts (adds three files to the bundle-modules ninja edge's inputs, adds sources.jsCodegen + js_classes.ts to the generate-classes edge's inputs, drops the dead InternalModuleRegistry.cpp input, exports two emitters for testing), scripts/glob-sources.ts (extracts the per-field loop body of globAllSources() into an exported globSourceList()), scripts/build/CLAUDE.md (documents the extra-inputs requirement), and a new test at test/internal/build-codegen-extra-inputs.test.ts. No runtime, native, or shipped JS code is touched — this is purely build-graph dependency metadata.
Security risks
None. The change only affects which source files' mtimes ninja compares before deciding to re-run two codegen scripts. No user input, no network, no auth, no crypto.
Level of scrutiny
Low-to-moderate. Ninja edge inputs are conservative by construction: over-declaring causes extra rebuilds, never wrong or missing output; under-declaring (the bug being fixed) causes stale output. The only removal (InternalModuleRegistry.cpp) I confirmed is referenced only in comments in src/codegen/, never opened or imported. The generate-classes edge gains inputs but its args (the actual script command line) is unchanged, and the third test case pins that. The glob-sources.ts change is a mechanical loop-body extraction with byte-identical logic.
Other factors
I grepped src/codegen/ to confirm each claimed dependency: internal-module-registry-scanner.ts:38 reads NativeModuleList.h, bundle-functions.ts:786 reads BunBuiltinNames.h, and replacements.ts / bundle-modules.ts / generate-classes.ts all import js_classes.ts. The new test's structure (mock Toolchain, resolveConfig into a temp dir, Ninja writer, edge parsing) mirrors the existing test/internal/build-codegen-declared-outputs.test.ts almost line-for-line, so it is not novel test infrastructure. The test derives the expected input set from the scripts' static import closure rather than pinning file names, which is more robust than the alternative. My earlier review comment (stale PR description re: InternalModuleRegistry.cpp) was addressed and the thread resolved. CI is green on the latest commit.
This branch has not been deployed
1 participant
Problem
src/jsc/modules/NativeModuleList.h(for example adding an entry toBUN_FOREACH_ESM_AND_CJS_NATIVE_MODULE) and runningbun bddoes not re-run bundle-modules:ninja -n codegen/InternalModuleRegistry+enum.hprintsno work to do, and the generated module ids stay stale until some file undersrc/jsorsrc/codegenhappens to be touched.scripts/build/codegen.ts(emitJsModules) is built fromsources.js(src/js/**/*.{js,ts}) andsources.jsCodegen(src/codegen/*.ts) plus two hand-listed files, but the script uses three more files that match neither glob:src/jsc/modules/NativeModuleList.h, opened bysrc/codegen/internal-module-registry-scanner.ts:38. Its order numbers the native modules; the numbers go intoInternalModuleRegistry+*.h,SyntheticModuleType.h,NativeModuleImpl.h,generated_resolved_source_tag.rsand therequire()rewrites inside every bundled module.src/js/builtins/BunBuiltinNames.h, opened bysrc/codegen/bundle-functions.ts:798.BunBuiltinNames+extras.his generated as "private names the builtins use, minus the ones this header already declares", so adding or removing a name in the header leaves a stale extras header (duplicate or missing name at C++ compile time).src/jsc/bindings/js_classes.ts, imported bysrc/codegen/replacements.ts:2. Each class's index is baked into the bundles as$inherits(<index>, ...).emitGeneratedClasses, inputs = the.classes.tsfiles only) has the same problem with everythinggenerate-classes.tsimports:js_classes.ts(it emits theswitch (id)the baked indices dispatch to inZigGeneratedClasses.cpp),class-definitions.tsandhelpers.ts.ErrorCode.ts,src/jsc/bindings/InternalModuleRegistry.cpp, is read by no codegen script (its comment said it was; it came over from the CMake input list), so touching it re-ran the 15 to 50 second bundle-modules step for nothing.grep -cfor any of the missing files in a freshly configuredbuild/debug/build.ninjaprints 0. The CMake build used the same globs, so none of this was ever tracked.Fix
emitJsModuleslistsNativeModuleList.h,BunBuiltinNames.handjs_classes.tsnext to the existingErrorCode.tsinput, and dropsInternalModuleRegistry.cpp.emitGeneratedClasseslistsjs_classes.tsandsources.jsCodegen(the way bundle-modules and cppbind already cover theirsrc/codegenimports); the script's command line is unchanged.js_classes.tsgoes on both edges because an index change has to regenerate the JS side and the C++ side in the same build; fixing one edge alone would leave them disagreeing. The rule hasrestat = 1and the scripts usewriteIfNotChanged, so a touch that changes nothing re-runs only the step itself. Warm trees are not invalidated: the new inputs are ordinary source files, older than the outputs unless they were actually edited since the last build (a second configure plusninja -non a built tree is stillno work to do).test/internal/build-codegen-extra-inputs.test.ts. Instead of pinning file names, it emits each of the two steps with the realsrc/codegenlist, walks the script's static import closure (Bun.Transpiler.scanImports+Bun.resolveSync), and checks the edge in both directions: everything imported or opened (reads, the tworeadFileSyncheaders) has to be on the edge, and every hand-listed input has to be imported or opened. On main it reportsuntracked: [js_classes.ts, NativeModuleList.h, BunBuiltinNames.h]andunexplained: [InternalModuleRegistry.cpp]for bundle-modules anduntracked: [class-definitions.ts, helpers.ts, js_classes.ts]for generate-classes; with this change both are empty. A third case pins that generate-classes still gets only the.classes.tsfiles as arguments. The table is meant to grow a row per step;scripts/build/CLAUDE.mdsays so.scripts/glob-sources.tsgainsglobSourceList(field)(the per-field half ofglobAllSources(), which now calls it) because globbing every pattern takes ~25 s under a debug build and the test only needs the 21-filesrc/codegenlist.no work to do; after, each touch plans its step (js_classes.tsandclass-definitions.tsplan generate-classes too) and touchingInternalModuleRegistry.cppno longer plans bundle-modules.bun bdbuilds and smoke-tests with the regenerated graph. Transcripts below.Background
scripts/build/codegen.tswrites one ninjabuildedge per codegen script. The edge's inputs come from configure-time globs (scripts/glob-sources.ts) plus whatever the emitter lists by hand. ninja decides whether to run the edge purely from the mtimes of those inputs versus the outputs; a file the script opens but the edge does not list is invisible to it.src/codegen/bundle-modules.ts, which also drivesbundle-functions.ts) turnssrc/jsinto the builtin module blob plus the C++ and Rust tables that index it. Several of those tables are numbered from hand-written lists outsidesrc/js: the native module list (modules implemented in C++, numbered after the JS ones), the builtin private-name list, and the$inheritsclass table.js_classes.tsis the list behind$inheritsBlob(x)and friends in builtin JS. replacements.ts turns each call into$inherits(<index in the list>, x)while bundling; generate-classes.ts emits the C++ function that switches on the same index. Both generated artifacts therefore depend on the file's order.Probe transcripts (linux-x64, build/debug)
Before, with both targets up to date:
After (
bun scripts/build.ts --profile=debug --configure-only, targets brought up to date between touches):Test output on main's
codegen.ts(with only the twoexportkeywords added so the file loads; condensed):Earlier revision of this PR
The first push only added the three files (and
js_classes.tsto generate-classes) and keptInternalModuleRegistry.cppwith a corrected comment; the test pinned the file names. Review pointed out that a dead input on a 15+ second step has a cost and no rationale, and that a name-pinning test only guards against re-deleting these lines, so the input was dropped and the test now derives the expected inputs from the scripts' imports, which is also what surfacedclass-definitions.ts/helpers.tson the generate-classes edge.Related but separate: #38038 (deleted glob inputs), #38035 (undeclared bindgen outputs), #37992 (PCH). The same import-closure check applied to the other edges finds more of this class (bindgen does not list
bindgen-lib*.ts; runtime.out.js and bake do not listsrc/runtime.js; several small steps do not listhelpers.ts), and generate-classes also reads thesrc/runtimeRust tree forgenerated_classes.rswithout listing it; both are filed separately rather than widened into this PR, and the test table here is where their rows go.[stamp-90s] gate passed · iteration 3 · 4 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file
root cause · written by the author bot
The build's codegen step only tracked its primary globbed source files as inputs, so edits to additional files that codegen reads, such as templates and supporting scripts, did not invalidate the cached outputs and stale generated code could ship. The fix extends the source globbing and codegen dependency tracking so these extra input files are registered alongside the globbed sources, ensuring any change to them triggers regeneration. A regression test verifies that modifying an extra input causes the codegen outputs to be rebuilt.