Conversation
BakeLoadServerHmrPatch and BakeLoadServerHmrPatchWithSourceMap are defined inside namespace Bake, so their GlobalObject* parameter is Bake::GlobalObject*, the production build's global. Their only callers are in DevServer.rs, which runs in the ordinary runtime VM and passes a plain Zig::GlobalObject. The bodies only use the JSGlobalObject part, so take JSC::JSGlobalObject* like BakeLoadInitialServerCode does. Add a source lint that flags any bake extern "C" entry point taking Bake::GlobalObject* that is bound from Rust outside production.rs, and run the source lints when bake C++ changes.
|
Warning Review limit reached
Next review available in: 3 minutes 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 |
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit e88dc2c has some failures in 🧪 To try this PR locally: bunx bun-pr 38973That installs a local version of the PR into your bun-38973 --bun |
|
Status: ready for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the two-line C++ retype is a strict widening to what the callers actually pass, and the new lint follows the established test/internal/source-lints/ pattern. The inline nit about the dead HOST_EXPORT regex arm is worth cleaning up but not blocking (that arm couldn't match a real scenario anyway, since a HOST_EXPORT marker sharing a name with a C++ extern "C" definition would be a duplicate symbol).
Extended reasoning...
Overview
Three files: (1) src/runtime/bake/BakeSourceProvider.cpp retypes two extern "C" parameters from GlobalObject* (→ Bake::GlobalObject* inside namespace Bake) to JSC::JSGlobalObject*; (2) a new 111-line source-lint test that greps src/runtime/bake/*.{cpp,h} for extern "C" functions taking Bake::GlobalObject* and fails if any Rust file other than production.rs binds them; (3) .github/workflows/source-lints.yml adds src/runtime/bake/*.{cpp,h} to the trigger paths.
Security risks
None. The C++ change widens a pointer parameter type to a base class the callers already pass; the function bodies only use JSGlobalObject members (vm(), SourceProvider::create, DevServerSourceProvider::create, executeProgram), so generated code is unchanged. The lint test only reads source files. The workflow change only adds path triggers.
Level of scrutiny
Low-to-medium. The runtime change is at an FFI boundary, which is normally high-scrutiny territory, but it is a 2-line type-only fix that makes the signature match what the Rust bindings in DevServer.rs:5568/5585 (verified: &JSGlobalObject) actually pass, mirroring BakeLoadInitialServerCode right above it. Bake::GlobalObject derives from Zig::GlobalObject derives from JSC::JSGlobalObject, so there is no ABI or codegen change. I confirmed BakeLoadModuleByKey and BakeGlobalObject__attachPerThreadData (which keep GlobalObject*) are bound only from production.rs, and BakeCreateProdGlobal (return type GlobalObject*, not a param) is correctly not flagged by the lint since only the params slice is tested against BAKE_GLOBAL_PARAM. The lint has self-checks (scannedCxx > 0, names non-empty, boundFromProduction non-empty) so it can't pass by matching nothing.
Other factors
The bug-hunting pass found one nit: the HOST_EXPORT alternative in RUST_BINDING is dead because readSources strips // comments before matching, and every HOST_EXPORT marker is a // comment. This is real but immaterial — HOST_EXPORT is the Rust→C++ export direction, so it can't legitimately share a name with a C++ extern "C" definition anyway (duplicate symbol at link time). The fn Name( and link_name arms cover the actual binding shapes for these entry points, and the boundFromProduction self-check stays green via the fn Name( arm. The runtime paths were exercised via test/bake/dev/server-sourcemap.test.ts and test/bake/dev/hot.test.ts per the PR body. No prior reviews or outstanding comments.
There was a problem hiding this comment.
LGTM — the fix commit (1563110) addresses the earlier note: stripLineComments is now applied only to the C++ scan, so the HOST_EXPORT arm of RUST_BINDING is live against raw Rust source.
What was reviewed:
- Confirmed
SourceProvider::createandDevServerSourceProvider::createboth takeJSC::JSGlobalObject*(BakeSourceProvider.h:16, DevServerSourceProvider.h:19), so the retyped bodies compile unchanged. - Checked that
stripLineCommentspreserves line count (replaces content, keeps newlines) solineOf()on the stripped C++ source stays accurate. - Verified the lint's self-checks (
scannedCxx > 0,namesandboundFromProductionnon-empty) guard against vacuous passing.
Extended reasoning...
Overview
This PR retypes two extern "C" entry points in src/runtime/bake/BakeSourceProvider.cpp — BakeLoadServerHmrPatch and BakeLoadServerHmrPatchWithSourceMap — from Bake::GlobalObject* to JSC::JSGlobalObject*. Their only callers live in DevServer.rs, which runs inside the ordinary runtime VM and passes a Zig::GlobalObject, not a Bake::GlobalObject. The bodies only touch base-class members (vm(), executeProgram, and the two SourceProvider::create factories, both of which I confirmed take JSC::JSGlobalObject*), so generated code is unchanged. It also adds a source-lint test that greps src/runtime/bake/*.{cpp,h} for extern "C" functions with a Bake::GlobalObject* parameter and fails if any Rust file other than production.rs binds them, plus two workflow trigger-path additions so the lint runs when bake C++ changes.
Prior feedback addressed
On the previous revision I flagged that readSources() unconditionally stripped line comments, making the HOST_EXPORT arm of the Rust-binding regex unreachable (HOST_EXPORT markers are themselves // comments). Commit 1563110 moved stripLineComments out of readSources() and into the C++ loop only, with an explanatory comment. The Rust scan now runs against raw source, so all three regex arms are live. This is exactly the first fix option I suggested.
Security risks
None. The runtime change is a type annotation on two FFI parameters; no new inputs, no new allocation, no control-flow change. The lint is a read-only grep over the source tree run in a GitHub Actions job against a released bun.
Level of scrutiny
Low. The C++ edit is 2 lines and type-only, matching the existing BakeLoadInitialServerCode signature immediately above. The workflow edit is 4 lines of trigger-path glob. The lint is new test-only code (~115 lines) following the established test/internal/source-lints/ pattern; it includes self-checks so it can't pass by matching nothing, and the stripLineComments regex uses [ \t]* (not \s*) so newlines survive and lineOf() on stripped source stays accurate.
Other factors
The PR description documents that the lint fails on main naming DevServer.rs:5568/:5585 and passes with the fix, and that test/bake/dev/server-sourcemap.test.ts and test/bake/dev/hot.test.ts pass against the debug build, exercising the retyped loaders at runtime. The bug-hunting system found no issues on this revision.
There was a problem hiding this comment.
LGTM — the earlier note about stripLineComments eating HOST_EXPORT markers is addressed in 1563110 (comments now stripped only on the C++ scan; Rust sources matched raw).
What was reviewed:
- Confirmed
SourceProvider::createandDevServerSourceProvider::createboth takeJSC::JSGlobalObject*, so the retyped bodies resolve to the same calls as before — ABI and generated code are unchanged. - Verified the DevServer.rs bindings declare these as
&JSGlobalObject/*const JSGlobalObject, matching the new C++ signature. - The Windows
test/bake/deinitialization.test.tssegfault on 1563110 cannot be introduced by a base-class pointer widening; 6d2778e is a CI retrigger for it.
Extended reasoning...
Overview
Three files: (1) src/runtime/bake/BakeSourceProvider.cpp widens two extern "C" parameter types from Bake::GlobalObject* to JSC::JSGlobalObject*; (2) a new source-lint test enforcing that any bake FFI entry point typed Bake::GlobalObject* is only bound from production.rs; (3) two path-trigger additions to .github/workflows/source-lints.yml so the lint runs when bake C++ changes.
Security risks
None. This is a pointer-type widening to a base class on an internal FFI boundary. No user input handling, no auth/crypto, no data exposure surface.
Level of scrutiny
Low-to-medium. The C++ change is two tokens per line and is provably behavior-preserving: both function bodies only call global->vm(), SourceProvider::create / DevServerSourceProvider::create (both of which I confirmed take JSC::JSGlobalObject* in their headers), and executeProgram — all base-class operations. BakeLoadInitialServerCode immediately above already uses the exact same signature and body shape, so this is aligning with existing precedent. The Rust callers in DevServer.rs already declare the parameter as JSGlobalObject, so nothing changes on that side either.
The new lint follows the established test/internal/source-lints/ pattern (23 sibling files), includes self-checks against vacuous passing (scannedCxx > 0, names non-empty, boundFromProduction non-empty), and the author demonstrated it fails on main and passes on the branch.
Other factors
I previously flagged that readSources() was stripping // comments from Rust sources too, which made the HOST_EXPORT regex arm unreachable. The author fixed this in 1563110 by moving stripLineComments into the C++ loop only and leaving Rust sources raw — I verified the current diff reflects this, and the added comment explains why. The thread is resolved.
The one CI failure (test/bake/deinitialization.test.ts segfault on Windows 2019 x64, build #97850 on commit 1563110) is not plausibly caused by this change: widening a pointer parameter to its base class is ABI-identical, and neither retyped body dereferences anything Bake-specific. The author pushed 6d2778e as a CI retrigger. No CODEOWNERS entry covers src/runtime/bake/.
BakeLoadModuleByKey and BakeGlobalObject__attachPerThreadData were the last two bake entry points whose parameter was typed Bake::GlobalObject*. Rust binds every one of them as &JSGlobalObject, so the parameter type is a claim nothing checks; follow BakeGlobalObject__getPerThreadData instead and uncheckedDowncast inside the one function that needs the production field, which asserts the claim in debug builds. The lint now simply bans Bake::GlobalObject* parameters on bake extern "C" functions. The source-lints workflow trigger is left alone: merge_group runs it unconditionally and bake changes almost always touch a .rs file.
robobun
changed the title
There was a problem hiding this comment.
LGTM. The follow-up commit (e88dc2c) simplified the approach after my earlier note — every bake extern "C" entry point now takes JSC::JSGlobalObject*, and the lint is a straight ban on Bake::GlobalObject* parameters instead of the Rust-caller cross-check. Note the PR description still reflects the earlier revision (it says BakeLoadModuleByKey/attachPerThreadData keep the subclass type and describes the Rust-side HOST_EXPORT/link_name scan and a workflow-paths change, none of which are in the final diff).
What was reviewed:
- The four retyped signatures only use
JSGlobalObjectmembers in their bodies;attachPerThreadDatanowuncheckedDowncasts likegetPerThreadDataalready does, and its only callers are in production.rs where the global comes fromBakeCreateProdGlobal. - The
BAKE_GLOBAL_PARAMlookbehind — correctly rejectsJSC::JSGlobalObject*andZig::GlobalObject*, and the return type ofBakeCreateProdGlobalis not scanned since only the paren-balanced parameter list is tested. - Workflow trigger:
merge_groupin source-lints.yml has no path filter, so the lint runs on every merge-queue entry regardless of which files a bake PR touches.
Extended reasoning...
Overview
This PR widens the first parameter of four extern "C" entry points in src/runtime/bake/ from Bake::GlobalObject* (a subclass that only exists in bun build --app production VMs) to JSC::JSGlobalObject*, which is what the Rust-side callers actually declare and pass. BakeGlobalObject__attachPerThreadData is the only one that touches a subclass field, and it now uncheckedDowncasts at the use site — the same idiom BakeGlobalObject__getPerThreadData already uses in the same file. A new source-lint test bans Bake::GlobalObject* parameters on any bake extern "C" function.
The PR went through three revisions: the initial change retyped only the two dev-server functions and added a lint that cross-referenced C++ signatures against Rust callers; my earlier review flagged a dead HOST_EXPORT regex arm; the author fixed that, then in e88dc2c replaced the whole approach with a simpler blanket ban and retyped the remaining two entry points. The PR description was not updated after e88dc2c and still describes the intermediate approach, but the commit message is accurate and self-contained.
Security risks
None. This is a type-only change at an internal FFI boundary; the function bodies are unchanged except for one added uncheckedDowncast. No user input handling, no auth/crypto/permissions.
Level of scrutiny
Low. The retyped parameters were already being passed base-class pointers by every caller (Rust declares them all as &JSGlobalObject), so the generated code is identical for three of the four functions. The fourth adds a debug-asserting downcast that mirrors an existing sibling. The lint is a self-contained regex scan over src/runtime/bake/*.{cpp,h} with a self-check that the pattern still matches something.
Other factors
- I confirmed
production.rsis still the only Rust binder ofBakeLoadModuleByKeyandBakeGlobalObject__attachPerThreadData, so theuncheckedDowncastclaim holds. - The candidate concern about the source-lints workflow not triggering on
src/runtime/bake/*.{cpp,h}was examined: the workflow'smerge_groupevent has no path filter, and e88dc2c's commit message explicitly documents this as the intended safety net after reverting the paths addition. - My earlier inline finding is resolved and superseded by the simpler design; nothing outstanding on the thread.
1 participant
Problem
BakeLoadServerHmrPatchandBakeLoadServerHmrPatchWithSourceMap(src/runtime/bake/BakeSourceProvider.cpp:61 and :83) are defined insidenamespace Bake, so theirGlobalObject*parameter isBake::GlobalObject*, the production build's global.mod cin src/runtime/bake/DevServer.rs). The dev server is created byBun.serveinside the normal runtime VM (src/runtime/server/mod.rs:2222), so what actually arrives is a plainZig::GlobalObject.Bake::GlobalObjectonly exists inbun build --appproduction builds (BakeCreateProdGlobal, called fromVirtualMachine::init_bake).&JSGlobalObject, and the two bodies happen to use only theJSGlobalObjectpart. Touchingm_perThreadDatain either function would read past the end of the dev server's global.Fix
extern "C"entry point now takesJSC::JSGlobalObject*, which is what Rust passes. The two HMR loaders andBakeLoadModuleByKeyonly useJSGlobalObjectmembers, so their bodies are unchanged;BakeGlobalObject__attachPerThreadData, the one function that writes a production-only field, doesuncheckedDowncast<Bake::GlobalObject>inside, the wayBakeGlobalObject__getPerThreadDatanext to it already does.uncheckedDowncastassertsis<Target>), instead of in a parameter type that the Rust side cannot see. Calls through the retyped signatures compile to the same code as before:Bake::GlobalObjectderives fromJSGlobalObject(viaZig::GlobalObjectandBun::GlobalScope) through single inheritance only, so the pointer value is the same under either type.BakeLoadModuleByKeyoverlaps by one line with bake: check for exceptions in the production build's module helpers #38949, which changes the same signature's other parameter and return type. Whichever lands second keepsJSC::JSGlobalObject*for the first parameter.extern "C"function in src/runtime/bake/*.{cpp,h} may take aBake::GlobalObject*parameter. On main it fails naming the five declarations above (attachPerThreadData twice, once per file); with this change it passes. It also asserts that it still found the directory'sextern "C"functions, so it cannot pass by matching nothing.bun bd test test/internal/source-lints/bake-global-object-ffi.test.ts(fails against main's versions of the three files, passes with them).bun bd test test/bake/dev/production.test.ts(9 pass with--timeout 120000; the debug build needs 6 to 9 s per test) drivesattachPerThreadDataandBakeLoadModuleByKeywith a real production global, so the new assertion ran and held.bun bd test test/bake/dev/server-sourcemap.test.ts(5 pass) drives the two retyped HMR loaders.Background
Zig::GlobalObjectis theJSGlobalObjectsubclass every normal Bun VM uses.Bake::GlobalObject(src/runtime/bake/BakeGlobalObject.h) derives from it and addsm_perThreadData, a pointer to the production build's module table, attached from src/runtime/bake/production.rs.unsafe extern "C"declarations in which every global is the opaqueJSGlobalObject, so a narrower C++ parameter type is documentation only.uncheckedDowncast<T>(p)(WTF TypeCasts.h) is astatic_castplus a debug-build assertion thatpreally is aT;downcastis the release-asserting variant.Earlier revision of this PR
The first revision retyped only the two HMR loaders and added a lint that allowed
Bake::GlobalObject*parameters as long as the function was bound only from production.rs, plus trigger paths for the source-lints workflow. Review pointed out that this pinned the two remaining narrow signatures in place, while the directory's own idiom (takeJSC::JSGlobalObject*, downcast inside) removes the need for an allowlist and checks the invariant at runtime, and that the workflow hunk collides with #38424 and is superseded by #36179. This revision retypes the remaining two functions, reduces the lint to a plain ban, and leaves the workflow alone.