Rename $zig() binding macro to $native() and key it off .rs files#32216
Rename $zig() binding macro to $native() and key it off .rs files#32216robobun wants to merge 9 commits into
Conversation
|
Updated 5:07 PM PT - Jun 17th, 2026
❌ @robobun, your commit 6f16c0b has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32216That installs a local version of the PR into your bun-32216 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenames the ChangesNative macro rename and JS2Native pipeline update
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@scripts/build/codegen.ts`:
- Line 59: The generated-file list was changed so zigFilesGeneratedIntoSrc no
longer includes "src/jsc/bindings/GeneratedJS2Native.zig" but src/bun.zig still
imports ./jsc/bindings/GeneratedJS2Native.zig, causing a producer/consumer
mismatch; either add "src/jsc/bindings/GeneratedJS2Native.zig" back into the
zigFilesGeneratedIntoSrc array in scripts/build/codegen.ts (so the generator
produces the file) or remove/update the import in src/bun.zig to stop depending
on GeneratedJS2Native.zig (and adjust any call sites that relied on it),
ensuring the producer (zigFilesGeneratedIntoSrc) and consumer (src/bun.zig
import) are consistent.
🪄 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: dcf03c6a-74c2-41b4-b7eb-e01a2b1b0e4c
📒 Files selected for processing (50)
.gitignoreCLAUDE.mdscripts/build/codegen.tssrc/CLAUDE.mdsrc/codegen/bundle-modules.tssrc/codegen/generate-js2native.tssrc/codegen/replacements.tssrc/js/builtins/Ipc.tssrc/js/builtins/ProcessObjectInternals.tssrc/js/bun/ffi.tssrc/js/internal-for-testing.tssrc/js/internal/assert/myers_diff.tssrc/js/internal/cluster/RoundRobinHandle.tssrc/js/internal/cluster/child.tssrc/js/internal/cluster/primary.tssrc/js/internal/fs/binding.tssrc/js/internal/http.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/util/inspect.jssrc/js/internal/webstreams_adapters.tssrc/js/node/_http2_upgrade.tssrc/js/node/_http_client.tssrc/js/node/_http_server.tssrc/js/node/child_process.tssrc/js/node/crypto.tssrc/js/node/dgram.tssrc/js/node/dns.tssrc/js/node/fs.tssrc/js/node/http2.tssrc/js/node/net.tssrc/js/node/os.tssrc/js/node/tls.tssrc/js/node/util.tssrc/js/node/zlib.tssrc/js/private.d.tssrc/jsc/Counters.rssrc/runtime/api.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/cli/Arguments.rssrc/runtime/dispatch_js2native.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/ffi/mod.rssrc/runtime/generated_js2native.rssrc/runtime/linear_fifo_testing.rssrc/runtime/shell/mod.rssrc/runtime/socket/mod.rssrc/runtime/timer/mod.rs
💤 Files with no reviewable changes (2)
- .gitignore
- src/runtime/shell/mod.rs
|
Re the duplicate flags: #30683 and #30698 are earlier, broader attempts (remove the .zig files wholesale / bulk-rename zig to rust) that have been idle for a month. This PR implements the design requested in #32210 ($native naming, .rs keys, .zig references left in place but decoupled) and intentionally leaves the wholesale .zig source removal to #30683. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codegen/cppbind.ts (1)
1154-1159: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix the CI-only
resultFilePathcrash.This branch still references
resultFilePath, which no longer exists. In CI that throws after writing outputs, so the codegen step exits 1 and breaks the build.🤖 Prompt for 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. In `@src/codegen/cppbind.ts` around lines 1154 - 1159, The log references the removed variable resultFilePath, causing a crash in CI; replace that reference with the correct existing output variable (or remove the path altogether). Locate the logging block in cppbind.ts (the snippet using resultFilePath) and either: 1) use the actual variable that holds the generated file path (e.g., resultFile or resultFiles if present in the surrounding scope) or 2) change the message to omit the path and instead print a safe summary (like number of generated files or a generic "generated bindings"). Ensure no remaining references to resultFilePath remain.Source: Pipeline failures
🤖 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/codegen/bindgenv2/script.ts`:
- Around line 56-60: Update listOutputs(), generate(), and the duplicate-name
check to remove Zig-specific behavior: in listOutputs() include every emitted
C++ artifact (both cppSourcePath(type) and cppHeader(type) when present) or
alternatively make generate() explicitly reject header-only types if headers
should not be treated as build outputs; in generate() ensure headers are
actually written when cppHeader exists (so clean builds declare them) and stop
referencing toZigNamespace(type.name) in the duplicate-name collision
logic—dedupe solely on type.name (from getNamedExports()) so two distinct types
are not rejected due to the removed Zig namespace mapping; adjust the
collision-check code (the block that used to reserve toZigNamespace and the
section around lines 89-95) accordingly.
In `@src/codegen/cppbind.ts`:
- Line 84: The code still imports and runs the Zig generation/validation (using
bannedTypes/sharedTypes and calling generateZigFn) even though cpp.zig was
removed; stop treating Zig errors as fatal by removing the call to generateZigFn
and the bannedTypes/sharedTypes Zig validation from the binding generation path,
and instead extract only the source-link collection logic you need into the
Rust/C++ path (keep the code that builds cpp.source-links) so the main loop
(where generateBindings/generateCppBinding or generateZigFn is invoked) no
longer executes or depends on generateZigFn or bannedTypes/sharedTypes; delete
the dead Zig-related code and ensure build result only includes cpp.rs and
cpp.source-links.
---
Outside diff comments:
In `@src/codegen/cppbind.ts`:
- Around line 1154-1159: The log references the removed variable resultFilePath,
causing a crash in CI; replace that reference with the correct existing output
variable (or remove the path altogether). Locate the logging block in cppbind.ts
(the snippet using resultFilePath) and either: 1) use the actual variable that
holds the generated file path (e.g., resultFile or resultFiles if present in the
surrounding scope) or 2) change the message to omit the path and instead print a
safe summary (like number of generated files or a generic "generated bindings").
Ensure no remaining references to resultFilePath remain.
🪄 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: 05badd5d-972c-4648-a606-89033adf52e5
📒 Files selected for processing (10)
.gitignorescripts/build/codegen.tsscripts/glob-sources.tssrc/codegen/bindgen.tssrc/codegen/bindgenv2/script.tssrc/codegen/ci_info.tssrc/codegen/cppbind.tssrc/codegen/generate-classes.tssrc/codegen/generate-node-errors.tstest/internal/js2native-codegen.test.ts
💤 Files with no reviewable changes (5)
- src/codegen/generate-node-errors.ts
- src/codegen/bindgen.ts
- src/codegen/ci_info.ts
- .gitignore
- src/codegen/generate-classes.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/GeneratedBindings.zig`:
- Around line 5-72: Update the header comments that currently say "Generated for
'<path>.zig'" to either reference the implementing Rust module or a generic
phrase so they don't imply a Zig source exists; change the comment above each
generated block for NodeModuleModule, bindgen_test, fmt_jsc, BunObject,
DevServer, and bake (look for the comment strings immediately preceding those
structs and the associated create*Callback or Formatter/BracesOptions
declarations) to something like "Generated for '<module>.rs'" or "Generated
bindings (implementation in Rust)". Ensure every occurrence in this file is
updated consistently.
In `@src/jsc/bindings/GeneratedJS2Native.zig`:
- Line 1: Update the file header comment in GeneratedJS2Native.zig to reference
the renamed macro by replacing the `$zig()` mention with `$native()` so the
comment reads that the file is generated based on seen calls to the `$native()`
JS macro; keep the rest of the header text intact.
- Around line 4-388: The exported Zig wrapper symbols use the wrong prefix
(JS2Zig___*); update every exported function name to use the JS2Native__ prefix
instead (e.g., rename
JS2Zig___src_runtime_node_node_assert_binding_zig__generate_workaround to
JS2Native__src_runtime_node_node_assert_binding_zig__generate_workaround and do
the same for all other functions in this file such as
JS2Zig___src_runtime_node_node_fs_binding_zig__createBinding_workaround and
JS2Zig___src_sql_jsc_mysql_zig__createBinding_workaround) so the Zig exports
match the Rust dispatch layer's expected JS2Native__* symbols.
🪄 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: a6c6d584-54ea-4ece-9ee8-ee50837075b0
📒 Files selected for processing (4)
src/jsc/bindings/GeneratedBindings.zigsrc/jsc/bindings/GeneratedJS2Native.zigsrc/runtime/cli/ci_info.rssrc/runtime/cli/mod.rs
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/GeneratedJS2Native.zig:1-3— Removing these two paths from.gitignorewithout deleting the on-disk copies caused git to pick up the stale generated outputs as new tracked files —GeneratedJS2Native.zigandGeneratedBindings.zigare now committed (392 lines each) with the pre-renameJS2Zig___*symbols and@import("../../foo.zig")paths. This contradicts the PR description's "Deleted … itssrc/jsc/bindings/GeneratedJS2Native.zigoutput" and "zero JS2Zig references remain";git rm src/jsc/bindings/Generated{JS2Native,Bindings}.zigrather than committing them.Extended reasoning...
What the bug is
The
.gitignorehunk in this PR removes two entries:-src/jsc/bindings/GeneratedJS2Native.zig -src/jsc/bindings/GeneratedBindings.zig
That removal is intentional — the generators that wrote those files (
getJS2NativeZig()ingenerate-js2native.tsand thewriteIfNotChanged(.../GeneratedBindings.zig)line inbindgen.ts) are deleted in this PR, so nothing will ever produce them again and there is nothing to ignore. However, the diff also shows both files being added as new tracked content:--- /dev/null +++ b/src/jsc/bindings/GeneratedJS2Native.zig @@ -0,0 +1,392 @@ +//! This file is generated by src/codegen/generate-js2native.ts based on seen calls to the $zig() JS macro ... +export fn JS2Zig___src_sql_jsc_mysql_zig__createBinding_workaround(...)
and likewise for
GeneratedBindings.zig.git log --diff-filter=Aconfirms both first became tracked in commit31df412fof this branch, andgit ls-filesshows them as tracked at HEAD.Why it happened
Classic un-gitignore footgun: these were generated artifacts written into
src/(the old Zig compiler refused out-of-tree imports — see the deletedzigFilesGeneratedIntoSrcdoc comment), so they sat on disk in the working tree but were hidden by.gitignore. Dropping the ignore entries made them visible to git, and a subsequentgit addin commit31df412fstaged them as new files. Nothing in CI catches this:.zigis no longer compiled (per CLAUDE.md, Zig files are inert porting references), and the newjs2native-codegen.test.tsonly exercises the.rsresolution path.Why it matters
The committed content is the stale pre-rename codegen output.
GeneratedJS2Native.zigis full ofJS2Zig___*_zig__*symbol names and@import("../../runtime/.../foo.zig")references — exactly the artifacts this PR claims to have eliminated. The PR description states "Deleted the dead Zig emittergetJS2NativeZig()and itssrc/jsc/bindings/GeneratedJS2Native.zigoutput" and "zero JS2Zig references remain"; both claims are contradicted by the 784 lines being added here.GeneratedBindings.zigsimilarly hard-codes@import("../../runtime/node/node_os.zig")paths to porting-reference files. There's no runtime impact (nothing compiles.zig), but it commits dead generated code that the PR's own goal is to remove, and it will confuse anyone grepping forJS2Zigor for whether Zig is still load-bearing.Step-by-step proof
- Before this PR:
src/jsc/bindings/GeneratedJS2Native.zigexists on disk (written bybundle-modules.tson the last codegen run) but is git-ignored. - Commit
d4e74b6deletesgetJS2NativeZig()and thewriteIfNotChanged(js2nativeZigPath, ...)call → nothing regenerates the file, but the stale copy remains on disk. - Commit
31df412f(or an earlier commit in the branch) removes the two.gitignorelines. git statusnow reports both files as untracked;git add -Astages them.- The diff shows
--- /dev/null → +++ b/src/jsc/bindings/GeneratedJS2Native.zig(+392) and the same forGeneratedBindings.zig(+392). grep -c JS2Zig src/jsc/bindings/GeneratedJS2Native.zig→ ~90 hits, contradicting "zero JS2Zig references remain".
Fix
git rm src/jsc/bindings/GeneratedJS2Native.zig src/jsc/bindings/GeneratedBindings.zig
Keep the
.gitignoreremoval (it's correct — nothing generates these anymore), just delete the stale on-disk artifacts instead of committing them. - Before this PR:
|
Re the stale generated .zig artifacts flagged above: fixed in b624021, which landed while that review was running. Both files were git rm'd exactly as suggested; the branch diff against main now contains no .zig files and no JS2Zig references. |
There was a problem hiding this comment.
Thanks for fixing the ci_info comments. I didn't find any issues on this pass, but with 60 files spanning the codegen pipeline, FFI symbol mangling, and nine re-keyed thunk targets, this is one a human should sign off on.
Extended reasoning...
Overview
This PR renames the $zig() / $newZigFunction() JS-to-native binding macros to $native() / $newNativeFunction(), re-keys the codegen resolver from .zig filenames to .rs filenames, and changes the exported FFI symbol prefix from JS2Zig__* to JS2Native__*. It touches 60 files: the codegen scripts (generate-js2native.ts, replacements.ts, bundle-modules.ts, bindgen.ts, bindgenv2/script.ts, cppbind.ts, generate-classes.ts, generate-node-errors.ts), the ninja build wiring (scripts/build/codegen.ts, scripts/glob-sources.ts), ~108 call sites in src/js/, the Rust dispatch landing pad (dispatch_js2native.rs), several Rust module visibility/alias changes, and deletes the ci_info.ts generator plus all dead .zig output edges. A new contract test (test/internal/js2native-codegen.test.ts) exercises the macro lowering and key resolution.
Security risks
None identified. This is a build-time codegen/symbol-naming refactor with no runtime data handling, auth, crypto, or user-input parsing changes. The FFI surface is renamed but not widened.
Level of scrutiny
High. The codegen scripts here are load-bearing for every build: they generate the C++ extern declarations and Rust #[no_mangle] thunks that link the JS module registry to native code. A mismatched symbol name, a wrong crate:: path derivation, or a dropped ninja output edge breaks the build (or worse, silently relies on stale local artifacts). Nine call sites were re-keyed to non-sibling .rs files with corresponding dispatch-alias renames and module-visibility changes (socket_body made pub, alias modules deleted), and getUseSystemCA was moved into Arguments.rs. Each of these is individually small but the composition is the kind of change where a second pair of eyes on the symbol/path derivation rules and the re-keyed targets is valuable.
Other factors
My earlier nit (stale ci_info.ts references in ci_info.rs / cli/mod.rs) was addressed in 31df412, and commit b624021 cleaned up accidentally-committed stale generated .zig artifacts that CodeRabbit flagged. All inline review threads are resolved. The PR description documents thorough verification (cargo check, full bun bd, smoke scripts on every re-keyed binding, test-suite parity with released bun), and the new contract test pins the macro/key/symbol behavior. CI on 31df412 still shows musl LTO link failures ("Linking two modules of different data layouts"), which look like build-infra rather than this change, but should be confirmed before merge. Given the breadth and the build-critical nature, I'm deferring rather than approving.
There was a problem hiding this comment.
No issues found on this pass and the earlier ci_info comment cleanup landed — but this rewires the entire JS↔native codegen/FFI dispatch (symbol mangling, .rs key resolution, dispatch re-exports, and removal of several codegen edges) across 60 files, so it's worth a human sign-off.
Extended reasoning...
Overview
This PR renames the $zig()/$newZigFunction() binding macros to $native()/$newNativeFunction() and re-keys them off .rs files instead of .zig files. It touches 60 files: the core codegen machinery (generate-js2native.ts, replacements.ts, bundle-modules.ts, scripts/build/codegen.ts, bindgen.ts, bindgenv2/script.ts, cppbind.ts, generate-classes.ts, generate-node-errors.ts), ~108 call sites across src/js/, the Rust dispatch landing pad (dispatch_js2native.rs), hand-exported symbols (dns.rs), module visibility in socket/mod.rs, shell/mod.rs, ffi/mod.rs, and a new function in cli/Arguments.rs. It also deletes the ci_info.ts generator and several dead .zig output edges, and adds a new contract test.
Security risks
None identified. This is a build-time codegen rename; no auth, crypto, permissions, or user-input handling is touched. The FFI symbol names change but both sides (C++ externs and Rust #[no_mangle] thunks) are emitted by the same generator, so drift would be a link error rather than a silent mis-dispatch.
Level of scrutiny
High. While most of the diff is mechanical (.zig → .rs extension swaps at call sites), the substantive changes are in production-critical build infrastructure: the JS-to-native dispatch table that every built-in module's native binding flows through, the symbol-mangling scheme shared between C++ and Rust, the rustTarget() path-derivation logic, the new ambiguous-basename guard, removal of the crate::shell::shell / crate::ffi::ffi / crate::socket::socket alias modules in favor of direct path derivation, the move of get_use_system_ca from the dispatch pad into cli/Arguments.rs, and the removal of multiple ninja codegen edges (zigFilesGeneratedIntoSrc, emitCiInfo, bindgenv2 .zig outputs, cpp.zig, ZigGeneratedClasses.zig, ErrorCode.zig, GeneratedBindings.zig). A mistake here breaks the build for everyone or silently mis-routes a native binding.
Other factors
The PR has already been through several rounds of feedback (alii, coderabbit, and my own prior inline note about stale ci_info.ts comments — all now resolved in commits d4e74b6, b624021, 31df412, d5b2aca). The description documents thorough verification (cargo check, full bun bd, smoke scripts for every re-keyed binding, test-suite parity against released bun) and a new test/internal/js2native-codegen.test.ts covers the macro lowering and key-resolution contract. The bug-hunting system found nothing on this pass. That all raises confidence considerably — but the scope (every native binding in the runtime, plus structural codegen/build-graph changes) puts this well outside the "simple/mechanical/obvious" bar for bot approval, and there are related broader PRs (#30683, #30698) whose interaction with this one a human should confirm.
|
CI status: the diff is green. Every build-rust lane and the full linux / darwin-x64 / darwin-26 test matrix pass. The only red is transient flake on two lanes, and the failing tests differ between runs (none related to this codegen/symbol rename):
This PR touches only build-time codegen and FFI symbol naming; it has no darwin-specific, HTTP/body, MySQL, Valkey, or test-runner behavior. A real codegen/symbol problem would fail the build-rust lanes, which are green on both runs. I have used my one CI re-roll (77e4531). Ready for a maintainer to merge. |
robobun
force-pushed
the
farm/1c84b455/rename-zig-macro-to-native
branch
from
77e4531 to
86a55f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/build/codegen.ts (1)
711-744:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack the Rust filename set for JS2Native generation.
bundle-modules.tsimportsgenerate-js2native.ts, which now scans.rspaths to resolve$nativekeys and producegenerated_js2native.rs/types. This edge does not depend on the Rust file set, so adding/removing/renaming an.rsfile can leave stale ambiguity checks, symbol paths, andNativeFilenameRustoutput until some JS/codegen input changes.Suggested fix
function emitJsModules({ n, cfg, sources, o, dirStamp }: Ctx): void { const script = resolve(cfg.cwd, "src", "codegen", "bundle-modules.ts"); // InternalModuleRegistry.cpp is read by the script (for a sanity check). const extraInput = resolve(cfg.cwd, "src", "jsc", "bindings", "InternalModuleRegistry.cpp"); // replacements.ts bakes ErrorCode.ts indices into every bundled module // ($makeErrorWithCode(N, ...)); without this dep an ErrorCode.ts edit leaves // stale error numbers in the JS bundles while the C++ enum regenerates. const errorCodeInput = resolve(cfg.cwd, "src", "jsc", "bindings", "ErrorCode.ts"); + mkdirSync(cfg.codegenDir, { recursive: true }); + const js2nativeRustSources = resolve(cfg.codegenDir, "js2native-rust-sources.txt"); + writeIfChanged( + js2nativeRustSources, + sources.rust + .filter(p => p.endsWith(".rs")) + .map(p => relative(cfg.cwd, p).replace(/\\/g, "/")) + .join("\n") + "\n", + ); const outputs = [ @@ n.build({ outputs, rule: "codegen", inputs: [script, ...sources.js, ...sources.jsCodegen, extraInput, errorCodeInput], + implicitInputs: [js2nativeRustSources], orderOnlyInputs: [dirStamp],As per coding guidelines, "Cache keys cover every input that shapes the output" and build scripts should use explicit/implicit inputs for invalidation signals.
🤖 Prompt for 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. In `@scripts/build/codegen.ts` around lines 711 - 744, The emitJsModules function's n.build() call is missing the Rust source file set from its inputs array. Since generate-js2native.ts scans .rs file paths to produce generated_js2native.rs and related output, add the Rust source files (similar to how sources.js and sources.jsCodegen are currently included) to the inputs array so that adding, removing, or renaming .rs files will properly trigger a rebuild and prevent stale generated output.Source: Coding guidelines
src/runtime/api/bun/h2_frame_parser.rs (1)
7740-7758:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t stop cleanup without scheduling the deferred teardown.
Returning early here avoids the UAF, but it also leaves
read_buffer/write_buffer/HPACK alive forever if no laterdetach()runs afterread_dispatch_depthdrops back to zero. A teardown triggered from a frame handler should mark pending cleanup and finish it once the dispatch unwinds.🤖 Prompt for 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. In `@src/runtime/api/bun/h2_frame_parser.rs` around lines 7740 - 7758, The early return when read_dispatch_depth is greater than zero prevents use-after-free but leaves buffers and HPACK permanently allocated if detach() never runs after the dispatch unwinds. Instead of just returning early, introduce a flag to mark that a deferred teardown is pending when returning from the dispatch context, then implement logic to actually perform the cleanup (clearing read_buffer, write_buffer, and setting hpack to None) once read_dispatch_depth drops back to zero, ensuring the resources are properly freed when the dispatch context completes.src/js/node/net.ts (1)
838-841:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard TLS error emission for standalone server-side TLSSocket.
The new standalone
::bunUpgradeServerTLS::path can useServerHandlerswith no owning server. The TLS error branch still callsthis.server.emit(...), so a post-handshake TLS error can turn intoTypeError: cannot read property 'emit'instead of surfacing the TLS failure.🛠️ Proposed fix
- this.server.emit("tlsClientError", error, data); + (this.server ?? data.server)?.emit("tlsClientError", error, data);Also applies to: 1897-1908
🤖 Prompt for 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. In `@src/js/node/net.ts` around lines 838 - 841, The TLS error handling code at the location with `this.server.emit("tlsClientError", error, data)` does not guard against cases where the standalone server-side TLSSocket has no owning server, which can cause a TypeError. Add a null/undefined check for `this.server` before calling the emit method to ensure it exists, and apply the same guard pattern to all similar occurrences mentioned in the range 1897-1908 where `this.server` is accessed.
🤖 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/codegen/generate-js2native.ts`:
- Around line 348-352: The code currently adds all bare basenames to the
rustNames set unconditionally, but ambiguous basenames (those appearing multiple
times across different directories) should be excluded since the resolver
rejects them. First create a counting pass to tally basename occurrences across
all sourceFiles, then in the main loop that populates rustNames, only call
rustNames.add(basename(file)) when that basename has a count of exactly one,
while always calling rustNames.add for the relative path via
path.relative(srcDir, file).replaceAll(sep, "/") regardless of ambiguity.
In `@src/js/internal/webstreams_adapters.ts`:
- Around line 193-195: The guard condition in the switch case for `causeCode`
checks if it is not null but does not verify it is a string type before calling
StringPrototypeStartsWith.$call, which can throw an error if causeCode is a
number, symbol, or object. Add a type guard to ensure causeCode is a string
(such as typeof causeCode === 'string') in the condition alongside the existing
causeCode != null check, so that StringPrototypeStartsWith.$call is only called
on actual string values.
- Around line 869-873: The validateBufferSourceChunk method in the
kValidateChunk function currently only rejects SharedArrayBuffer-backed data but
does not enforce that the chunk is actually a valid BufferSource type
(ArrayBuffer, Buffer, TypedArray, or DataView). Add type validation to first
check whether the chunk is one of the valid BufferSource types before proceeding
with the SharedArrayBuffer check. This ensures non-BufferSource values are
properly rejected and prevents invalid inputs from passing through the guard.
In `@src/js/node/http2.ts`:
- Around line 4056-4062: The condition checking `options.signal` uses truthiness
evaluation, which skips validateAbortSignal() for falsy values like null, false,
0, or empty strings. This allows invalid signal values to pass without throwing
argument-type errors. Change the condition from checking `options.signal`
(truthiness) to checking for the actual presence of the signal property (such as
using `'signal' in options` or checking against undefined) so that
validateAbortSignal() is called for all signal values, including those that are
falsy but still need validation.
In `@src/js/node/net.ts`:
- Around line 617-623: The ALPN protocol parsing loop in this code does not
validate that the length-prefixed data from the network-controlled
`protocolsWire` buffer stays within bounds before slicing and converting. Add a
bounds check immediately after reading the length prefix `n` from `wire[i]` to
ensure that `i + 1 + n` does not exceed `wire.length`. If this validation fails,
reject the malformed data and return early before invoking the callback, rather
than allowing `toString()` to silently truncate the protocol and pass invalid
data downstream. Place this validation check before any further processing or
side effects on the protocols array.
- Around line 578-588: The consumeSNIResult() function call in onSNIResolution()
can throw errors from user code execution via context getters/proxies, but these
errors are not being caught. If consumeSNIResult() throws, the function exits
without calling resumeSNI(), leaving the connection suspended. Wrap the
consumeSNIResult(state, err, context) call in a try-catch block, and in the
catch handler, set state.failed to the caught error so that the subsequent logic
correctly calls resumeSNI() with error handling. This ensures resumeSNI() is
always invoked to resume the parked handshake regardless of whether
consumeSNIResult() succeeds or throws.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 1270-1275: The newly added last_peer_stream_id field is not being
used in the JS-visible public API surfaces, so session.goaway() and
state.lastProcStreamID are still reading from last_stream_id instead of
last_peer_stream_id, causing them to report the wrong value. Locate the
implementations of the session.goaway() method and the state.lastProcStreamID
property accessor, and update both to read from the last_peer_stream_id field
instead of last_stream_id to ensure they report the peer-processed stream ID
rather than the local next-stream counter.
In `@src/runtime/api/bun/SecureContext.rs`:
- Around line 92-122: The parse_pkcs12 function only checks if args is empty but
does not explicitly reject null or undefined values passed for the PFX
certificate argument, causing these values to be stringified via to_slice() and
passed to OpenSSL instead of throwing a proper argument error. Add a check to
verify that args[0] is not null or undefined (similar to the check used for the
passphrase at args[1]) before attempting to process it, and throw an error with
a clear message if the certificate argument is explicitly null or undefined.
- Around line 342-376: The add_ca_cert method modifies the underlying SSL
context by adding CA certificates but does not update the extra_memory field
used for garbage collection accounting. After successfully adding the CA
certificate with the c::us_ssl_ctx_add_ca_cert call (when ok is not 0),
increment the this.extra_memory field by the size of the certificate bytes being
added (bytes.len()) so that the garbage collector has an accurate account of the
increased memory consumption.
---
Outside diff comments:
In `@scripts/build/codegen.ts`:
- Around line 711-744: The emitJsModules function's n.build() call is missing
the Rust source file set from its inputs array. Since generate-js2native.ts
scans .rs file paths to produce generated_js2native.rs and related output, add
the Rust source files (similar to how sources.js and sources.jsCodegen are
currently included) to the inputs array so that adding, removing, or renaming
.rs files will properly trigger a rebuild and prevent stale generated output.
In `@src/js/node/net.ts`:
- Around line 838-841: The TLS error handling code at the location with
`this.server.emit("tlsClientError", error, data)` does not guard against cases
where the standalone server-side TLSSocket has no owning server, which can cause
a TypeError. Add a null/undefined check for `this.server` before calling the
emit method to ensure it exists, and apply the same guard pattern to all similar
occurrences mentioned in the range 1897-1908 where `this.server` is accessed.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 7740-7758: The early return when read_dispatch_depth is greater
than zero prevents use-after-free but leaves buffers and HPACK permanently
allocated if detach() never runs after the dispatch unwinds. Instead of just
returning early, introduce a flag to mark that a deferred teardown is pending
when returning from the dispatch context, then implement logic to actually
perform the cleanup (clearing read_buffer, write_buffer, and setting hpack to
None) once read_dispatch_depth drops back to zero, ensuring the resources are
properly freed when the dispatch context completes.
🪄 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: 07362376-c31b-41dc-bd7b-57b12cb932f5
📒 Files selected for processing (60)
.gitignoreCLAUDE.mdscripts/build/codegen.tsscripts/glob-sources.tssrc/CLAUDE.mdsrc/codegen/bindgen.tssrc/codegen/bindgenv2/script.tssrc/codegen/bundle-modules.tssrc/codegen/ci_info.tssrc/codegen/cppbind.tssrc/codegen/generate-classes.tssrc/codegen/generate-js2native.tssrc/codegen/generate-node-errors.tssrc/codegen/replacements.tssrc/js/builtins/Ipc.tssrc/js/builtins/ProcessObjectInternals.tssrc/js/bun/ffi.tssrc/js/internal-for-testing.tssrc/js/internal/assert/myers_diff.tssrc/js/internal/cluster/RoundRobinHandle.tssrc/js/internal/cluster/child.tssrc/js/internal/cluster/primary.tssrc/js/internal/fs/binding.tssrc/js/internal/http.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/util/inspect.jssrc/js/internal/webstreams_adapters.tssrc/js/node/_http2_upgrade.tssrc/js/node/_http_client.tssrc/js/node/_http_server.tssrc/js/node/child_process.tssrc/js/node/crypto.tssrc/js/node/dgram.tssrc/js/node/dns.tssrc/js/node/fs.tssrc/js/node/http2.tssrc/js/node/net.tssrc/js/node/os.tssrc/js/node/tls.tssrc/js/node/util.tssrc/js/node/zlib.tssrc/js/private.d.tssrc/jsc/Counters.rssrc/runtime/api.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/ci_info.rssrc/runtime/cli/mod.rssrc/runtime/dispatch_js2native.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/ffi/mod.rssrc/runtime/generated_js2native.rssrc/runtime/linear_fifo_testing.rssrc/runtime/shell/mod.rssrc/runtime/socket/mod.rssrc/runtime/timer/mod.rstest/internal/js2native-codegen.test.ts
💤 Files with no reviewable changes (7)
- src/codegen/generate-node-errors.ts
- src/codegen/bindgen.ts
- src/codegen/generate-classes.ts
- test/internal/js2native-codegen.test.ts
- src/codegen/ci_info.ts
- src/runtime/shell/mod.rs
- .gitignore
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/build/codegen.ts (1)
711-744:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack the Rust filename set for JS2Native generation.
bundle-modules.tsimportsgenerate-js2native.ts, which now scans.rspaths to resolve$nativekeys and producegenerated_js2native.rs/types. This edge does not depend on the Rust file set, so adding/removing/renaming an.rsfile can leave stale ambiguity checks, symbol paths, andNativeFilenameRustoutput until some JS/codegen input changes.Suggested fix
function emitJsModules({ n, cfg, sources, o, dirStamp }: Ctx): void { const script = resolve(cfg.cwd, "src", "codegen", "bundle-modules.ts"); // InternalModuleRegistry.cpp is read by the script (for a sanity check). const extraInput = resolve(cfg.cwd, "src", "jsc", "bindings", "InternalModuleRegistry.cpp"); // replacements.ts bakes ErrorCode.ts indices into every bundled module // ($makeErrorWithCode(N, ...)); without this dep an ErrorCode.ts edit leaves // stale error numbers in the JS bundles while the C++ enum regenerates. const errorCodeInput = resolve(cfg.cwd, "src", "jsc", "bindings", "ErrorCode.ts"); + mkdirSync(cfg.codegenDir, { recursive: true }); + const js2nativeRustSources = resolve(cfg.codegenDir, "js2native-rust-sources.txt"); + writeIfChanged( + js2nativeRustSources, + sources.rust + .filter(p => p.endsWith(".rs")) + .map(p => relative(cfg.cwd, p).replace(/\\/g, "/")) + .join("\n") + "\n", + ); const outputs = [ @@ n.build({ outputs, rule: "codegen", inputs: [script, ...sources.js, ...sources.jsCodegen, extraInput, errorCodeInput], + implicitInputs: [js2nativeRustSources], orderOnlyInputs: [dirStamp],As per coding guidelines, "Cache keys cover every input that shapes the output" and build scripts should use explicit/implicit inputs for invalidation signals.
🤖 Prompt for 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. In `@scripts/build/codegen.ts` around lines 711 - 744, The emitJsModules function's n.build() call is missing the Rust source file set from its inputs array. Since generate-js2native.ts scans .rs file paths to produce generated_js2native.rs and related output, add the Rust source files (similar to how sources.js and sources.jsCodegen are currently included) to the inputs array so that adding, removing, or renaming .rs files will properly trigger a rebuild and prevent stale generated output.Source: Coding guidelines
src/runtime/api/bun/h2_frame_parser.rs (1)
7740-7758:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t stop cleanup without scheduling the deferred teardown.
Returning early here avoids the UAF, but it also leaves
read_buffer/write_buffer/HPACK alive forever if no laterdetach()runs afterread_dispatch_depthdrops back to zero. A teardown triggered from a frame handler should mark pending cleanup and finish it once the dispatch unwinds.🤖 Prompt for 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. In `@src/runtime/api/bun/h2_frame_parser.rs` around lines 7740 - 7758, The early return when read_dispatch_depth is greater than zero prevents use-after-free but leaves buffers and HPACK permanently allocated if detach() never runs after the dispatch unwinds. Instead of just returning early, introduce a flag to mark that a deferred teardown is pending when returning from the dispatch context, then implement logic to actually perform the cleanup (clearing read_buffer, write_buffer, and setting hpack to None) once read_dispatch_depth drops back to zero, ensuring the resources are properly freed when the dispatch context completes.src/js/node/net.ts (1)
838-841:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard TLS error emission for standalone server-side TLSSocket.
The new standalone
::bunUpgradeServerTLS::path can useServerHandlerswith no owning server. The TLS error branch still callsthis.server.emit(...), so a post-handshake TLS error can turn intoTypeError: cannot read property 'emit'instead of surfacing the TLS failure.🛠️ Proposed fix
- this.server.emit("tlsClientError", error, data); + (this.server ?? data.server)?.emit("tlsClientError", error, data);Also applies to: 1897-1908
🤖 Prompt for 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. In `@src/js/node/net.ts` around lines 838 - 841, The TLS error handling code at the location with `this.server.emit("tlsClientError", error, data)` does not guard against cases where the standalone server-side TLSSocket has no owning server, which can cause a TypeError. Add a null/undefined check for `this.server` before calling the emit method to ensure it exists, and apply the same guard pattern to all similar occurrences mentioned in the range 1897-1908 where `this.server` is accessed.
🤖 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/codegen/generate-js2native.ts`:
- Around line 348-352: The code currently adds all bare basenames to the
rustNames set unconditionally, but ambiguous basenames (those appearing multiple
times across different directories) should be excluded since the resolver
rejects them. First create a counting pass to tally basename occurrences across
all sourceFiles, then in the main loop that populates rustNames, only call
rustNames.add(basename(file)) when that basename has a count of exactly one,
while always calling rustNames.add for the relative path via
path.relative(srcDir, file).replaceAll(sep, "/") regardless of ambiguity.
In `@src/js/internal/webstreams_adapters.ts`:
- Around line 193-195: The guard condition in the switch case for `causeCode`
checks if it is not null but does not verify it is a string type before calling
StringPrototypeStartsWith.$call, which can throw an error if causeCode is a
number, symbol, or object. Add a type guard to ensure causeCode is a string
(such as typeof causeCode === 'string') in the condition alongside the existing
causeCode != null check, so that StringPrototypeStartsWith.$call is only called
on actual string values.
- Around line 869-873: The validateBufferSourceChunk method in the
kValidateChunk function currently only rejects SharedArrayBuffer-backed data but
does not enforce that the chunk is actually a valid BufferSource type
(ArrayBuffer, Buffer, TypedArray, or DataView). Add type validation to first
check whether the chunk is one of the valid BufferSource types before proceeding
with the SharedArrayBuffer check. This ensures non-BufferSource values are
properly rejected and prevents invalid inputs from passing through the guard.
In `@src/js/node/http2.ts`:
- Around line 4056-4062: The condition checking `options.signal` uses truthiness
evaluation, which skips validateAbortSignal() for falsy values like null, false,
0, or empty strings. This allows invalid signal values to pass without throwing
argument-type errors. Change the condition from checking `options.signal`
(truthiness) to checking for the actual presence of the signal property (such as
using `'signal' in options` or checking against undefined) so that
validateAbortSignal() is called for all signal values, including those that are
falsy but still need validation.
In `@src/js/node/net.ts`:
- Around line 617-623: The ALPN protocol parsing loop in this code does not
validate that the length-prefixed data from the network-controlled
`protocolsWire` buffer stays within bounds before slicing and converting. Add a
bounds check immediately after reading the length prefix `n` from `wire[i]` to
ensure that `i + 1 + n` does not exceed `wire.length`. If this validation fails,
reject the malformed data and return early before invoking the callback, rather
than allowing `toString()` to silently truncate the protocol and pass invalid
data downstream. Place this validation check before any further processing or
side effects on the protocols array.
- Around line 578-588: The consumeSNIResult() function call in onSNIResolution()
can throw errors from user code execution via context getters/proxies, but these
errors are not being caught. If consumeSNIResult() throws, the function exits
without calling resumeSNI(), leaving the connection suspended. Wrap the
consumeSNIResult(state, err, context) call in a try-catch block, and in the
catch handler, set state.failed to the caught error so that the subsequent logic
correctly calls resumeSNI() with error handling. This ensures resumeSNI() is
always invoked to resume the parked handshake regardless of whether
consumeSNIResult() succeeds or throws.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 1270-1275: The newly added last_peer_stream_id field is not being
used in the JS-visible public API surfaces, so session.goaway() and
state.lastProcStreamID are still reading from last_stream_id instead of
last_peer_stream_id, causing them to report the wrong value. Locate the
implementations of the session.goaway() method and the state.lastProcStreamID
property accessor, and update both to read from the last_peer_stream_id field
instead of last_stream_id to ensure they report the peer-processed stream ID
rather than the local next-stream counter.
In `@src/runtime/api/bun/SecureContext.rs`:
- Around line 92-122: The parse_pkcs12 function only checks if args is empty but
does not explicitly reject null or undefined values passed for the PFX
certificate argument, causing these values to be stringified via to_slice() and
passed to OpenSSL instead of throwing a proper argument error. Add a check to
verify that args[0] is not null or undefined (similar to the check used for the
passphrase at args[1]) before attempting to process it, and throw an error with
a clear message if the certificate argument is explicitly null or undefined.
- Around line 342-376: The add_ca_cert method modifies the underlying SSL
context by adding CA certificates but does not update the extra_memory field
used for garbage collection accounting. After successfully adding the CA
certificate with the c::us_ssl_ctx_add_ca_cert call (when ok is not 0),
increment the this.extra_memory field by the size of the certificate bytes being
added (bytes.len()) so that the garbage collector has an accurate account of the
increased memory consumption.
---
Outside diff comments:
In `@scripts/build/codegen.ts`:
- Around line 711-744: The emitJsModules function's n.build() call is missing
the Rust source file set from its inputs array. Since generate-js2native.ts
scans .rs file paths to produce generated_js2native.rs and related output, add
the Rust source files (similar to how sources.js and sources.jsCodegen are
currently included) to the inputs array so that adding, removing, or renaming
.rs files will properly trigger a rebuild and prevent stale generated output.
In `@src/js/node/net.ts`:
- Around line 838-841: The TLS error handling code at the location with
`this.server.emit("tlsClientError", error, data)` does not guard against cases
where the standalone server-side TLSSocket has no owning server, which can cause
a TypeError. Add a null/undefined check for `this.server` before calling the
emit method to ensure it exists, and apply the same guard pattern to all similar
occurrences mentioned in the range 1897-1908 where `this.server` is accessed.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 7740-7758: The early return when read_dispatch_depth is greater
than zero prevents use-after-free but leaves buffers and HPACK permanently
allocated if detach() never runs after the dispatch unwinds. Instead of just
returning early, introduce a flag to mark that a deferred teardown is pending
when returning from the dispatch context, then implement logic to actually
perform the cleanup (clearing read_buffer, write_buffer, and setting hpack to
None) once read_dispatch_depth drops back to zero, ensuring the resources are
properly freed when the dispatch context completes.
🪄 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: 07362376-c31b-41dc-bd7b-57b12cb932f5
📒 Files selected for processing (60)
.gitignoreCLAUDE.mdscripts/build/codegen.tsscripts/glob-sources.tssrc/CLAUDE.mdsrc/codegen/bindgen.tssrc/codegen/bindgenv2/script.tssrc/codegen/bundle-modules.tssrc/codegen/ci_info.tssrc/codegen/cppbind.tssrc/codegen/generate-classes.tssrc/codegen/generate-js2native.tssrc/codegen/generate-node-errors.tssrc/codegen/replacements.tssrc/js/builtins/Ipc.tssrc/js/builtins/ProcessObjectInternals.tssrc/js/bun/ffi.tssrc/js/internal-for-testing.tssrc/js/internal/assert/myers_diff.tssrc/js/internal/cluster/RoundRobinHandle.tssrc/js/internal/cluster/child.tssrc/js/internal/cluster/primary.tssrc/js/internal/fs/binding.tssrc/js/internal/http.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/util/inspect.jssrc/js/internal/webstreams_adapters.tssrc/js/node/_http2_upgrade.tssrc/js/node/_http_client.tssrc/js/node/_http_server.tssrc/js/node/child_process.tssrc/js/node/crypto.tssrc/js/node/dgram.tssrc/js/node/dns.tssrc/js/node/fs.tssrc/js/node/http2.tssrc/js/node/net.tssrc/js/node/os.tssrc/js/node/tls.tssrc/js/node/util.tssrc/js/node/zlib.tssrc/js/private.d.tssrc/jsc/Counters.rssrc/runtime/api.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/ci_info.rssrc/runtime/cli/mod.rssrc/runtime/dispatch_js2native.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/ffi/mod.rssrc/runtime/generated_js2native.rssrc/runtime/linear_fifo_testing.rssrc/runtime/shell/mod.rssrc/runtime/socket/mod.rssrc/runtime/timer/mod.rstest/internal/js2native-codegen.test.ts
💤 Files with no reviewable changes (7)
- src/codegen/generate-node-errors.ts
- src/codegen/bindgen.ts
- src/codegen/generate-classes.ts
- test/internal/js2native-codegen.test.ts
- src/codegen/ci_info.ts
- src/runtime/shell/mod.rs
- .gitignore
🛑 Comments failed to post (9)
src/codegen/generate-js2native.ts (1)
348-352:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
NativeFilenameRustaligned with ambiguity rejection.The resolver rejects ambiguous Rust basenames, but the DTS union still includes every bare basename. Count basenames first and only add a bare name when it resolves uniquely; always add the
src/-relative path.Suggested fix
const srcDir = path.join(import.meta.dir, ".."); const rustNames = new Set<string>(); + const basenameCounts = new Map<string, number>(); + const rustFiles = sourceFiles.filter(file => file.endsWith(".rs")); + for (const file of rustFiles) { + const name = basename(file); + basenameCounts.set(name, (basenameCounts.get(name) ?? 0) + 1); + } - for (const file of sourceFiles) { - if (!file.endsWith(".rs")) continue; - rustNames.add(basename(file)); + for (const file of rustFiles) { + const name = basename(file); + if (basenameCounts.get(name) === 1) rustNames.add(name); rustNames.add(path.relative(srcDir, file).replaceAll(sep, "/")); }🤖 Prompt for 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. In `@src/codegen/generate-js2native.ts` around lines 348 - 352, The code currently adds all bare basenames to the rustNames set unconditionally, but ambiguous basenames (those appearing multiple times across different directories) should be excluded since the resolver rejects them. First create a counting pass to tally basename occurrences across all sourceFiles, then in the main loop that populates rustNames, only call rustNames.add(basename(file)) when that basename has a count of exactly one, while always calling rustNames.add for the relative path via path.relative(srcDir, file).replaceAll(sep, "/") regardless of ambiguity.src/js/internal/webstreams_adapters.ts (2)
193-195:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
cause.codeas a string before callingstartsWith.
causeCode != nullallows non-strings (number/symbol/object), andStringPrototypeStartsWith.$call(causeCode, ...)can throw here, masking the original stream error path.Suggested fix
- case causeCode != null && + case typeof causeCode === "string" && (StringPrototypeStartsWith.$call(causeCode, "ERR__ERROR_") || StringPrototypeStartsWith.$call(causeCode, "ERR_BROTLI_DECODER_ERROR_")): {🤖 Prompt for 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. In `@src/js/internal/webstreams_adapters.ts` around lines 193 - 195, The guard condition in the switch case for `causeCode` checks if it is not null but does not verify it is a string type before calling StringPrototypeStartsWith.$call, which can throw an error if causeCode is a number, symbol, or object. Add a type guard to ensure causeCode is a string (such as typeof causeCode === 'string') in the condition alongside the existing causeCode != null check, so that StringPrototypeStartsWith.$call is only called on actual string values.
869-873:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce full BufferSource type validation for Compression/Decompression inputs.
This validator currently rejects only SharedArrayBuffer-backed data. Non-BufferSource values can still pass this guard, which violates the stated contract here and can miss the intended synchronous dual-side error behavior.
Suggested fix
function newBufferSourceTransformPairFromDuplex(duplex) { const { isArrayBufferView, isSharedArrayBuffer } = require("node:util/types"); return newReadableWritablePairFromDuplex(duplex, { [kValidateChunk]: function validateBufferSourceChunk(chunk) { - if (isSharedArrayBuffer(isArrayBufferView(chunk) ? chunk.buffer : chunk)) { + const isBufferSource = isAnyArrayBuffer(chunk) || isArrayBufferView(chunk); + if (!isBufferSource || isSharedArrayBuffer(isArrayBufferView(chunk) ? chunk.buffer : chunk)) { throw $ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer", "TypedArray", "DataView"], chunk); } }, [kDestroyOnSyncError]: true, }); }🤖 Prompt for 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. In `@src/js/internal/webstreams_adapters.ts` around lines 869 - 873, The validateBufferSourceChunk method in the kValidateChunk function currently only rejects SharedArrayBuffer-backed data but does not enforce that the chunk is actually a valid BufferSource type (ArrayBuffer, Buffer, TypedArray, or DataView). Add type validation to first check whether the chunk is one of the valid BufferSource types before proceeding with the SharedArrayBuffer check. This ensures non-BufferSource values are properly rejected and prevents invalid inputs from passing through the guard.src/js/node/http2.ts (1)
4056-4062:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
options.signalby presence, not truthiness.Line 4056 skips
validateAbortSignal()for falsy values (null,false,0,""), so invalidoptions.signalvalues can be silently accepted instead of throwing argument-type errors.Suggested fix
- if ($isObject(options) && options.signal) { + if ($isObject(options) && options.signal !== undefined) { // Node validates the signal before reading .aborted: any object with an // 'aborted' property passes (so a duck-typed { aborted: true } takes // the abort fast path), while objects without one and non-objects // throw ERR_INVALID_ARG_TYPE synchronously. validateAbortSignal(options.signal, "options.signal");🤖 Prompt for 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. In `@src/js/node/http2.ts` around lines 4056 - 4062, The condition checking `options.signal` uses truthiness evaluation, which skips validateAbortSignal() for falsy values like null, false, 0, or empty strings. This allows invalid signal values to pass without throwing argument-type errors. Change the condition from checking `options.signal` (truthiness) to checking for the actual presence of the signal property (such as using `'signal' in options` or checking against undefined) so that validateAbortSignal() is called for all signal values, including those that are falsy but still need validation.src/js/node/net.ts (2)
578-588:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch SNI result coercion errors before resuming the parked handshake.
For asynchronous
SNICallbackcompletions,consumeSNIResult()can still run user code viacontext.contextgetters/proxies. If that throws here,resumeSNI()is skipped and the connection remains suspended.🛠️ Proposed fix
if (state.settled) return; // an SNICallback must resolve exactly once state.settled = true; - consumeSNIResult(state, err, context); + try { + consumeSNIResult(state, err, context); + } catch (err) { + state.failed = toSNIError(err); + } if (!state.suspended) return; // synchronous resolution - serverName's return carries it🤖 Prompt for 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. In `@src/js/node/net.ts` around lines 578 - 588, The consumeSNIResult() function call in onSNIResolution() can throw errors from user code execution via context getters/proxies, but these errors are not being caught. If consumeSNIResult() throws, the function exits without calling resumeSNI(), leaving the connection suspended. Wrap the consumeSNIResult(state, err, context) call in a try-catch block, and in the catch handler, set state.failed to the caught error so that the subsequent logic correctly calls resumeSNI() with error handling. This ensures resumeSNI() is always invoked to resume the parked handshake regardless of whether consumeSNIResult() succeeds or throws.
617-623:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate ALPN length prefixes before slicing peer data.
protocolsWireis network-controlled length-prefixed data; ifnextends pastwire.length,toString()truncates and the callback sees a protocol the peer did not validly offer. Refuse malformed vectors before invoking_ALPNCallback.🛡️ Proposed fix
- for (let i = 0; i + 1 <= wire.length; ) { - const n = wire[i]; - protocols.push(wire.toString("latin1", i + 1, i + 1 + n)); - i += 1 + n; + for (let i = 0; i < wire.length; ) { + const n = wire[i++]; + if (n === 0 || i + n > wire.length) { + return undefined; + } + protocols.push(wire.toString("latin1", i, i + n)); + i += n; }As per coding guidelines, “Validate untrusted input BEFORE any processing, allocation, or side effect.”
🤖 Prompt for 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. In `@src/js/node/net.ts` around lines 617 - 623, The ALPN protocol parsing loop in this code does not validate that the length-prefixed data from the network-controlled `protocolsWire` buffer stays within bounds before slicing and converting. Add a bounds check immediately after reading the length prefix `n` from `wire[i]` to ensure that `i + 1 + n` does not exceed `wire.length`. If this validation fails, reject the malformed data and return early before invoking the callback, rather than allowing `toString()` to silently truncate the protocol and pass invalid data downstream. Place this validation check before any further processing or side effects on the protocols array.Source: Coding guidelines
src/runtime/api/bun/h2_frame_parser.rs (1)
1270-1275:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread
last_peer_stream_idthrough the public surfaces.This field is updated, but the JS-visible GOAWAY/state paths still read
last_stream_id, sosession.goaway()andstate.lastProcStreamIDcan still report the local next-stream counter instead of the peer-processed one.🤖 Prompt for 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. In `@src/runtime/api/bun/h2_frame_parser.rs` around lines 1270 - 1275, The newly added last_peer_stream_id field is not being used in the JS-visible public API surfaces, so session.goaway() and state.lastProcStreamID are still reading from last_stream_id instead of last_peer_stream_id, causing them to report the wrong value. Locate the implementations of the session.goaway() method and the state.lastProcStreamID property accessor, and update both to read from the last_peer_stream_id field instead of last_stream_id to ensure they report the peer-processed stream ID rather than the local next-stream counter.src/runtime/api/bun/SecureContext.rs (2)
92-122:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject explicit
null/undefinedbefore coercion.
args.is_empty()only catches a missing slot. Both entry points still stringifyundefined/nullviato_slice()and hand that garbage to OpenSSL instead of surfacing an argument error.Suggested fix
- if args.is_empty() { + if args.is_empty() || args[0].is_undefined_or_null() { return Err(global.throw(format_args!("PFX certificate argument is mandatory"))); }- if args.is_empty() { + if args.is_empty() || args[0].is_undefined_or_null() { return Err( global.throw_invalid_arguments(format_args!("addCACert requires a certificate")) ); }Also applies to: 342-363
🤖 Prompt for 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. In `@src/runtime/api/bun/SecureContext.rs` around lines 92 - 122, The parse_pkcs12 function only checks if args is empty but does not explicitly reject null or undefined values passed for the PFX certificate argument, causing these values to be stringified via to_slice() and passed to OpenSSL instead of throwing a proper argument error. Add a check to verify that args[0] is not null or undefined (similar to the check used for the passphrase at args[1]) before attempting to process it, and throw an error with a clear message if the certificate argument is explicitly null or undefined.
342-376:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep
extra_memoryin sync withaddCACert.
memory_cost()is the GC accounting hook, but this mutator increases the underlying SSL_CTX and never updates the cached byte estimate. Any context that adds CA material later will be under-accounted until collection, so GC pressure drifts from reality.🤖 Prompt for 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. In `@src/runtime/api/bun/SecureContext.rs` around lines 342 - 376, The add_ca_cert method modifies the underlying SSL context by adding CA certificates but does not update the extra_memory field used for garbage collection accounting. After successfully adding the CA certificate with the c::us_ssl_ctx_add_ca_cert call (when ok is not 0), increment the this.extra_memory field by the size of the certificate bytes being added (bytes.len()) so that the garbage collector has an accurate account of the increased memory consumption.
There was a problem hiding this comment.
Both earlier nits are addressed (4d13092, 38a69d9) and no further issues found — deferring to a human for the merge given this rewires the codegen/FFI symbol contract across 61 files and overlaps with #30683/#30698.
Extended reasoning...
Overview
This PR renames the $zig()/$newZigFunction() JS-to-native macros to $native()/$newNativeFunction(), switches generate-js2native.ts to resolve .rs files instead of .zig, changes the mangled symbol prefix from JS2Zig__* to JS2Native__*, and removes the dead Zig emitters from bundle-modules.ts, bindgen.ts, bindgenv2/script.ts, cppbind.ts, generate-classes.ts, generate-node-errors.ts, and scripts/build/codegen.ts (including deleting src/codegen/ci_info.ts and the zigFilesGeneratedIntoSrc mechanism). It updates all 108 JS call sites, renames the corresponding pub use re-exports in dispatch_js2native.rs, moves getUseSystemCA into cli/Arguments.rs, removes the crate::{shell,ffi,socket}::{shell,ffi,socket} alias modules, and adds test/internal/js2native-codegen.test.ts.
Since my last two inline comments, commits 4d13092 and 38a69d9 removed the re-committed Generated{Bindings,JS2Native}.zig artifacts (kept in .gitignore) and fixed the stale ZigGeneratedClasses.zig comments in generate-classes.ts and class-definitions.ts. The bug-hunting pass on this revision found nothing.
Security risks
None identified. The change is build-time codegen and symbol naming; no auth, crypto, network input parsing, or permission surface is touched. The closest thing to a runtime move is get_use_system_ca, which simply reads an existing atomic flag.
Level of scrutiny
High. The contract being changed is the link-time symbol set shared between GeneratedJS2Native.h (C++ externs) and generated_js2native.rs (Rust #[no_mangle] thunks), plus the ninja output graph in scripts/build/codegen.ts. A mismatch here breaks every platform's link step, and the PR has already had two rounds of accidentally re-committed stale generated artifacts. The author's verification (full bun bd, cargo check, per-binding smoke tests, suite parity vs released bun) is thorough, and the new contract test pins the macro lowering and symbol shape — but the breadth (61 files, ~9 re-keyed bindings whose crate:: paths changed, removal of three alias modules) and the explicit overlap with open PRs #30683/#30698 make this a maintainer call rather than a bot-approval candidate.
Other factors
There is one design choice a human should ratify: $native vs the $rust naming used in #30683. The class-definitions.ts lang default also flipped from "zig" to "rust" in the doc comment — that matches the existing (a.lang ?? "rust") filter in generate-classes.ts, so it's a documentation correction rather than a behavior change, but worth a glance. CI on the latest commit (build #63097) was still running at the time of the robobun status update.
|
CI update for build #63230 (6f16c0b, rebased + all review feedback applied): All 16
None of these touch the codegen/FFI-symbol rename; a regression here would fail build-rust or shift binary size. The re-roll was already spent on 77e4 before the rebase; not pushing another. Ready for a maintainer. Earlier build #63097 (38a69d9, pre-rebase)285/287 passed; failures were |
The JS to native binding macros were still named for the Zig implementation and keyed by .zig filenames that generate-js2native.ts required to exist on disk, making the .zig porting references load-bearing for the build. - $zig -> $native, $newZigFunction -> $newNativeFunction, all call sites updated to .rs keys (bare basename, or src/-relative path when ambiguous; ambiguity is now a codegen error instead of first match) - mangled symbols renamed JS2Zig__* -> JS2Native__*, derived from the .rs path; hand-exported DNS symbols updated to match - nine keys with no .rs sibling re-keyed to the implementing file; dispatch_js2native.rs aliases renamed, codegen-only alias modules (shell::shell, ffi::ffi, socket::socket) deleted, getUseSystemCA moved next to the flag it reads in cli/Arguments.rs - dead Zig emitter getJS2NativeZig and its GeneratedJS2Native.zig output removed Fixes #32210
…ct tests The build still declared and wrote several .zig artifacts that nothing consumes since the Rust port: GeneratedBindings.zig (bindgen), ErrorCode.zig, ZigGeneratedClasses.zig, cpp.zig (cppbind), ci_info.zig, and bindgenv2's per-namespace .zig outputs. Remove the write sites and the ninja edges; scripts/build/codegen.ts no longer references Zig. ci_info.ts generated only ci_info.zig (the live implementation is src/runtime/cli/ci_info.rs), so the generator and its edge are deleted. The sources.zigGeneratedClasses glob key is renamed to classesTs. test/internal/js2native-codegen.test.ts drives the macro lowering and key resolution directly: $native()/$newNativeFunction() lower to lazy intrinsics, keys must name a real unambiguous .rs file, .zig keys are rejected, and the generated C++/Rust/d.ts agree on JS2Native__ symbols.
The CI-gated success log still referenced resultFilePath after the cpp.zig write was removed, throwing ReferenceError and failing the cppbind codegen edge on every CI lane. Point it at cpp.rs instead.
The deleted src/codegen/ci_info.ts was still named as the sync source in src/runtime/cli/ci_info.rs and mod.rs. The hand-maintained table is canonical now; point readers at watson/ci-info vendors.json instead.
GeneratedBindings.zig and GeneratedJS2Native.zig were pre-rename build outputs left on disk by a build of main; they rode along in 31df412. Nothing compiles them and this branch no longer generates either file.
… set - getJS2NativeDTS: the NativeFilenameRust union now excludes bare basenames that resolveNativeFileId would reject as ambiguous (the src/-relative form for each file is always included). The test covers this with "lib.rs" which exists in ~200 crates. - emitJsModules: write the src/**/*.rs path set at configure time and list it as an implicit input to the bundle-modules edge, so adding/removing/renaming an .rs file invalidates the $native() key resolution without depending on every Rust file's content.
22bbfb1 accidentally re-staged GeneratedBindings.zig and GeneratedJS2Native.zig after a build of main regenerated them on disk. Restore the two .gitignore entries so stale build outputs from older checkouts cannot ride along in future commits; the generators for both files are removed on this branch, so nothing will produce them going forward.
…mission The ZigGeneratedClasses.zig write was removed in de52544 but the heading comment on the Rust emitter block and the ClassDefinition.lang doc comment still described it as a live output. Both now describe the actual emit path (generated_classes.rs satisfies the C++ externs; lang: "zig" classes are skipped). The @default on lang is also corrected to "rust" to match the only read site.
robobun
force-pushed
the
farm/1c84b455/rename-zig-macro-to-native
branch
from
38a69d9 to
79ce1c1
Compare
With ErrorCode.zig removed from outputs, the remaining two outputs are C++ headers that the Rust build does not include! or embed, so pushing them into rustInputs only adds spurious implicit deps on the cargo edge. emitBindgen/emitBindgenV2/emitCppBind already had the analogous push removed or narrowed when their .zig outputs went away; this is the one emitter where the line was missed.
2 participants
Fixes #32210
Problem
The
$zig("file.zig", "symbol")/$newZigFunction(...)macros insrc/js/were named for the old Zig implementation and keyed by.zigfilenames.generate-js2native.tsresolved those keys by scanningsrc/for.zigfiles and threwCould not find file <name>.zig in $zig callwhen one was missing, so the otherwise inert.zigporting references were load-bearing for the build.What changed
$zigis now$native,$newZigFunctionis now$newNativeFunction(replacements.ts,generate-js2native.ts,private.d.ts, all 108 call sites insrc/js/).resolveNativeFileIdscans.rsfiles instead of.zig. A key is the implementing.rsfile: bare basename, or a path relative tosrc/when the basename is ambiguous. An ambiguous basename is now a codegen error listing the candidates instead of a silent first match.JS2Zig___src_sql_jsc_mysql_zig__createBindingtoJS2Native___src_sql_jsc_mysql_createBinding. The C++ externs (GeneratedJS2Native.h) and the Rust#[no_mangle]thunks (generated_js2native.rs) both come from the same generator so they cannot drift; the two hand-exported DNS symbols indns.rsand thehandExportedskip list were updated to the new names.crate::thunk targets identical. Nine files had no.rssibling (bun.zig,crash_handler.zig,escapeRegExp.zig,ffi.zig,ini.zig,patch.zig,shell.zig,sys.zig,runtime/socket/socket.zig) and were re-keyed to the real implementing file. The matchingdispatch_js2native.rsaliases were renamed, and the codegen-only alias modules (crate::shell::shell,crate::ffi::ffi,crate::socket::socket) are deleted now that the generator derives the real module paths.getUseSystemCAmoved from the dispatch landing pad intocli/Arguments.rs, next to the flag it reads.getJS2NativeZig()and itssrc/jsc/bindings/GeneratedJS2Native.zigoutput (bundle-modules.ts,scripts/build/codegen.ts,.gitignore). Nothing has consumed it since the Rust port..zigreferences are not read by the build or codegen.Verification
.zigfiles deleted: movedmysql.zigandnode_net_binding.zigout of the tree and re-ranbundle-modules.tscleanly (previously threwCould not find file mysql.zig in $zig call). The missing-file, wrong-extension, and ambiguous-basename guards all still throw.generated_js2native.rsagainst the previous output shows only the nine intended re-keys change their thunk targets; everything else is byte-identical modulo symbol names. All 102JS2Native__*externs inGeneratedJS2Native.hmatch the Rust exports one to one, and zeroJS2Zigreferences remain.cargo check -p bun_runtimeclean, fullbun bdbuild, plus smoke scripts invoking every re-keyed binding and representative swapped ones (ini, patch, shell lex, escapeRegExp, sys error tables, crash_handler, createSocketPair, tls getUseSystemCA, dns Resolver, ffi cc, zlib, os, net BlockList, assert myers diff, sourcemap VLQ, npa, hostedGitInfo, counters, bindgen, Stat, unicode, timer, event loop stats) with output identical to the released bun.No runtime behavior change: symbol names and thunk shapes are regenerated on both sides of the FFI boundary by the same script, and wrapper function display names derive from the unchanged symbol argument. There is no new test because the contract is enforced structurally: a bad key is a codegen error and a bad target is a
cargo checkerror, both of which fail the build itself.Review follow-up (second commit)
Per review,
scripts/build/codegen.tsno longer references Zig at all: the remaining dead.zigcodegen outputs are gone.GeneratedBindings.zig(bindgen),ErrorCode.zig,ZigGeneratedClasses.zig,cpp.zig(cppbind), and bindgenv2's per-namespace.zigfiles lose their write sites and ninja edges;zigFilesGeneratedIntoSrcis deleted.src/codegen/ci_info.tsgenerated onlyci_info.zig(the live implementation issrc/runtime/cli/ci_info.rs), so that generator and its edge are deleted outright. The generators' internal zig-emitter code that still exists (for examplegenerateZigin generate-classes.ts) is intentionally left for the wholesale.zigremoval in #30683. Thesources.zigGeneratedClassesglob key is renamed toclassesTs.Tests
test/internal/js2native-codegen.test.tsdrives the macro lowering and key resolution directly (same pattern astest/internal/macos-cross-config.test.ts):$native()/$newNativeFunction()lower to lazy intrinsics, keys must name a real and unambiguous.rsfile,.zigkeys are rejected, and the generated C++ externs, Rust thunks, and d.ts agree on theJS2Native__*symbols andcrate::targets. All 8 tests fail against the pre-renamesrc/codegen(verified by checking out the merge-basesrc/and re-running) and pass with this branch.Rebase onto 1ee0960 (after #31826)
src/js/node/_http_client.ts: node:stream: sync the stream suite to Node v26.3.0 and fix the gaps it surfaces — stream/iter + zlib/iter behind --experimental-stream-iter, read()-one-buffer semantics, compose/pipeline/web-adapter/http/fs/net parity (247/251 upstream tests, +63 vendored) #31826 rewrote this as a full port of Node'slib/_http_client.jsonnode:net/node:tls+ llhttp, removing thenodeHttpClientfetch binding entirely. Took main's version; net diff is zero.src/js/internal/streams/iter/transform.ts(new in node:stream: sync the stream suite to Node v26.3.0 and fix the gaps it surfaces — stream/iter + zlib/iter behind --experimental-stream-iter, read()-one-buffer semantics, compose/pipeline/web-adapter/http/fs/net parity (247/251 upstream tests, +63 vendored) #31826): three$zig("node_zlib_binding.zig", ...)calls renamed to$native("node_zlib_binding.rs", ...), matchingsrc/js/node/zlib.ts. Verifiedtest-stream-iter-transform-roundtrip.jspasses under--experimental-stream-iter.