Resolve the audited TODO(port)/TODO(b2)/PORT NOTE backlog#31783
Merged
Conversation
Drain the Rust port's marker backlog: every audited TODO(port)/TODO(b2-*) marker and PORT NOTE comment was individually resolved - fixed, deleted as stale, untagged to a plain comment, or split into a tracked follow-up work order. ~495 real code fixes, each citing the Zig sibling for intended semantics; the 101 markers that remain are claimed by written work orders for changes too risky to land in a bulk branch (GC rooting, arena lifetime threading, cross-thread ownership restructures). Verified: full debug build + smoke tests on linux-x64, cross-target cargo check on windows/macos/linux x64/aarch64.
Collaborator
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Contributor
WalkthroughThis PR updates analytics feature plumbing, AST and bun_core runtime helpers, bundler task and output paths, CSS parser and serializer behavior, several FFI/platform mappings, and related tests and documentation comments. ChangesRuntime and parser port updates
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/bundler/ThreadPool.rs (1)
150-166:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncrement
REF_COUNTin the post-lock initialized path.If another thread initializes the IO pool before this thread acquires
MUTEX, this branch returns a live pool without counting the new owner. That letsshutdown()observeREF_COUNT == 1while aThreadPoolstill holdsio_pool, so the static pool can be dropped while it is still in use.💡 Proposed fix
if REF_COUNT.load(Ordering::Relaxed) == 0 { // SAFETY: we hold MUTEX and REF_COUNT == 0, so no other thread is reading THREAD_POOL. unsafe { (*THREAD_POOL.get()).write(ThreadPoolLib::ThreadPool::init( ThreadPoolLib::Config { @@ } // 2 means initialized and referenced by one `ThreadPool`. REF_COUNT.store(2, Ordering::Release); } else { - // Zig fell through to `return &thread_pool` without - // bumping the ref count here, which is a latent bug in the source - // (the racing acquirer's reference isn't counted). Mirrored. + // Another thread initialized the pool while we were waiting on the mutex. + REF_COUNT.fetch_add(1, Ordering::Relaxed); }🤖 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/bundler/ThreadPool.rs` around lines 150 - 166, The else branch that returns the already-initialized static pool must increment the global reference counter so new owners are counted; update the branch that runs when REF_COUNT.load(...) != 0 to atomically increment REF_COUNT (e.g., fetch_add(1, ...) with appropriate ordering) while still holding MUTEX (or immediately after verifying initialization) before returning the THREAD_POOL pointer created via ThreadPoolLib::ThreadPool::init, so shutdown() will see the correct live owner count for REF_COUNT and the static THREAD_POOL won't be dropped while still in use.src/css/properties/grid.rs (2)
525-557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
.null-cell tokens instead of rejecting them.The
.branch still falls through into the name-codepoint path, so any row containing an unnamed area fails to parse. That makes spec-valid values like"header . sidebar"invalid.Suggested fix
column += 1; if strings::starts_with_char(rest, b'.') { - // TODO(port): the Zig original falls through here without `continue` — - // likely a bug (per upstream lightningcss, a `.` null-cell token should - // push None and continue; as written, any string containing `.` fails - // the name-codepoint check below and the whole value fails to parse). - // Mirroring the observable Zig control flow for now (Zig dead-advances - // `string = rest[idx..]`; this block is empty — same outcome). + let dot_len = rest.iter().take_while(|&&c| c == b'.').count(); + tokens.append(None); + string = &rest[dot_len..]; + continue; } let starts_with_name_codepoint = 'brk: {🤖 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/css/properties/grid.rs` around lines 525 - 557, When strings::starts_with_char(rest, b'.') is true, treat it as a null-cell: append a None entry to tokens, advance string past the '.' (e.g., string = &rest[1..]) and continue the parsing loop instead of falling through into the name-codepoint logic; this fixes handling of unnamed areas (update the block around strings::starts_with_char(rest, b'.') to tokens.append(None), advance string, and continue while preserving bump usage where needed).
470-500:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse all
grid-template-areasrows, not just the first one.This only attempts
expect_string()once, so multi-row declarations leave subsequent rows unparsed, and non-noneinputs with no leading string can fall through toAreas { columns: 0, areas: [] }instead of erroring. This breaks validgrid-template-areasvalues with more than one row.Suggested fix
- if let Ok(s) = input.try_parse(|i| i.expect_string().map(std::ptr::from_ref::<[u8]>)) { + while let Ok(s) = input.try_parse(|i| i.expect_string().map(std::ptr::from_ref::<[u8]>)) { // SAFETY: `s` points to a slice returned by `expect_string`, which is backed by the // parser's input arena and remains valid for the duration of this parse. let s = unsafe { crate::arena_str(s) }; let parsed_columns = match Self::parse_string(input.arena(), s, &mut tokens) { Ok(v) => v, @@ row += 1; // The final `row += 1` is dead on the last iteration but read on // the next; `unused_assignments` can't see that. Touch it. let _ = row; } + + if row == 0 { + return Err(input.new_custom_error(css::ParserError::invalid_declaration)); + } Ok(GridTemplateAreas::Areas { columns, areas: tokens, })🤖 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/css/properties/grid.rs` around lines 470 - 500, The code only calls input.try_parse(|i| i.expect_string()) once, so subsequent grid-template-areas rows are ignored; change the single if let Ok(s) = ... block into a loop (while let Ok(s) = input.try_parse(|i| i.expect_string().map(std::ptr::from_ref::<[u8]>))) and repeat the parsing/column-consistency logic for each row using the existing parse_string, row, columns, and tokens variables, keeping the unsafe crate::arena_str conversion inside the loop; after the loop, if row == 0 handle the non-string case by returning an appropriate parse error (unless the token is the keyword `none`), so inputs with no leading string no longer silently fall through to Areas { columns: 0, areas: [] } but instead error correctly.src/css/values/color.rs (1)
2397-2403:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept the standards-compliant
a98-rgbcolorspace name here.This dispatch only matches
a99-rgb, but the earlierfromhandling in the same function family already recognizesa98-rgb. As written,color(a98-rgb …)falls into the unexpected-token path even though the file otherwise modelsA98.Suggested fix
- b"a99-rgb" => PredefinedColor::A98(A98 { r: a, g: b, b: c, alpha }), + b"a98-rgb" | b"a99-rgb" => PredefinedColor::A98(A98 { r: a, g: b, b: c, alpha }),🤖 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/css/values/color.rs` around lines 2397 - 2403, The match arm in the colorspace dispatch uses the incorrect literal b"a99-rgb", causing valid "a98-rgb" inputs to be rejected; update the match in the crate::match_ignore_ascii_case! block (the arm that constructs PredefinedColor::A98 via A98 { r: a, g: b, b: c, alpha }) to match b"a98-rgb" instead so it aligns with the earlier from handling and the A98 type/PredefinedColor::A98 variant.src/css/media_query.rs (1)
1054-1060:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize equality media ranges with
=.
MediaFeatureComparison::Equalcurrently emits-, so a parsed equality range round-trips as invalid CSS instead of(feature = value). If the Zig side still has this behavior, this is the spot to intentionally diverge rather than preserve the bug.Suggested fix
match self { - MediaFeatureComparison::Equal => dest.delim(b'-', true), + MediaFeatureComparison::Equal => dest.delim(b'=', true), MediaFeatureComparison::GreaterThan => dest.delim(b'>', 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/css/media_query.rs` around lines 1054 - 1060, The serializer is emitting '-' for equality which breaks round-trip; in MediaFeatureComparison::Equal inside to_css replace the call that writes '-' with one that writes '=' (i.e. change the delimiter passed to dest.delim from b'-' to b'=') so equality media ranges serialize as "(feature = value)"; keep the same call site (to_css -> dest.delim) and ensure surrounding behavior (the true flag) remains unchanged.src/css/properties/font.rs (1)
748-755:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
fontshorthand still can't parsevariant-capsin the pre-size slot.
variant_capsis initialized toNone, so gatingFontVariantCaps::parse_css2behindvariant_caps.is_some()makes this branch unreachable until after a value is already set. That drops valid shorthands likeitalic small-caps 16px serif. This needs to stayis_none()even if Zig had the inverted condition.🐛 Suggested fix
- if variant_caps.is_some() { + if variant_caps.is_none() { // Zig has `if (variant_caps != null)` here — preserved verbatim (likely upstream bug; should be `== null`) if let Ok(value) = input.try_parse(FontVariantCaps::parse_css2) { variant_caps = Some(value); count += 1; continue;🤖 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/css/properties/font.rs` around lines 748 - 755, The branch that attempts to parse FontVariantCaps::parse_css2 is incorrectly guarded by variant_caps.is_some(), making it unreachable because variant_caps starts as None; change the condition to variant_caps.is_none() so the if let Ok(value) = input.try_parse(FontVariantCaps::parse_css2) branch can run when no value is yet set, assign variant_caps = Some(value), increment count, and continue (the code around the variant_caps variable and the input.try_parse call should be the target).src/collections/linear_fifo.rs (1)
119-145:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDocumented UB is still reachable here.
StaticBufferstill reinterprets the entire[MaybeUninit<T>; N]as&[T]/&mut [T], so any path that exposes unwritten slots creates references to uninitializedT. The new comment correctly calls out that in-tree FIFO element types already violate the “any bit pattern is valid” assumption, which makes this a real soundness bug, not just deferred cleanup. Either restrict these buffers to byte/MaybeUninit-safe element types or change the accessor surface to operate onMaybeUninit<T>for unwritten regions before merge.🤖 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/collections/linear_fifo.rs` around lines 119 - 145, StaticBuffer currently reinterprets the whole [MaybeUninit<T>; N] as &[T] / &mut [T] (see as_slice, as_mut_slice using assume_init_slice/_mut) which creates references to uninitialized T; change the accessor surface to expose &[MaybeUninit<T>] and &mut [MaybeUninit<T>] instead and update the LinearFifoBuffer trait signatures accordingly so callers must assume-init only the logically-written subranges (use assume_init_slice/_mut only on those subranges), or alternatively add a strict trait bound restricting T to a POD/any-bit-pattern-safe marker if you prefer to keep returning &[T]; pick one approach and apply it consistently to StaticBuffer::as_slice, StaticBuffer::as_mut_slice and the LinearFifoBuffer trait so no path casts the full buffer to &[T] over unwritten slots.src/bundler/options.rs (1)
382-400:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the missing stdin name for
Loader::Jsonc.Line 383 makes omissions silent, and this initializer never assigns
Loader::Jsonc. That means JSONC stdin now gets an empty pseudo-filename instead of an extension-bearing one, which can degrade diagnostics and any downstream extension-based behavior.Suggested fix
map[Loader::File] = b"input"; map[Loader::Json] = b"input.json"; + map[Loader::Jsonc] = b"input.jsonc"; map[Loader::Toml] = b"input.toml"; map[Loader::Yaml] = b"input.yaml";🤖 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/bundler/options.rs` around lines 382 - 400, stdin_name_map currently never assigns a pseudo-filename for Loader::Jsonc, leaving its entry as an empty slice; update the stdin_name_map initializer (function stdin_name_map) to set map[Loader::Jsonc] = b"input.jsonc" so JSONC stdin receives an extension-bearing name for diagnostics and extension-based logic.src/ast/lib.rs (1)
3443-3452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
DebugOnlyDisablerScopethread-affine (!Send)
DebugOnlyDisablerScope<T>is currently justPhantomData<T>, so it remains auto-SendwhenT: Send. OnDropit callsDebugOnlyDisabler::<T>::enable(), which mutatesdebug_disabler_state::DISABLEDstored instd::thread_local!. Dropping the guard on another thread therefore operates on the wrong TLS slot, hitting theexpect("DebugOnlyDisabler::enable without matching disable")panic and/or leaving the originating thread’s disable count unbalanced.Suggested fix
- DebugOnlyDisablerScope(core::marker::PhantomData) + DebugOnlyDisablerScope(core::marker::PhantomData, core::marker::PhantomData) ... -pub struct DebugOnlyDisablerScope<T: 'static>(core::marker::PhantomData<T>); +pub struct DebugOnlyDisablerScope<T: 'static>( + core::marker::PhantomData<T>, + core::marker::PhantomData<std::rc::Rc<()>>, +);🤖 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/ast/lib.rs` around lines 3443 - 3452, The guard is currently auto-Send because it only holds PhantomData<T>, so dropping it on a different thread can access the wrong thread-local state; change DebugOnlyDisablerScope<T>(core::marker::PhantomData<T>) to carry a PhantomData of a non-Send marker (for example core::marker::PhantomData<std::rc::Rc<()>>) so the type becomes !Send, update the constructor DebugOnlyDisabler::scope to create the new PhantomData variant, and keep the Drop impl that calls DebugOnlyDisabler::enable() unchanged.src/css/selectors/parser.rs (1)
3725-3747:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLowercase the full attribute local name for HTML case-sensitivity detection (not only the suffix after the first uppercase).
When
local_namecontains an uppercase byte,local_name_loweris built from&local_name[first_uppercase..](only the suffix), then passed intoattribute_flags.to_case_sensitivity(local_name_lower.v(), ...). ForAttributeFlags::CaseSensitivityDependsOnName,to_case_sensitivity()checks HTML case-insensitive attribute names likedir/relviais_html_case_insensitive_attribute(local_name), so selectors such as[dIr](first uppercase not at offset 0) can be misclassified as case-sensitive.🐛 Proposed fix
- if let Some(first_uppercase) = first_uppercase { - let str_ = &local_name[first_uppercase..]; - // PERF(port): was arena alloc — profile if hot (see `arena_lowercase`). - let lowered: *const [u8] = arena_lowercase(input.arena(), str_); + if first_uppercase.is_some() { + // PERF(port): was arena alloc — profile if hot (see `arena_lowercase`). + let lowered: *const [u8] = arena_lowercase(input.arena(), local_name); break 'brk (Ident { v: lowered }, false); } else {🤖 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/css/selectors/parser.rs` around lines 3725 - 3747, The current code only lowercases the suffix after the first uppercase, causing misclassification for names like "dIr"; change the logic around local_name_lower/local_name_is_ascii_lowercase so that when any uppercase is found you call arena_lowercase(input.arena(), local_name) to produce a fully-lowercased Ident (use Ident { v: lowered }) and set local_name_is_ascii_lowercase = false; keep the existing branch for the no-uppercase case that returns Ident { v: std::ptr::from_ref::<[u8]>(local_name) } and true; ensure this updated local_name_lower is what gets passed into attribute_flags.to_case_sensitivity(...) so is_html_case_insensitive_attribute sees the full lowercased name.src/css/css_parser.rs (3)
5663-5676:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
split_source_map()keeps the delimiter and drops EOF-terminated URLs.When the first delimiter is found,
&contents[..i + 1]includes that delimiter byte in the stored URL. And if the directive payload runs to EOF, the function returnsNoneinstead of the whole URL. So/*# sourceMappingURL=app.css.map*/is ignored entirely, while/*# sourceMappingURL=app.css.map */stores a trailing space/newline.Suggested fix
for (i, &c) in contents.iter().enumerate() { match c { b' ' | b'\t' | FORM_FEED_BYTE | b'\r' | b'\n' => { - return Some(&contents[0..i + 1]); + return Some(&contents[0..i]); } _ => {} } } - None + Some(contents) }🤖 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/css/css_parser.rs` around lines 5663 - 5676, split_source_map currently returns &contents[..i+1] (including the delimiter) and returns None when no delimiter is found, which drops EOF-terminated URLs; change the behavior so when a delimiter byte is matched in split_source_map (matching bytes in contents via the for (i, &c) loop) you return the slice excluding the delimiter (e.g., &contents[0..i]) and if the loop completes with no delimiter found return Some(contents) so EOF-terminated URLs are preserved.
2015-2047:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the layer context on the error path.
parse_style_block(input)?can return afteron_layer_rule(),push_to_enclosing_layer(), orbump_anon_layer_count(1)have already mutated parser state. In recovery mode, later rules then inherit the wrong enclosing layer / anonymous-layer depth.Suggested fix
T::on_layer_rule(this.at_rule_parser, &layer); let old_len = T::enclosing_layer_length(this.at_rule_parser); - if name.is_some() { + let is_anon = name.is_none(); + if name.is_some() { // Drain the sole element by value — avoids a second clone. T::push_to_enclosing_layer(this.at_rule_parser, layer.swap_remove(0)); } else { T::bump_anon_layer_count(this.at_rule_parser, 1); } + scopeguard::defer! { + if is_anon { + T::bump_anon_layer_count(this.at_rule_parser, -1); + } + T::reset_enclosing_layer(this.at_rule_parser, old_len); + } let rules = this.parse_style_block(input)?; - if name.is_none() { - T::bump_anon_layer_count(this.at_rule_parser, -1); - } - T::reset_enclosing_layer(this.at_rule_parser, old_len); - this.rulesAs per coding guidelines, "Prefer RAII (
Drop) over manual cleanup in Rust code".🤖 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/css/css_parser.rs` around lines 2015 - 2047, The AtRule Prelude Layer branch mutates parser state (calls T::on_layer_rule, possibly T::push_to_enclosing_layer or T::bump_anon_layer_count) before calling parse_style_block(input) and does not restore state if parse_style_block returns an error; create an RAII guard (e.g., LayerContextGuard) inside the AtRulePrelude::Layer handling that captures old_len, whether a name was pushed or anon count bumped, and on Drop calls T::reset_enclosing_layer(this.at_rule_parser, old_len) and undoes the anon bump or removed push as appropriate, and ensure the guard is dismissed/disabled only after parse_style_block succeeds so the cleanup runs on error paths (refer to symbols: parse_style_block, T::on_layer_rule, T::push_to_enclosing_layer, T::bump_anon_layer_count, T::reset_enclosing_layer, T::enclosing_layer_length, the local name variable).
3015-3084:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis only plucks the first leading
@import.Both passes break on the same iteration that flips
saw_importstotrue, so a stylesheet with multiple top-level imports only counts and rewrites the first one. The remaining imports stay inself.ruleswith stale import-record indexes.Suggested fix
{ let mut saw_imports = false; for rule in self.rules.v.iter() { match rule { CssRule::LayerBlock(_) => {} CssRule::Import(_) => { - if !saw_imports { - saw_imports = true; - } + saw_imports = true; count += 1; } CssRule::Unknown(u) => { if u.name == b"tailwind" { continue; } } - _ => {} - } - if saw_imports { - break; + _ if saw_imports => break, + _ => {} } } } @@ let mut saw_imports = false; for rule in self.rules.v.iter_mut() { match rule { // TODO: layer, might have imports CssRule::LayerBlock(_) => {} CssRule::Import(import_rule) => { - if !saw_imports { - saw_imports = true; - } + saw_imports = true; let import_record_idx = u32::try_from(new_import_records.len()).unwrap(); import_rule.import_record_idx = import_record_idx; new_import_records.push(ImportRecord { @@ let old = core::mem::replace(rule, CssRule::Ignored); // PERF(port): was appendAssumeCapacity out.v.push(old); } CssRule::Unknown(u) => { if u.name == b"tailwind" { continue; } } - _ => {} - } - if saw_imports { - break; + _ if saw_imports => break, + _ => {} } }🤖 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/css/css_parser.rs` around lines 3015 - 3084, The two loops that scan self.rules.v set saw_imports on the first CssRule::Import and then unconditionally break at the end of that iteration, so only the first leading `@import` is counted/rewritten; update both loops (the initial counting loop and the rewrite loop that assigns import_record_idx and pushes to out.v) to not break immediately when saw_imports becomes true but instead continue processing subsequent rules and only break when you encounter the first non-import rule after you have already seen imports (i.e., break only when saw_imports == true and the current rule is neither CssRule::Import nor an allowed-leading rule like CssRule::LayerBlock or CssRule::Unknown with name == b"tailwind"); ensure new_import_records.len() and import_rule.import_record_idx logic runs for each Import encountered so import_record_idx values remain correct for all leading imports.src/bundler/transpiler.rs (1)
3042-3054: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMacroRemap insertion isn’t a fallible
Resultdrop:m.insert(k, inner)isStringArrayHashMap::insert(&mut self, ..., ...) -> Option<V>(notResult), and its backingpush_entrypath doesn’t returnAllocError, so there’s noResulthere to silently drop (seesrc/collections/array_hash_map.rs).
Optional: if you wantAllocError-to-bunerror conversion symmetry withput(StringArrayHashMap::put(...) -> Result<(), AllocError>), switch this tom.put(k, inner)?/bun_core::handle_oom(m.put(k, inner))instead ofinsert.🤖 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/bundler/transpiler.rs` around lines 3042 - 3054, The current code uses MacroRemap::default() and calls m.insert(k, inner) where StringArrayHashMap::insert returns Option<V> and is not fallible; replace the insert call with the fallible put API to preserve AllocError->bun error handling: call m.put(k, inner)? (or wrap with bun_core::handle_oom(m.put(k, inner))) when iterating self.options.macro_remap so failures propagate; keep the surrounding inner = v.clone().map_err(|_| bun_core::err!("OutOfMemory"))? logic and otherwise return m as before.
🤖 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/analytics/lib.rs`:
- Around line 123-126: The macro arm matching "(`@storage` $(#[$doc:meta])*
$ident:ident, $core:ident)" currently applies #[allow(non_upper_case_globals)]
to the "pub use ::bun_core::Global::features::$core as $ident;" line which is
ineffective and triggers Clippy; remove the #[allow(non_upper_case_globals)]
attribute from the pub use arm and ensure that the attribute remains only on the
actual "pub static $ident: AtomicUsize" arm so the allow applies to the real
static/const declaration.
In `@src/ast/lib.rs`:
- Around line 1473-1489: The identifier scanner currently only handles a leading
backslash escape after the first iter.next(), so an identifier starting with a
"\u{...}" is truncated; update the initial branch where you check "if
bun_core::identifier::is_identifier_start(cursor.c) || cursor.c == '\\' as i32"
to, when cursor.c == '\\' (the initial cursor), perform the same
backslash-unicode-skip logic used inside the while (the checks on text[cursor.i
+ 1] == b'u' and text[cursor.i + 2] == b'{' and advancing cursor.i until the
closing '}'), so the starting cursor is advanced past a leading "\u{...}" before
entering the main iter.next() loop in the identifier scan (use the same
cursor/text bounds checks as the inner block to avoid out-of-bounds).
In `@src/ast/new_store.rs`:
- Around line 21-27: The sealing module is currently public which allows
downstream crates to implement sealed::Sealed and break the invariants; change
the declaration from `pub mod sealed` to a non-public module (e.g. `mod sealed`
or `pub(crate) mod sealed`) so only this crate can implement `sealed::Sealed`,
leaving the `pub trait StoredIn<S>: sealed::Sealed<S> {}` line unchanged and
ensure any `new_store!` macro references remain using
`$crate::new_store::sealed::Sealed` so the macro still points to the crate-local
sealed trait.
In `@src/brotli/lib.rs`:
- Around line 442-455: The flush() implementation on BrotliWriter currently
calls self.compressor.write(b"", false) which uses the normal flush_op; change
it to invoke the compressor's full flush operation so the encoder emits any
pending bytes before returning — e.g. call the compressor with full_flush_op
(replace self.compressor.write(b"", false) with a call that uses full_flush_op,
e.g. self.compressor.write(b"", self.compressor.full_flush_op() or
full_flush_op()) and then keep writing the returned out to self.input_writer and
flushing as before in BrotliWriter::flush.
In `@src/bun_core/output.rs`:
- Around line 1517-1518: The issue is that removing per-scope buffers means
self.lock only serializes access to a single ScopedLogger while all scopes call
into the shared SCOPED_FILE_WRITER, allowing concurrent write_fmt/write_str
sequences to interleave; fix by introducing a shared sink lock around the shared
writer so write_fmt and flush are performed atomically (e.g., add a global Mutex
or RwLock used by SCOPED_FILE_WRITER and acquire it in
ScopedLogger::write_fmt/flush before calling into the writer), or alternatively
restore a per-scope buffer in ScopedLogger and emit a single write to
SCOPED_FILE_WRITER, ensuring no partial write_str calls are issued to the shared
sink.
In `@src/bun_core/util.rs`:
- Around line 4914-4917: spawn_sync_inherit_impl currently indexes argv[0]
without checking for an empty slice which can panic; guard against empty argv at
the start of spawn_sync_inherit_impl and return a proper Err(crate::Error) (the
same failure path/semantics used by the Windows implementation) instead of
indexing into argv[0] or aborting, updating any downstream uses of argv[0]
within the function to assume non-empty after that check; reference
spawn_sync_inherit_impl, SpawnStatus and crate::Error when making this change.
In `@src/bundler/linker.rs`:
- Around line 377-395: The code currently always calls
dupe(modkey.hash_name(...)) which interns a fresh process-lifetime string on
every lookup (via dupe) while IS_CACHE_ENABLED is hard-disabled, causing
unbounded memory growth; instead, use the hashed_filenames cache before
interning and only call dupe when inserting a new entry: compute the key with
bun_wyhash::hash(file_path.text), check self.hashed_filenames for an existing
entry and return it if present, otherwise call modkey.hash_name into
hash_name_buf, dupe the result and insert it into self.hashed_filenames; also
guard against hash collisions by verifying the string equality before returning
an entry.
In `@src/clap/args.rs`:
- Around line 339-344: Rename the parameter `allocations` in the test helper
`test_shell_iterator_ok` to reflect that it measures owned-result count (e.g.,
`expected_owned_results` or `expected_owned_count`), update all uses inside the
function (`owned_results` comparisons/assertions) and any call sites, and adjust
the leading comment to say "owned-result count" / "borrowed (zero-copy) fast
path" instead of "allocations"; apply the same renaming and comment tweak to the
other matching test helper instance referenced in the diff.
In `@src/collections/linear_fifo.rs`:
- Around line 805-817: The Write impl for LinearFifo<u8, B> should not map
capacity-exhaustion on fixed buffers to OutOfMemory; change the body of
std::io::Write::write (the impl for LinearFifo::write) to first compute the
available writable_length() and, for non-dynamic buffers (when B::DYNAMIC is
false), write at most that many bytes and return Ok(n) (return Ok(0) if full)
instead of mapping an AllocError to ErrorKind::OutOfMemory; only treat
AllocError as true OOM when B::DYNAMIC is true (i.e., an actual allocation was
attempted by ensure_total_capacity), preserving the existing mapping to
std::io::ErrorKind::OutOfMemory in that dynamic case.
In `@src/crash_handler/lib.rs`:
- Around line 946-948: The branch currently gates printing the unsupported-libuv
banner on the global bun_analytics::features::unsupported_uv_function counter
but the payload is stored in the thread-local UNSUPPORTED_UV_FUNCTION, causing
cross-thread leakage; change the gating so the banner is printed only when the
current thread's UNSUPPORTED_UV_FUNCTION TLS slot is set (e.g., check
UNSUPPORTED_UV_FUNCTION.with(|v| v.is_some() or peek the TLS value) before
emitting the banner) and keep bun_analytics::features::unsupported_uv_function
as a process-global analytics counter only; apply the same TLS-based gatefix to
the other similar branches that reference
bun_analytics::features::unsupported_uv_function and UNSUPPORTED_UV_FUNCTION
(including the branches associated with CrashHandler__unsupportedUVFunction()).
In `@src/css/css_parser.rs`:
- Around line 3091-3106: contains_tailwind_directives currently checks for
CssRule::Custom(_) which is never produced because
BundlerAtRuleParser::parse_prelude maps unknown bundler directives to
AtRulePrelude::Unknown; update the function
(StyleSheet<BundlerAtRule>::contains_tailwind_directives) to detect Tailwind
at-rules by matching CssRule::AtRule(at_rule) and checking at_rule.prelude ==
AtRulePrelude::Unknown (and at_rule.name == "tailwind" or matches the expected
tailwind directive name(s)), returning true when found; keep the existing
handling for CssRule::LayerBlock and CssRule::Import but replace the unreachable
Custom arm with the new AtRule check.
In `@src/css/properties/border.rs`:
- Around line 1628-1635: The macro arm for ($id:ident) currently builds an
UnparsedProperty into the local upppppppppp and records the flushed property but
never enqueues it to dest, dropping rewritten border-block-* declarations; fix
by appending the rewritten unparsed property to dest after calling
context.add_unparsed_fallbacks (i.e., push or extend dest with upppppppppp
returned from unparsed.with_property_id) so that the rewritten physical
border-top/border-bottom properties are emitted; keep the existing call to
self.flushed_properties.insert(BorderProperty::try_from_property_id(PropertyIdTag::$id).unwrap()).
In `@src/css/selectors/parser.rs`:
- Around line 1651-1653: The debug/display mismatch: make the debug() helpers
only available for the concrete impl_::Selectors and fix SelectorDebugFmt
output; specifically, move or reimplement GenericSelectorList<Impl>::debug() and
GenericSelector<Impl>::debug() as impl blocks for
GenericSelectorList<impl_::Selectors> and GenericSelector<impl_::Selectors> (so
they return SelectorListDebugFmt/SelectorDebugFmt that match the existing
fmt::Display impl for SelectorListDebugFmt<'a, impl_::Selectors> and
SelectorDebugFmt<'a, impl_::Selectors>), and in SelectorDebugFmt::fmt ensure
both branches write the closing ')' (append ')' in the Ok branch and in the Err
branch) and replace the Err branch's writeln! with write! so no stray newline is
emitted; leave parse_attribute_selector logic as-is.
- Around line 1964-1968: The SelectorDebugFmt implementation writes the opening
"Selector(" but never emits the closing ")" and the Err branch wrongly uses
writeln! (inserting a newline); update the match arms in the Debug impl for
SelectorDebugFmt so both arms call write! (not writeln!) and append the closing
parenthesis—i.e., for Ok(()) write the buffer then write!(")") (or include the
")" in the same write! that writes the buffer) and for Err(e) write the error
message without a newline and then write!(")") so the formatted output is a
single line like Selector(<content>) in both success and error cases.
In `@src/css/values/color.rs`:
- Around line 2497-2503: The match arm for ColorSpaceName::XyzD50 dispatches to
interpolate::<XYZd65>, causing xyz-d50 mixes to run in the wrong colorspace;
update that arm to call first_color.interpolate::<XYZd50>(p1, &second_color, p2,
hue_method) (replace XYZd65 with XYZd50) so ColorSpaceName::XyzD50 uses the
correct XYZd50 implementation.
- Around line 1507-1520: Construct and return the specific enum variants instead
of always using LABColor::Lab: update new_oklab to return LABColor::OkLab(OKLAB
{ l, a, b, alpha }), update new_lch to return LABColor::Lch(LCH { l, a, b, alpha
}), and update new_oklch to return LABColor::OkLch(OKLCH { l, a, b, alpha }) so
each constructor preserves its intended colorspace variant (modify the
variant/struct names to match the exact identifiers used in the file if they
differ).
In `@src/errno/lib.rs`:
- Around line 380-399: Add a Windows-gated unit test for win32_errno_name that
asserts known mappings (e.g., GetLastError codes like ERROR_FILE_NOT_FOUND /
ERROR_ACCESS_DENIED map to their SystemErrno names) and that an unmapped or
out-of-range u32 returns None; place it under #[cfg(windows)] in the same test
module so it exercises win32_errno_name (and the u16::try_from fallback)
directly. Use the win32_errno_name function name to locate the code and include
at least two mapped codes and one unmapped/high u32 value to lock down both
translation and fallback behavior.
---
Outside diff comments:
In `@src/ast/lib.rs`:
- Around line 3443-3452: The guard is currently auto-Send because it only holds
PhantomData<T>, so dropping it on a different thread can access the wrong
thread-local state; change
DebugOnlyDisablerScope<T>(core::marker::PhantomData<T>) to carry a PhantomData
of a non-Send marker (for example core::marker::PhantomData<std::rc::Rc<()>>) so
the type becomes !Send, update the constructor DebugOnlyDisabler::scope to
create the new PhantomData variant, and keep the Drop impl that calls
DebugOnlyDisabler::enable() unchanged.
In `@src/bundler/options.rs`:
- Around line 382-400: stdin_name_map currently never assigns a pseudo-filename
for Loader::Jsonc, leaving its entry as an empty slice; update the
stdin_name_map initializer (function stdin_name_map) to set map[Loader::Jsonc] =
b"input.jsonc" so JSONC stdin receives an extension-bearing name for diagnostics
and extension-based logic.
In `@src/bundler/ThreadPool.rs`:
- Around line 150-166: The else branch that returns the already-initialized
static pool must increment the global reference counter so new owners are
counted; update the branch that runs when REF_COUNT.load(...) != 0 to atomically
increment REF_COUNT (e.g., fetch_add(1, ...) with appropriate ordering) while
still holding MUTEX (or immediately after verifying initialization) before
returning the THREAD_POOL pointer created via ThreadPoolLib::ThreadPool::init,
so shutdown() will see the correct live owner count for REF_COUNT and the static
THREAD_POOL won't be dropped while still in use.
In `@src/bundler/transpiler.rs`:
- Around line 3042-3054: The current code uses MacroRemap::default() and calls
m.insert(k, inner) where StringArrayHashMap::insert returns Option<V> and is not
fallible; replace the insert call with the fallible put API to preserve
AllocError->bun error handling: call m.put(k, inner)? (or wrap with
bun_core::handle_oom(m.put(k, inner))) when iterating self.options.macro_remap
so failures propagate; keep the surrounding inner = v.clone().map_err(|_|
bun_core::err!("OutOfMemory"))? logic and otherwise return m as before.
In `@src/collections/linear_fifo.rs`:
- Around line 119-145: StaticBuffer currently reinterprets the whole
[MaybeUninit<T>; N] as &[T] / &mut [T] (see as_slice, as_mut_slice using
assume_init_slice/_mut) which creates references to uninitialized T; change the
accessor surface to expose &[MaybeUninit<T>] and &mut [MaybeUninit<T>] instead
and update the LinearFifoBuffer trait signatures accordingly so callers must
assume-init only the logically-written subranges (use assume_init_slice/_mut
only on those subranges), or alternatively add a strict trait bound restricting
T to a POD/any-bit-pattern-safe marker if you prefer to keep returning &[T];
pick one approach and apply it consistently to StaticBuffer::as_slice,
StaticBuffer::as_mut_slice and the LinearFifoBuffer trait so no path casts the
full buffer to &[T] over unwritten slots.
In `@src/css/css_parser.rs`:
- Around line 5663-5676: split_source_map currently returns &contents[..i+1]
(including the delimiter) and returns None when no delimiter is found, which
drops EOF-terminated URLs; change the behavior so when a delimiter byte is
matched in split_source_map (matching bytes in contents via the for (i, &c)
loop) you return the slice excluding the delimiter (e.g., &contents[0..i]) and
if the loop completes with no delimiter found return Some(contents) so
EOF-terminated URLs are preserved.
- Around line 2015-2047: The AtRule Prelude Layer branch mutates parser state
(calls T::on_layer_rule, possibly T::push_to_enclosing_layer or
T::bump_anon_layer_count) before calling parse_style_block(input) and does not
restore state if parse_style_block returns an error; create an RAII guard (e.g.,
LayerContextGuard) inside the AtRulePrelude::Layer handling that captures
old_len, whether a name was pushed or anon count bumped, and on Drop calls
T::reset_enclosing_layer(this.at_rule_parser, old_len) and undoes the anon bump
or removed push as appropriate, and ensure the guard is dismissed/disabled only
after parse_style_block succeeds so the cleanup runs on error paths (refer to
symbols: parse_style_block, T::on_layer_rule, T::push_to_enclosing_layer,
T::bump_anon_layer_count, T::reset_enclosing_layer, T::enclosing_layer_length,
the local name variable).
- Around line 3015-3084: The two loops that scan self.rules.v set saw_imports on
the first CssRule::Import and then unconditionally break at the end of that
iteration, so only the first leading `@import` is counted/rewritten; update both
loops (the initial counting loop and the rewrite loop that assigns
import_record_idx and pushes to out.v) to not break immediately when saw_imports
becomes true but instead continue processing subsequent rules and only break
when you encounter the first non-import rule after you have already seen imports
(i.e., break only when saw_imports == true and the current rule is neither
CssRule::Import nor an allowed-leading rule like CssRule::LayerBlock or
CssRule::Unknown with name == b"tailwind"); ensure new_import_records.len() and
import_rule.import_record_idx logic runs for each Import encountered so
import_record_idx values remain correct for all leading imports.
In `@src/css/media_query.rs`:
- Around line 1054-1060: The serializer is emitting '-' for equality which
breaks round-trip; in MediaFeatureComparison::Equal inside to_css replace the
call that writes '-' with one that writes '=' (i.e. change the delimiter passed
to dest.delim from b'-' to b'=') so equality media ranges serialize as "(feature
= value)"; keep the same call site (to_css -> dest.delim) and ensure surrounding
behavior (the true flag) remains unchanged.
In `@src/css/properties/font.rs`:
- Around line 748-755: The branch that attempts to parse
FontVariantCaps::parse_css2 is incorrectly guarded by variant_caps.is_some(),
making it unreachable because variant_caps starts as None; change the condition
to variant_caps.is_none() so the if let Ok(value) =
input.try_parse(FontVariantCaps::parse_css2) branch can run when no value is yet
set, assign variant_caps = Some(value), increment count, and continue (the code
around the variant_caps variable and the input.try_parse call should be the
target).
In `@src/css/properties/grid.rs`:
- Around line 525-557: When strings::starts_with_char(rest, b'.') is true, treat
it as a null-cell: append a None entry to tokens, advance string past the '.'
(e.g., string = &rest[1..]) and continue the parsing loop instead of falling
through into the name-codepoint logic; this fixes handling of unnamed areas
(update the block around strings::starts_with_char(rest, b'.') to
tokens.append(None), advance string, and continue while preserving bump usage
where needed).
- Around line 470-500: The code only calls input.try_parse(|i|
i.expect_string()) once, so subsequent grid-template-areas rows are ignored;
change the single if let Ok(s) = ... block into a loop (while let Ok(s) =
input.try_parse(|i| i.expect_string().map(std::ptr::from_ref::<[u8]>))) and
repeat the parsing/column-consistency logic for each row using the existing
parse_string, row, columns, and tokens variables, keeping the unsafe
crate::arena_str conversion inside the loop; after the loop, if row == 0 handle
the non-string case by returning an appropriate parse error (unless the token is
the keyword `none`), so inputs with no leading string no longer silently fall
through to Areas { columns: 0, areas: [] } but instead error correctly.
In `@src/css/selectors/parser.rs`:
- Around line 3725-3747: The current code only lowercases the suffix after the
first uppercase, causing misclassification for names like "dIr"; change the
logic around local_name_lower/local_name_is_ascii_lowercase so that when any
uppercase is found you call arena_lowercase(input.arena(), local_name) to
produce a fully-lowercased Ident (use Ident { v: lowered }) and set
local_name_is_ascii_lowercase = false; keep the existing branch for the
no-uppercase case that returns Ident { v: std::ptr::from_ref::<[u8]>(local_name)
} and true; ensure this updated local_name_lower is what gets passed into
attribute_flags.to_case_sensitivity(...) so is_html_case_insensitive_attribute
sees the full lowercased name.
In `@src/css/values/color.rs`:
- Around line 2397-2403: The match arm in the colorspace dispatch uses the
incorrect literal b"a99-rgb", causing valid "a98-rgb" inputs to be rejected;
update the match in the crate::match_ignore_ascii_case! block (the arm that
constructs PredefinedColor::A98 via A98 { r: a, g: b, b: c, alpha }) to match
b"a98-rgb" instead so it aligns with the earlier from handling and the A98
type/PredefinedColor::A98 variant.
🪄 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: 365c518e-c2f5-4bfb-9a5c-4fb23638e5cc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (299)
src/analytics/lib.rssrc/analytics/schema.rssrc/api/lib.rssrc/ast/Cargo.tomlsrc/ast/ast_result.rssrc/ast/b.rssrc/ast/char_freq.rssrc/ast/e.rssrc/ast/expr.rssrc/ast/g.rssrc/ast/import_record.rssrc/ast/known_global.rssrc/ast/lib.rssrc/ast/loader.rssrc/ast/new_store.rssrc/ast/nodes.rssrc/ast/op.rssrc/ast/runtime.rssrc/ast/scope.rssrc/ast/server_component_boundary.rssrc/ast/stmt.rssrc/ast/symbol.rssrc/ast/target.rssrc/ast/ts.rssrc/base64/lib.rssrc/brotli/lib.rssrc/brotli_sys/brotli_c.rssrc/bun.js.rssrc/bun_alloc/MaxHeapAllocator.rssrc/bun_alloc/NullableAllocator.rssrc/bun_alloc/fallback/z.rssrc/bun_alloc/heap_breakdown.rssrc/bun_alloc/lib.rssrc/bun_alloc/maybe_owned.rssrc/bun_alloc/memory.rssrc/bun_bin/lib.rssrc/bun_core/Global.rssrc/bun_core/Progress.rssrc/bun_core/bounded_array.rssrc/bun_core/deprecated.rssrc/bun_core/env.rssrc/bun_core/env_var.rssrc/bun_core/external_shared.rssrc/bun_core/fmt.rssrc/bun_core/lib.rssrc/bun_core/output.rssrc/bun_core/result.rssrc/bun_core/string/MutableString.rssrc/bun_core/string/SmolStr.rssrc/bun_core/string/StringBuilder.rssrc/bun_core/string/StringJoiner.rssrc/bun_core/string/escapeRegExp.rssrc/bun_core/string/identifier.rssrc/bun_core/string/immutable.rssrc/bun_core/string/immutable/exact_size_matcher.rssrc/bun_core/string/immutable/unicode.rssrc/bun_core/string/mod.rssrc/bun_core/string/wtf.rssrc/bun_core/tty.rssrc/bun_core/util.rssrc/bun_core/wtf.rssrc/bundler/AstBuilder.rssrc/bundler/BundleThread.rssrc/bundler/Cargo.tomlsrc/bundler/Chunk.rssrc/bundler/DeferredBatchTask.rssrc/bundler/Graph.rssrc/bundler/HTMLScanner.rssrc/bundler/IndexStringMap.rssrc/bundler/LinkerContext.rssrc/bundler/LinkerGraph.rssrc/bundler/OutputFile.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/analyze_transpiled_module.rssrc/bundler/barrel_imports.rssrc/bundler/bundle_v2.rssrc/bundler/bundled_ast.rssrc/bundler/cache.rssrc/bundler/defines.rssrc/bundler/entry_points.rssrc/bundler/lib.rssrc/bundler/linker.rssrc/bundler/linker_context/MetafileBuilder.rssrc/bundler/linker_context/OutputFileListBuilder.rssrc/bundler/linker_context/StaticRouteVisitor.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/convertStmtsForChunkForDevServer.rssrc/bundler/linker_context/doStep5.rssrc/bundler/linker_context/findAllImportedPartsInJSOrder.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForCssChunk.rssrc/bundler/linker_context/generateCompileResultForHtmlChunk.rssrc/bundler/linker_context/generateCompileResultForJSChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessHTMLChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/renameSymbolsInChunk.rssrc/bundler/linker_context/scanImportsAndExports.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/bundler_jsc/analyze_jsc.rssrc/bunfig/arguments.rssrc/bunfig/bunfig.rssrc/cares_sys/c_ares.rssrc/clap/args.rssrc/clap/comptime.rssrc/clap/lib.rssrc/clap/streaming.rssrc/collections/StaticHashMap.rssrc/collections/array_hash_map.rssrc/collections/array_list.rssrc/collections/bit_set.rssrc/collections/comptime_string_map.rssrc/collections/hive_array.rssrc/collections/lib.rssrc/collections/linear_fifo.rssrc/collections/pool.rssrc/collections/string_map.rssrc/crash_handler/CPUFeatures.rssrc/crash_handler/handle_oom.rssrc/crash_handler/lib.rssrc/csrf/lib.rssrc/css/context.rssrc/css/css_modules.rssrc/css/css_parser.rssrc/css/declaration.rssrc/css/dependencies.rssrc/css/error.rssrc/css/generics.rssrc/css/lib.rssrc/css/media_query.rssrc/css/printer.rssrc/css/properties/align.rssrc/css/properties/animation.rssrc/css/properties/background.rssrc/css/properties/border.rssrc/css/properties/border_image.rssrc/css/properties/border_radius.rssrc/css/properties/box_shadow.rssrc/css/properties/css_modules.rssrc/css/properties/custom.rssrc/css/properties/display.rssrc/css/properties/flex.rssrc/css/properties/font.rssrc/css/properties/grid.rssrc/css/properties/margin_padding.rssrc/css/properties/masking.rssrc/css/properties/mod.rssrc/css/properties/overflow.rssrc/css/properties/prefix_handler.rssrc/css/properties/properties_generated.rssrc/css/properties/properties_impl.rssrc/css/properties/shape.rssrc/css/properties/size.rssrc/css/properties/text.rssrc/css/properties/transform.rssrc/css/properties/transition.rssrc/css/properties/ui.rssrc/css/rules/container.rssrc/css/rules/counter_style.rssrc/css/rules/document.rssrc/css/rules/font_face.rssrc/css/rules/font_palette_values.rssrc/css/rules/import.rssrc/css/rules/keyframes.rssrc/css/rules/layer.rssrc/css/rules/media.rssrc/css/rules/mod.rssrc/css/rules/namespace.rssrc/css/rules/nesting.rssrc/css/rules/page.rssrc/css/rules/property.rssrc/css/rules/scope.rssrc/css/rules/starting_style.rssrc/css/rules/style.rssrc/css/rules/supports.rssrc/css/rules/unknown.rssrc/css/rules/viewport.rssrc/css/selectors/builder.rssrc/css/selectors/parser.rssrc/css/selectors/selector.rssrc/css/small_list.rssrc/css/sourcemap.rssrc/css/targets.rssrc/css/values/alpha.rssrc/css/values/angle.rssrc/css/values/calc.rssrc/css/values/color.rssrc/css/values/easing.rssrc/css/values/gradient.rssrc/css/values/ident.rssrc/css/values/image.rssrc/css/values/length.rssrc/css/values/percentage.rssrc/css/values/position.rssrc/css/values/ratio.rssrc/css/values/rect.rssrc/css/values/size.rssrc/css/values/syntax.rssrc/css/values/time.rssrc/css/values/url.rssrc/css_jsc/color_js.rssrc/css_jsc/css_internals.rssrc/dns/lib.rssrc/dotenv/env_loader.rssrc/errno/lib.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/AnyTaskWithExtraContext.rssrc/event_loop/AutoFlusher.rssrc/event_loop/ConcurrentTask.rssrc/event_loop/DeferredTaskQueue.rssrc/event_loop/EventLoopTimer.rssrc/event_loop/ManagedTask.rssrc/event_loop/MiniEventLoop.rssrc/event_loop/SpawnSyncEventLoop.rssrc/event_loop/lib.rssrc/exe_format/elf.rssrc/exe_format/macho.rssrc/exe_format/pe.rssrc/glob/GlobWalker.rssrc/glob/lib.rssrc/glob/matcher.rssrc/highway/lib.rssrc/http/AsyncHTTP.rssrc/http/CertificateInfo.rssrc/http/Decompressor.rssrc/http/H2Client.rssrc/http/H3Client.rssrc/http/HTTPCertError.rssrc/http/HTTPContext.rssrc/http/HTTPRequestBody.rssrc/http/HTTPThread.rssrc/http/HeaderBuilder.rssrc/http/HeaderValueIterator.rssrc/http/Headers.rssrc/http/InternalState.rssrc/http/ProxyTunnel.rssrc/http/SendFile.rssrc/http/Signals.rssrc/http/ThreadSafeStreamBuffer.rssrc/http/h2_client/ClientSession.rssrc/http/h2_client/PendingConnect.rssrc/http/h2_client/Stream.rssrc/http/h2_client/dispatch.rssrc/http/h2_client/encode.rssrc/http/h3_client/AltSvc.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/encode.rssrc/http/lib.rssrc/http/lshpack.rssrc/http/ssl_config.rssrc/http/websocket.rssrc/http/zlib.rssrc/http_jsc/headers_jsc.rssrc/http_jsc/method_jsc.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/CppWebSocket.rssrc/http_jsc/websocket_client/WebSocketDeflate.rssrc/http_jsc/websocket_client/WebSocketProxy.rssrc/http_jsc/websocket_client/WebSocketProxyTunnel.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/http_types/ETag.rssrc/http_types/Method.rssrc/http_types/MimeType.rssrc/http_types/URLPath.rssrc/http_types/h2.rssrc/ini/lib.rssrc/install/ConfigVersion.rssrc/install/NetworkTask.rssrc/install/PackageInstall.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/CommandLineArguments.rssrc/install/PackageManager/PackageJSONEditor.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/PackageManagerLifecycle.rssrc/install/PackageManager/PackageManagerOptions.rssrc/install/PackageManager/PackageManagerResolution.rssrc/install/PackageManager/PopulateManifestCache.rssrc/install/PackageManager/ProgressStrings.rssrc/install/PackageManager/UpdateRequest.rssrc/install/PackageManager/WorkspacePackageJSONCache.rssrc/install/PackageManager/install_with_manager.rssrc/install/PackageManager/patchPackage.rssrc/install/PackageManager/processDependencyList.rssrc/install/PackageManager/runTasks.rssrc/install/PackageManager/security_scanner.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/PackageManagerTask.rs
💤 Files with no reviewable changes (17)
- src/css/rules/document.rs
- src/ast/op.rs
- src/bundler/linker_context/generateCompileResultForCssChunk.rs
- src/css/rules/nesting.rs
- src/css/values/gradient.rs
- src/css/values/ratio.rs
- src/bun_core/tty.rs
- src/css/error.rs
- src/css/rules/starting_style.rs
- src/bundler/IndexStringMap.rs
- src/css/small_list.rs
- src/crash_handler/CPUFeatures.rs
- src/bun.js.rs
- src/css/properties/overflow.rs
- src/bun_alloc/fallback/z.rs
- src/css/values/angle.rs
- src/css/values/time.rs
Comment on lines
+1517
to
+1518
| // Unlike Zig, there is no per-scope `[4096]u8` buffered writer; logs route | ||
| // through `scoped_writer()` directly (debug-logging perf only). |
Contributor
There was a problem hiding this comment.
Serialize writes across scoped loggers.
After removing the per-scope buffer, self.lock only protects one ScopedLogger, but every scope still writes through the same SCOPED_FILE_WRITER. write_fmt can issue multiple write_str calls, so two scopes logging at once can interleave bytes and garble a single line. Add a shared sink lock around write_fmt/flush, or restore per-scope buffering.
🔒 Minimal fix
pub mod scoped_debug_writer {
use super::*;
+ pub(crate) static WRITE_LOCK: Mutex<()> = Mutex::new(());
pub(crate) static SCOPED_FILE_WRITER: crate::RacyCell<QuietWriter> =
crate::RacyCell::new(QuietWriter::ZEROED); let _lock = self.lock.lock();
+ let _sink_lock = scoped_debug_writer::WRITE_LOCK.lock();
let mut out = scoped_writer();
let result = out.write_fmt(args);Also applies to: 1606-1621
🤖 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/bun_core/output.rs` around lines 1517 - 1518, The issue is that removing
per-scope buffers means self.lock only serializes access to a single
ScopedLogger while all scopes call into the shared SCOPED_FILE_WRITER, allowing
concurrent write_fmt/write_str sequences to interleave; fix by introducing a
shared sink lock around the shared writer so write_fmt and flush are performed
atomically (e.g., add a global Mutex or RwLock used by SCOPED_FILE_WRITER and
acquire it in ScopedLogger::write_fmt/flush before calling into the writer), or
alternatively restore a per-scope buffer in ScopedLogger and emit a single write
to SCOPED_FILE_WRITER, ensuring no partial write_str calls are issued to the
shared sink.
Comment on lines
+1651
to
+1653
| // Concrete `impl_::Selectors` only — `sel.debug()` formatting requires the | ||
| // concrete `Display` impl on `SelectorDebugFmt` (see the comment there). | ||
| impl<'a> fmt::Display for SelectorListDebugFmt<'a, impl_::Selectors> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/css/selectors/parser.rs"
# Sanity: confirm file exists and print nearby line ranges.
ls -l "$FILE"
# Show the exact code around the referenced lines.
sed -n '1620,1710p' "$FILE" | cat -n
echo "----"
sed -n '1900,2005p' "$FILE" | cat -n
echo "----"
# Locate parse_attribute_selector and show its implementation vicinity.
rg -n "fn parse_attribute_selector" "$FILE"
# Print a window around it.
python3 - <<'PY'
import re, pathlib
p=pathlib.Path("src/css/selectors/parser.rs")
txt=p.read_text()
m=re.search(r"fn parse_attribute_selector[^{]*\{", txt)
if not m:
print("parse_attribute_selector not found")
raise SystemExit(1)
start=txt.rfind("\n",0,m.start())
# Convert char index to line number
line_count=txt[:m.start()].count("\n")+1
print("parse_attribute_selector starts at line", line_count)
PY
# print around estimated start
python3 - <<'PY'
import re, pathlib
p=pathlib.Path("src/css/selectors/parser.rs")
txt=p.read_text().splitlines()
for i,line in enumerate(txt,1):
if "fn parse_attribute_selector" in line:
start=max(1,i-80)
end=min(len(txt),i+220)
for j in range(start,end+1):
print(f"{j}: {txt[j-1]}")
break
PY
echo "----"
# Locate SelectorDebugFmt::fmt and print its body.
rg -n "struct SelectorDebugFmt|impl.*SelectorDebugFmt|SelectorDebugFmt::fmt|fn fmt\(" "$FILE"
rg -n "SelectorDebugFmt" "$FILE"
python3 - <<'PY'
import pathlib
p=pathlib.Path("src/css/selectors/parser.rs")
lines=p.read_text().splitlines()
# Find impl fmt::Display for SelectorDebugFmt and print around it.
for i,l in enumerate(lines,1):
if "impl" in l and "fmt::Display for SelectorDebugFmt" in l:
start=max(1,i-20); end=min(len(lines),i+120)
for j in range(start,end+1):
print(f"{j}: {lines[j-1]}")
break
PY
echo "----"
# Search for the lowercasing logic referencing local_name_lower[first_uppercase..]
rg -n "local_name_lower|first_uppercase" "$FILE"
echo "----"
# Check that debug wrappers still exist for GenericSelector/GenericSelectorList.
rg -n "impl_::|GenericSelector.*debug|GenericSelectorList.*debug|fn debug\\(" "$FILE"
# Print around generic debug methods if present.
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("src/css/selectors/parser.rs")
lines=p.read_text().splitlines()
targets=[]
for i,l in enumerate(lines,1):
if re.search(r"fn debug\(", l) and ("GenericSelector" in l or "GenericSelectorList" in l):
targets.append(i)
for t in targets:
start=max(1,t-40); end=min(len(lines),t+120)
print("---- debug around", t)
for j in range(start,end+1):
print(f"{j}: {lines[j-1]}")
PYRepository: oven-sh/bun
Length of output: 34098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/css/selectors/parser.rs"
# Find arena_lowercase definition / uses.
rg -n "fn arena_lowercase|arena_lowercase\\(" src/css/selectors/parser.rs src/css -S
# If not in this file, search repo-wide for the helper definition.
rg -n "fn arena_lowercase" src -S
# Show the function body with a tight window around its definition.
def_file="$(rg -n "fn arena_lowercase" src -S --files-with-matches | head -n 1)"
echo "arena_lowercase defined in: $def_file"
rg -n "fn arena_lowercase" "$def_file" -n
start=$(rg -n "fn arena_lowercase" "$def_file" | head -n 1 | cut -d: -f1)
# print ~60 lines around it
sed -n "$((start-10)),$((start+120))p" "$def_file" | cat -n
# Also show any comment mentioning Zig DebugFmt.format for SelectorDebugFmt.
rg -n "DebugFmt\\.format|Mirrors the Zig|Zig `DebugFmt|Selector\\(" "$FILE" -SRepository: oven-sh/bun
Length of output: 7383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/css/selectors/parser.rs"
# Locate to_case_sensitivity definition and the types involved.
rg -n "to_case_sensitivity\\(" "$FILE"
# Show surrounding code for the callsites already present plus definition(s) elsewhere in module.
rg -n "fn to_case_sensitivity" -S src/css/selectors -g'*.rs'
# Inspect attrs::ParsedCaseSensitivity type / usage.
rg -n "ParsedCaseSensitivity" src/css/selectors -S
# Inspect how Ident compares for case sensitivity: find Ident struct and its eql/hash behavior.
rg -n "struct Ident\\b" -S src/css/selectors src/css -g'*.rs'
rg -n "LocalName|local_name_lower" -S src/css/selectors/parser.rs src/css/selectors -g'*.rs'
# Narrow to attrs matching/equality: find where ParsedCaseSensitivity is consumed.
rg -n "case_sensitivity" src/css/selectors -S
# Show the exact match/impl for SelectorListDebugFmt / SelectorDebugFmt again with more context (avoid diff).
sed -n '1600,1705p' "$FILE" | cat -n
sed -n '1900,2005p' "$FILE" | cat -nRepository: oven-sh/bun
Length of output: 3824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/css/selectors/parser.rs"
# Show ParsedCaseSensitivity enum + AttributeFlags->case conversion.
sed -n '380,520p' "$FILE" | cat -n
echo "----"
sed -n '4180,4320p' "$FILE" | cat -n
echo "----"
# Inspect AttributeFlags / parse_attribute_flags bits around the local lines.
rg -n "struct AttributeFlags|enum AttributeFlags|fn parse_attribute_flags" "$FILE"
rg -n "parse_attribute_flags" "$FILE"
sed -n '3960,4300p' "$FILE" | cat -n | head -n 220
echo "----"
# Inspect selector.rs for how ParsedCaseSensitivity is used during matching/serialization.
SEL="src/css/selectors/selector.rs"
rg -n "ParsedCaseSensitivity::|case_sensitive" "$SEL"
# Print around the earlier uses (roughly 260-380, 850-920, and 1580-1630).
sed -n '260,380p' "$SEL" | cat -n
echo "----"
sed -n '840,940p' "$SEL" | cat -n
echo "----"
sed -n '1560,1640p' "$SEL" | cat -nRepository: oven-sh/bun
Length of output: 38649
Fix selector debug Display/debug() contract mismatch and repair SelectorDebugFmt output.
GenericSelectorList<Impl>::debug()/GenericSelector<Impl>::debug()are generic overImpl: BunSelectorImpl, butfmt::Displayexists only forSelectorListDebugFmt<'a, impl_::Selectors>/SelectorDebugFmt<'a, impl_::Selectors>;format!("{}", sel.debug())won’t compile for otherImpl. Restrictdebug()toimpl_::Selectors(or makeDisplayavailable for all relevantImpls).SelectorDebugFmt::fmtwritesSelector(but never writes the closing), and theErrbranch useswriteln!(newline). Add the closing)in both branches and switch the error branch towrite!so diagnostics are well-formed.- The
parse_attribute_selectorlowercasing logic is fine:first_uppercaseis the first ASCIIA..=Zbyte, so bytes before it contain no uppercase; lowercasing only&local_name[first_uppercase..]still produces a fully lowercase cache forlocal_name_lower.
🤖 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/css/selectors/parser.rs` around lines 1651 - 1653, The debug/display
mismatch: make the debug() helpers only available for the concrete
impl_::Selectors and fix SelectorDebugFmt output; specifically, move or
reimplement GenericSelectorList<Impl>::debug() and
GenericSelector<Impl>::debug() as impl blocks for
GenericSelectorList<impl_::Selectors> and GenericSelector<impl_::Selectors> (so
they return SelectorListDebugFmt/SelectorDebugFmt that match the existing
fmt::Display impl for SelectorListDebugFmt<'a, impl_::Selectors> and
SelectorDebugFmt<'a, impl_::Selectors>), and in SelectorDebugFmt::fmt ensure
both branches write the closing ')' (append ')' in the Ok branch and in the Err
branch) and replace the Err branch's writeln! with write! so no stray newline is
emitted; leave parse_attribute_selector logic as-is.
Collaborator
Author
There was a problem hiding this comment.
Deliberate parity: the reference DebugFmt (selectors/parser.zig:1620-1639) also writes "Selector(" with no closing paren and prints the error arm with a trailing newline; this is a debug-builds-only formatter and the output shape is kept identical. The generic debug() returning a wrapper whose Display is only implemented for the concrete impl_::Selectors compiles fine and matches how the original is used.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, got it — thanks for the context. I’ll retract this finding: keeping the unterminated Selector( / newline error arm for Zig DebugFmt parity is intentional, and the concrete-only Display impl for the generic debug wrapper is fine given the actual impl_::Selectors usage.
🐇💭
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/css/selectors/parser.rs:1631-1633
Timestamp: 2026-06-05T01:49:57.190Z
Learning: In oven-sh/bun `src/css/selectors/parser.rs`, do not flag `SelectorDebugFmt` / `SelectorListDebugFmt` debug output for missing a closing `)` or using a trailing newline in the error arm. This intentionally matches the reference `selectors/parser.zig` `DebugFmt` behavior and is debug-build-only parity. Also, do not flag generic `GenericSelectorList<Impl>::debug()` / `GenericSelector<Impl>::debug()` returning wrappers whose `fmt::Display` is implemented only for `impl_::Selectors`; this compiles for the intended concrete usage and matches how the original is used.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/resolve_builtins/HardcodedModule.rs:179-186
Timestamp: 2026-06-04T23:12:16.599Z
Learning: In oven-sh/bun (`src/resolve_builtins/HardcodedModule.rs`), the `#[strum(serialize = "...")]` attribute on `HardcodedModule` enum variants defines the `ResolvedSourceTag` identifier (e.g., `"internal:repl"` with a colon), which matches `generated_resolved_source_tag.rs`. The `HardcodedModule::MAP` and `BUN_EXTRA_ALIAS_KVS` entries independently use the slash-form require specifiers (e.g., `b"internal/repl"`). The two forms serve distinct purposes and there is no code path that round-trips a `ResolvedSourceTag` name back into a require specifier for these debug-only (`--expose-internals` / debug build) entries. Do NOT flag the colon-vs-slash mismatch between strum serializations and MAP/Alias keys for `NodeInternalRepl`, `NodeInternalReplAwait`, `NodeInternalReplHistory`, and `NodeInternalUtilInspect` as a bug.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to src/**/*.zig : `.zig` files alongside `.rs` files are original Zig implementations kept only as porting references - new code goes in `.rs`, never add new behavior to `.zig` files
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.rs : Platform-specific code: compile across all targets (linux/macos/windows × x64/aarch64) by running `bun run rust:check-all` when making platform-specific changes
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.
Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31175
File: src/valkey/valkey_protocol.rs:407-408
Timestamp: 2026-05-22T01:23:05.156Z
Learning: In oven-sh/bun Rust crates configured with `edition = "2024"`, `size_of::<T>()` is available via the Rust 2024 prelude. When reviewing, do not flag `size_of::<T>()` calls as missing `use core::mem::size_of` or `use std::mem::size_of` imports in these crates.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31338
File: src/runtime/shell/shell_body.rs:923-926
Timestamp: 2026-05-24T10:04:48.082Z
Learning: When reviewing oven-sh/bun code, do NOT treat calls to `JSValue::get_own_truthy` as a “missing JS truthiness” check. Despite the misleading name, `get_own_truthy` only performs an own-property slot lookup (via `get_own`, no prototype walk) and filters out only `undefined` (internally: `!prop.is_undefined()`). Values like `""`, `false`, `0`, and `null` are allowed and should return `Some(...)`. So reviews should not flag uses for not excluding other falsy values; the correct semantic gate to expect is only the `undefined` guard.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31335
File: src/install/PackageManager/PackageManagerDirectories.rs:842-848
Timestamp: 2026-05-24T10:58:13.503Z
Learning: When reviewing Rust code in this repo that indexes/slices a `PathBuffer` (which derefs to a `&[u8]` in `src/bun_core/util.rs`), treat slice indexing like `buf[..n]`, `buf[n..len]`, or `buf[len] = 0` as bounds-checked by Rust. Rust’s slice/indexing machinery will panic on out-of-bounds/overflow rather than write past the buffer. Therefore, do not flag these operations as potential out-of-bounds/unsafe writes, and do not recommend adding `debug_assert!` bounds checks that merely duplicate the existing Rust slice bounds checks (the checks are already enforced at indexing).
Learnt from: robobun
Repo: oven-sh/bun PR: 31579
File: src/sys/lib.rs:9225-9231
Timestamp: 2026-05-29T19:17:51.017Z
Learning: In this repo’s Rust code, don’t treat pattern matching on a borrowed enum/struct field as an incorrect “move-out-of-&” error when the pattern only binds payload fields that are `Copy` (e.g., matching `WriteFileData::Buffer { buffer }` where `buffer: &[u8]`, or `PathOrFileDescriptor::Path(bytes)` / `Fd(fd)` where `bytes: &[u8]` and `Fd` is `Copy`). Rust allows copying `Copy` payloads out of a borrowed aggregate without requiring the aggregate enum/struct itself to be `Copy`. In code review, if the compile error is about moving the aggregate but the bound fields are `Copy`, don’t recommend making the whole enum/struct `Copy` (e.g., don’t suggest `#[derive(Copy)]` on the enum solely for this match); keeping it non-`Copy` may be intentional for future non-`Copy` variants.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/jsc/VirtualMachine.rs:2598-2599
Timestamp: 2026-05-30T00:05:21.707Z
Learning: When reviewing Rust in this repo, do not flag direct `.0` access on `bun_core::String` as an encapsulation / sealed-field violation if it is used to reach the public inner newtype field and then call a public inner method (e.g., `bun_core::String::ascii(msg).0.mark_global()`). This usage is intended to preserve the prior Zig pattern (e.g., `...init(...).mark_global()`). Prefer considering `message.mark_global()`-style alternatives as style-only; only raise an issue if there is an actual API/compilation/semantic problem (e.g., method not public or code won’t compile / behavior changes), not merely because `.0` was accessed.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_fs.rs:6118-6119
Timestamp: 2026-05-30T00:05:31.762Z
Learning: When reviewing migration-only diffs that change implementation details like string types or API renames (e.g., ZigString -> BunString), do not treat pre-existing OOM handling patterns such as `.expect("oom")` as PR-introduced issues. Confirm in the base branch (before the change) that the same `.expect("oom")` usage pattern already existed; only flag it if the PR actually introduces or modifies the OOM-handling code path, not just the surrounding string/type/API migration.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_process.rs:398-398
Timestamp: 2026-05-30T00:05:32.312Z
Learning: In this codebase (oven-sh/bun), during ZigString → BunString migration reviews, treat `BunString::ascii(bytes)` **without** a subsequent `.with_encoding(...)` as byte-level equivalent to the old `ZigString::init(bytes)` **when the original Zig code had no encoding marking**. Therefore, do not flag `BunString::ascii(...)` (no `.with_encoding`) as a new encoding regression in that migration context. If the new code uses `.with_encoding(...)`, then encoding-related regression checks may still apply.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/server/server_body.rs:2002-2006
Timestamp: 2026-05-30T00:05:28.628Z
Learning: In this repo, avoid raising an issue just because code accesses the intentionally public inner tuple field of `bun_core::String` via `.0` (e.g., `some_bun_string.0.mark_utf8()`). Since `.0` is public and `bun_alloc::String::mark_utf8` is a public method, this pattern may be used to preserve base behavior during BunString/StringView migration. Only flag cases when there’s a separate, real problem (e.g., incorrect logic, misuse of the API), not merely the presence of `.0` field access.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/webcore/Blob.rs:6941-6943
Timestamp: 2026-05-30T00:05:38.722Z
Learning: When reviewing this repo’s Rust code for the (BunString) “compile-breaking sealed-internals” concern, do not flag tuple-field access like `bun_string.0.mark_global()` / `bun_string.0.mark_utf8()` as sealed-internals—provided that the tuple field `.0` is publicly visible and the invoked `mark_*` methods are also public. In that case, treat it as a style preference at most. Only raise/retain the sealed-internals issue if, for the codebase version you’re reviewing, the tuple field `.0` is not public (or the target `mark_*` methods are not public), making the access compile-breaking.
Comment on lines
+1964
to
1968
| write!(f, "Selector(")?; | ||
| match result { | ||
| Ok(()) => write!(f, "{}", bstr::BStr::new(&buf)), | ||
| Err(e) => writeln!(f, "<error writing selector: {}>", e.name()), | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the exact fmt block near the referenced lines
sed -n '1930,2020p' src/css/selectors/parser.rs | nl -ba | sed -n '1,140p'Repository: oven-sh/bun
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1930,2020p' src/css/selectors/parser.rs | nl -baRepository: oven-sh/bun
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1930,2020p' src/css/selectors/parser.rs | nl -baRepository: oven-sh/bun
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show formatter code around the referenced lines with line numbers (without `nl`)
awk 'NR>=1930 && NR<=2020 { printf "%5d\t%s\n", NR, $0 }' src/css/selectors/parser.rsRepository: oven-sh/bun
Length of output: 4358
Fix SelectorDebugFmt to close ) in both match arms
- The formatter writes
Selector(but neither theOknorErrbranch outputs the closing). - The
Errbranch useswriteln!, injecting a newline into what should be a single-line diagnostic.
🩹 Proposed fix
write!(f, "Selector(")?;
match result {
- Ok(()) => write!(f, "{}", bstr::BStr::new(&buf)),
- Err(e) => writeln!(f, "<error writing selector: {}>", e.name()),
+ Ok(()) => write!(f, "{})", bstr::BStr::new(&buf)),
+ Err(e) => write!(f, "<error writing selector: {}>)", e.name()),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| write!(f, "Selector(")?; | |
| match result { | |
| Ok(()) => write!(f, "{}", bstr::BStr::new(&buf)), | |
| Err(e) => writeln!(f, "<error writing selector: {}>", e.name()), | |
| } | |
| write!(f, "Selector(")?; | |
| match result { | |
| Ok(()) => write!(f, "{})", bstr::BStr::new(&buf)), | |
| Err(e) => write!(f, "<error writing selector: {}>)", e.name()), | |
| } |
🤖 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/css/selectors/parser.rs` around lines 1964 - 1968, The SelectorDebugFmt
implementation writes the opening "Selector(" but never emits the closing ")"
and the Err branch wrongly uses writeln! (inserting a newline); update the match
arms in the Debug impl for SelectorDebugFmt so both arms call write! (not
writeln!) and append the closing parenthesis—i.e., for Ok(()) write the buffer
then write!(")") (or include the ")" in the same write! that writes the buffer)
and for Err(e) write the error message without a newline and then write!(")") so
the formatted output is a single line like Selector(<content>) in both success
and error cases.
Collaborator
Author
There was a problem hiding this comment.
Same as the thread above: the missing ) and the newline in the error arm are byte-for-byte the reference DebugFmt's output (selectors/parser.zig:1625,1637); keeping it identical is intentional.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner got it — thanks for the pointer. I’ll treat the missing ) and the newline in the error arm as intentional byte-for-byte parity with the Zig DebugFmt, not a formatting bug.
૮₍ • ᴥ • ₎ა
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/css/selectors/parser.rs:1942-1946
Timestamp: 2026-06-05T01:49:55.688Z
Learning: In oven-sh/bun `src/css/selectors/parser.rs`, the `fmt::Display` implementation for `SelectorDebugFmt<'a, impl_::Selectors>` intentionally matches the reference `selectors/parser.zig` `DebugFmt` output byte-for-byte: the formatted `Selector(` output does not append a closing `)`, and the error arm uses newline-emitting behavior equivalent to the Zig reference around `selectors/parser.zig` lines 1625 and 1637. Do not flag the missing closing parenthesis or newline in the error branch as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to src/**/*.zig : `.zig` files alongside `.rs` files are original Zig implementations kept only as porting references - new code goes in `.rs`, never add new behavior to `.zig` files
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.rs : Platform-specific code: compile across all targets (linux/macos/windows × x64/aarch64) by running `bun run rust:check-all` when making platform-specific changes
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.
Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31175
File: src/valkey/valkey_protocol.rs:407-408
Timestamp: 2026-05-22T01:23:05.156Z
Learning: In oven-sh/bun Rust crates configured with `edition = "2024"`, `size_of::<T>()` is available via the Rust 2024 prelude. When reviewing, do not flag `size_of::<T>()` calls as missing `use core::mem::size_of` or `use std::mem::size_of` imports in these crates.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31338
File: src/runtime/shell/shell_body.rs:923-926
Timestamp: 2026-05-24T10:04:48.082Z
Learning: When reviewing oven-sh/bun code, do NOT treat calls to `JSValue::get_own_truthy` as a “missing JS truthiness” check. Despite the misleading name, `get_own_truthy` only performs an own-property slot lookup (via `get_own`, no prototype walk) and filters out only `undefined` (internally: `!prop.is_undefined()`). Values like `""`, `false`, `0`, and `null` are allowed and should return `Some(...)`. So reviews should not flag uses for not excluding other falsy values; the correct semantic gate to expect is only the `undefined` guard.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31335
File: src/install/PackageManager/PackageManagerDirectories.rs:842-848
Timestamp: 2026-05-24T10:58:13.503Z
Learning: When reviewing Rust code in this repo that indexes/slices a `PathBuffer` (which derefs to a `&[u8]` in `src/bun_core/util.rs`), treat slice indexing like `buf[..n]`, `buf[n..len]`, or `buf[len] = 0` as bounds-checked by Rust. Rust’s slice/indexing machinery will panic on out-of-bounds/overflow rather than write past the buffer. Therefore, do not flag these operations as potential out-of-bounds/unsafe writes, and do not recommend adding `debug_assert!` bounds checks that merely duplicate the existing Rust slice bounds checks (the checks are already enforced at indexing).
Learnt from: robobun
Repo: oven-sh/bun PR: 31579
File: src/sys/lib.rs:9225-9231
Timestamp: 2026-05-29T19:17:51.017Z
Learning: In this repo’s Rust code, don’t treat pattern matching on a borrowed enum/struct field as an incorrect “move-out-of-&” error when the pattern only binds payload fields that are `Copy` (e.g., matching `WriteFileData::Buffer { buffer }` where `buffer: &[u8]`, or `PathOrFileDescriptor::Path(bytes)` / `Fd(fd)` where `bytes: &[u8]` and `Fd` is `Copy`). Rust allows copying `Copy` payloads out of a borrowed aggregate without requiring the aggregate enum/struct itself to be `Copy`. In code review, if the compile error is about moving the aggregate but the bound fields are `Copy`, don’t recommend making the whole enum/struct `Copy` (e.g., don’t suggest `#[derive(Copy)]` on the enum solely for this match); keeping it non-`Copy` may be intentional for future non-`Copy` variants.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/jsc/VirtualMachine.rs:2598-2599
Timestamp: 2026-05-30T00:05:21.707Z
Learning: When reviewing Rust in this repo, do not flag direct `.0` access on `bun_core::String` as an encapsulation / sealed-field violation if it is used to reach the public inner newtype field and then call a public inner method (e.g., `bun_core::String::ascii(msg).0.mark_global()`). This usage is intended to preserve the prior Zig pattern (e.g., `...init(...).mark_global()`). Prefer considering `message.mark_global()`-style alternatives as style-only; only raise an issue if there is an actual API/compilation/semantic problem (e.g., method not public or code won’t compile / behavior changes), not merely because `.0` was accessed.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_fs.rs:6118-6119
Timestamp: 2026-05-30T00:05:31.762Z
Learning: When reviewing migration-only diffs that change implementation details like string types or API renames (e.g., ZigString -> BunString), do not treat pre-existing OOM handling patterns such as `.expect("oom")` as PR-introduced issues. Confirm in the base branch (before the change) that the same `.expect("oom")` usage pattern already existed; only flag it if the PR actually introduces or modifies the OOM-handling code path, not just the surrounding string/type/API migration.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_process.rs:398-398
Timestamp: 2026-05-30T00:05:32.312Z
Learning: In this codebase (oven-sh/bun), during ZigString → BunString migration reviews, treat `BunString::ascii(bytes)` **without** a subsequent `.with_encoding(...)` as byte-level equivalent to the old `ZigString::init(bytes)` **when the original Zig code had no encoding marking**. Therefore, do not flag `BunString::ascii(...)` (no `.with_encoding`) as a new encoding regression in that migration context. If the new code uses `.with_encoding(...)`, then encoding-related regression checks may still apply.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/server/server_body.rs:2002-2006
Timestamp: 2026-05-30T00:05:28.628Z
Learning: In this repo, avoid raising an issue just because code accesses the intentionally public inner tuple field of `bun_core::String` via `.0` (e.g., `some_bun_string.0.mark_utf8()`). Since `.0` is public and `bun_alloc::String::mark_utf8` is a public method, this pattern may be used to preserve base behavior during BunString/StringView migration. Only flag cases when there’s a separate, real problem (e.g., incorrect logic, misuse of the API), not merely the presence of `.0` field access.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/webcore/Blob.rs:6941-6943
Timestamp: 2026-05-30T00:05:38.722Z
Learning: When reviewing this repo’s Rust code for the (BunString) “compile-breaking sealed-internals” concern, do not flag tuple-field access like `bun_string.0.mark_global()` / `bun_string.0.mark_utf8()` as sealed-internals—provided that the tuple field `.0` is publicly visible and the invoked `mark_*` methods are also public. In that case, treat it as a style preference at most. Only raise/retain the sealed-internals issue if, for the codebase version you’re reviewing, the tuple field `.0` is not public (or the target `mark_*` methods are not public), making the access compile-breaking.
Comment on lines
1507
to
1520
| pub fn new_oklab(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | ||
| // PORT NOTE: Zig had `LABColor{ .lab = OKLAB.new(...) }` which looks like a bug; | ||
| // Zig had `LABColor{ .lab = OKLAB.new(...) }` which looks like a bug; | ||
| // mirrored as Lab variant for behavioral parity. | ||
| LABColor::Lab(LAB { l, a, b, alpha }) | ||
| } | ||
|
|
||
| pub fn new_lch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | ||
| // PORT NOTE: Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | ||
| // Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | ||
| LABColor::Lab(LAB { l, a, b, alpha }) | ||
| } | ||
|
|
||
| pub fn new_oklch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | ||
| // PORT NOTE: Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | ||
| // Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | ||
| LABColor::Lab(LAB { l, a, b, alpha }) |
Contributor
There was a problem hiding this comment.
Return the correct LABColor variant from these constructors.
These helpers currently all construct LABColor::Lab, so new_oklab, new_lch, and new_oklch erase the authored colorspace immediately and will serialize/behave as the wrong variant.
Suggested fix
pub fn new_oklab(l: f32, a: f32, b: f32, alpha: f32) -> LABColor {
- LABColor::Lab(LAB { l, a, b, alpha })
+ LABColor::Oklab(OKLAB { l, a, b, alpha })
}
pub fn new_lch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor {
- LABColor::Lab(LAB { l, a, b, alpha })
+ LABColor::Lch(LCH { l, c: a, h: b, alpha })
}
pub fn new_oklch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor {
- LABColor::Lab(LAB { l, a, b, alpha })
+ LABColor::Oklch(OKLCH { l, c: a, h: b, alpha })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn new_oklab(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | |
| // PORT NOTE: Zig had `LABColor{ .lab = OKLAB.new(...) }` which looks like a bug; | |
| // Zig had `LABColor{ .lab = OKLAB.new(...) }` which looks like a bug; | |
| // mirrored as Lab variant for behavioral parity. | |
| LABColor::Lab(LAB { l, a, b, alpha }) | |
| } | |
| pub fn new_lch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | |
| // PORT NOTE: Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | |
| // Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | |
| LABColor::Lab(LAB { l, a, b, alpha }) | |
| } | |
| pub fn new_oklch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | |
| // PORT NOTE: Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | |
| // Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | |
| LABColor::Lab(LAB { l, a, b, alpha }) | |
| pub fn new_oklab(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | |
| // Zig had `LABColor{ .lab = OKLAB.new(...) }` which looks like a bug; | |
| // mirrored as Lab variant for behavioral parity. | |
| LABColor::Oklab(OKLAB { l, a, b, alpha }) | |
| } | |
| pub fn new_lch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | |
| // Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | |
| LABColor::Lch(LCH { l, c: a, h: b, alpha }) | |
| } | |
| pub fn new_oklch(l: f32, a: f32, b: f32, alpha: f32) -> LABColor { | |
| // Zig had `LABColor{ .lab = LCH.new(...) }` (likely bug); mirrored. | |
| LABColor::Oklch(OKLCH { l, c: a, h: b, alpha }) | |
| } |
🤖 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/css/values/color.rs` around lines 1507 - 1520, Construct and return the
specific enum variants instead of always using LABColor::Lab: update new_oklab
to return LABColor::OkLab(OKLAB { l, a, b, alpha }), update new_lch to return
LABColor::Lch(LCH { l, a, b, alpha }), and update new_oklch to return
LABColor::OkLch(OKLCH { l, a, b, alpha }) so each constructor preserves its
intended colorspace variant (modify the variant/struct names to match the exact
identifiers used in the file if they differ).
Collaborator
Author
There was a problem hiding this comment.
Deliberate parity: the reference newLCH/newOKLCH return .lab as well (css/values/color.zig:1453-1463), and the Rust code carries an explicit '(sic) — kept for behavioral compatibility' note. Changing the variant would diverge from the shipped behavior; if it's a real bug it should be fixed in its own change with CSS color tests.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, you’re right — I missed that this is intentional parity with the Zig reference and that the Rust comments already document the (sic) behavior. I’ll treat this as a false positive for this PR; changing these constructors would be a behavior change and belongs in a separate, test-backed CSS color fix if pursued.
🐇💭
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/css/values/color.rs:1494-1507
Timestamp: 2026-06-05T01:50:05.381Z
Learning: In oven-sh/bun `src/css/values/color.rs`, do not flag `LABColor::new_oklab`, `LABColor::new_lch`, or `LABColor::new_oklch` returning `LABColor::Lab(...)` as a bug in port/TODO-audit reviews. This is deliberate behavioral parity with the Zig reference `css/values/color.zig` where `newLCH`/`newOKLCH` also return `.lab`, and the Rust code documents it with `(sic) — kept for behavioral compatibility`. Any semantic correction should be a separate change with CSS color tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to src/**/*.zig : `.zig` files alongside `.rs` files are original Zig implementations kept only as porting references - new code goes in `.rs`, never add new behavior to `.zig` files
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.rs : Platform-specific code: compile across all targets (linux/macos/windows × x64/aarch64) by running `bun run rust:check-all` when making platform-specific changes
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.
Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31175
File: src/valkey/valkey_protocol.rs:407-408
Timestamp: 2026-05-22T01:23:05.156Z
Learning: In oven-sh/bun Rust crates configured with `edition = "2024"`, `size_of::<T>()` is available via the Rust 2024 prelude. When reviewing, do not flag `size_of::<T>()` calls as missing `use core::mem::size_of` or `use std::mem::size_of` imports in these crates.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31338
File: src/runtime/shell/shell_body.rs:923-926
Timestamp: 2026-05-24T10:04:48.082Z
Learning: When reviewing oven-sh/bun code, do NOT treat calls to `JSValue::get_own_truthy` as a “missing JS truthiness” check. Despite the misleading name, `get_own_truthy` only performs an own-property slot lookup (via `get_own`, no prototype walk) and filters out only `undefined` (internally: `!prop.is_undefined()`). Values like `""`, `false`, `0`, and `null` are allowed and should return `Some(...)`. So reviews should not flag uses for not excluding other falsy values; the correct semantic gate to expect is only the `undefined` guard.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31335
File: src/install/PackageManager/PackageManagerDirectories.rs:842-848
Timestamp: 2026-05-24T10:58:13.503Z
Learning: When reviewing Rust code in this repo that indexes/slices a `PathBuffer` (which derefs to a `&[u8]` in `src/bun_core/util.rs`), treat slice indexing like `buf[..n]`, `buf[n..len]`, or `buf[len] = 0` as bounds-checked by Rust. Rust’s slice/indexing machinery will panic on out-of-bounds/overflow rather than write past the buffer. Therefore, do not flag these operations as potential out-of-bounds/unsafe writes, and do not recommend adding `debug_assert!` bounds checks that merely duplicate the existing Rust slice bounds checks (the checks are already enforced at indexing).
Learnt from: robobun
Repo: oven-sh/bun PR: 31579
File: src/sys/lib.rs:9225-9231
Timestamp: 2026-05-29T19:17:51.017Z
Learning: In this repo’s Rust code, don’t treat pattern matching on a borrowed enum/struct field as an incorrect “move-out-of-&” error when the pattern only binds payload fields that are `Copy` (e.g., matching `WriteFileData::Buffer { buffer }` where `buffer: &[u8]`, or `PathOrFileDescriptor::Path(bytes)` / `Fd(fd)` where `bytes: &[u8]` and `Fd` is `Copy`). Rust allows copying `Copy` payloads out of a borrowed aggregate without requiring the aggregate enum/struct itself to be `Copy`. In code review, if the compile error is about moving the aggregate but the bound fields are `Copy`, don’t recommend making the whole enum/struct `Copy` (e.g., don’t suggest `#[derive(Copy)]` on the enum solely for this match); keeping it non-`Copy` may be intentional for future non-`Copy` variants.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/jsc/VirtualMachine.rs:2598-2599
Timestamp: 2026-05-30T00:05:21.707Z
Learning: When reviewing Rust in this repo, do not flag direct `.0` access on `bun_core::String` as an encapsulation / sealed-field violation if it is used to reach the public inner newtype field and then call a public inner method (e.g., `bun_core::String::ascii(msg).0.mark_global()`). This usage is intended to preserve the prior Zig pattern (e.g., `...init(...).mark_global()`). Prefer considering `message.mark_global()`-style alternatives as style-only; only raise an issue if there is an actual API/compilation/semantic problem (e.g., method not public or code won’t compile / behavior changes), not merely because `.0` was accessed.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_fs.rs:6118-6119
Timestamp: 2026-05-30T00:05:31.762Z
Learning: When reviewing migration-only diffs that change implementation details like string types or API renames (e.g., ZigString -> BunString), do not treat pre-existing OOM handling patterns such as `.expect("oom")` as PR-introduced issues. Confirm in the base branch (before the change) that the same `.expect("oom")` usage pattern already existed; only flag it if the PR actually introduces or modifies the OOM-handling code path, not just the surrounding string/type/API migration.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_process.rs:398-398
Timestamp: 2026-05-30T00:05:32.312Z
Learning: In this codebase (oven-sh/bun), during ZigString → BunString migration reviews, treat `BunString::ascii(bytes)` **without** a subsequent `.with_encoding(...)` as byte-level equivalent to the old `ZigString::init(bytes)` **when the original Zig code had no encoding marking**. Therefore, do not flag `BunString::ascii(...)` (no `.with_encoding`) as a new encoding regression in that migration context. If the new code uses `.with_encoding(...)`, then encoding-related regression checks may still apply.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/server/server_body.rs:2002-2006
Timestamp: 2026-05-30T00:05:28.628Z
Learning: In this repo, avoid raising an issue just because code accesses the intentionally public inner tuple field of `bun_core::String` via `.0` (e.g., `some_bun_string.0.mark_utf8()`). Since `.0` is public and `bun_alloc::String::mark_utf8` is a public method, this pattern may be used to preserve base behavior during BunString/StringView migration. Only flag cases when there’s a separate, real problem (e.g., incorrect logic, misuse of the API), not merely the presence of `.0` field access.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/webcore/Blob.rs:6941-6943
Timestamp: 2026-05-30T00:05:38.722Z
Learning: When reviewing this repo’s Rust code for the (BunString) “compile-breaking sealed-internals” concern, do not flag tuple-field access like `bun_string.0.mark_global()` / `bun_string.0.mark_utf8()` as sealed-internals—provided that the tuple field `.0` is publicly visible and the invoked `mark_*` methods are also public. In that case, treat it as a style preference at most. Only raise/retain the sealed-internals issue if, for the codebase version you’re reviewing, the tuple field `.0` is not public (or the target `mark_*` methods are not public), making the access compile-breaking.
Comment on lines
2497
to
2503
| ColorSpaceName::Xyz | ColorSpaceName::XyzD65 => { | ||
| first_color.interpolate::<XYZd65>(p1, &second_color, p2, hue_method) | ||
| } | ||
| // PORT NOTE: Zig used XYZd65 for xyz-d50 too (likely bug); mirrored for parity. | ||
| // Zig used XYZd65 for xyz-d50 too (likely bug); mirrored for parity. | ||
| ColorSpaceName::XyzD50 => { | ||
| first_color.interpolate::<XYZd65>(p1, &second_color, p2, hue_method) | ||
| } |
Contributor
There was a problem hiding this comment.
Mix xyz-d50 colors in XYZd50, not XYZd65.
ColorSpaceName::XyzD50 currently dispatches to interpolate::<XYZd65>, so color-mix(in xyz-d50, …) runs in the wrong colorspace.
Suggested fix
ColorSpaceName::Xyz | ColorSpaceName::XyzD65 => {
first_color.interpolate::<XYZd65>(p1, &second_color, p2, hue_method)
}
ColorSpaceName::XyzD50 => {
- first_color.interpolate::<XYZd65>(p1, &second_color, p2, hue_method)
+ first_color.interpolate::<XYZd50>(p1, &second_color, p2, hue_method)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ColorSpaceName::Xyz | ColorSpaceName::XyzD65 => { | |
| first_color.interpolate::<XYZd65>(p1, &second_color, p2, hue_method) | |
| } | |
| // PORT NOTE: Zig used XYZd65 for xyz-d50 too (likely bug); mirrored for parity. | |
| // Zig used XYZd65 for xyz-d50 too (likely bug); mirrored for parity. | |
| ColorSpaceName::XyzD50 => { | |
| first_color.interpolate::<XYZd65>(p1, &second_color, p2, hue_method) | |
| } | |
| ColorSpaceName::Xyz | ColorSpaceName::XyzD65 => { | |
| first_color.interpolate::<XYZd65>(p1, &second_color, p2, hue_method) | |
| } | |
| // Zig used XYZd65 for xyz-d50 too (likely bug); mirrored for parity. | |
| ColorSpaceName::XyzD50 => { | |
| first_color.interpolate::<XYZd50>(p1, &second_color, p2, hue_method) | |
| } |
🤖 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/css/values/color.rs` around lines 2497 - 2503, The match arm for
ColorSpaceName::XyzD50 dispatches to interpolate::<XYZd65>, causing xyz-d50
mixes to run in the wrong colorspace; update that arm to call
first_color.interpolate::<XYZd50>(p1, &second_color, p2, hue_method) (replace
XYZd65 with XYZd50) so ColorSpaceName::XyzD50 uses the correct XYZd50
implementation.
Collaborator
Author
There was a problem hiding this comment.
Deliberate parity: the reference also interpolates xyz-d50 as XYZd65 (css/values/color.zig:3115: ."xyz-d50" => interpolate XYZd65), and the code is annotated '(sic) — kept for behavioral compatibility'. A correctness fix should be a separate change with tests rather than a silent behavioral diff inside this PR.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, agreed — thanks for the clarification. I’ll withdraw this as a finding for this PR. Since the Rust port is intentionally preserving the Zig behavior and the code is annotated as parity-preserving, changing xyz-d50 to interpolate through XYZd50 would be a separate behavioral fix and should come with targeted tests.
I’ll remember not to flag this parity case in future reviews.
🐇💭
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/css/values/color.rs:2481-2487
Timestamp: 2026-06-05T01:50:08.214Z
Learning: In oven-sh/bun `src/css/values/color.rs`, `parse_color_mix` intentionally maps `ColorSpaceName::XyzD50` to `first_color.interpolate::<XYZd65>(...)` for parity with the Zig reference (`css/values/color.zig`, where `"xyz-d50"` also interpolates as `XYZd65`). Do not flag this as a bug in TODO/PORT-note audit or parity-preserving PRs; any correctness change to interpolate `xyz-d50` through `XYZd50` should be treated as a separate behavioral change with tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.rs : Platform-specific code: compile across all targets (linux/macos/windows × x64/aarch64) by running `bun run rust:check-all` when making platform-specific changes
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.
Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31175
File: src/valkey/valkey_protocol.rs:407-408
Timestamp: 2026-05-22T01:23:05.156Z
Learning: In oven-sh/bun Rust crates configured with `edition = "2024"`, `size_of::<T>()` is available via the Rust 2024 prelude. When reviewing, do not flag `size_of::<T>()` calls as missing `use core::mem::size_of` or `use std::mem::size_of` imports in these crates.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31338
File: src/runtime/shell/shell_body.rs:923-926
Timestamp: 2026-05-24T10:04:48.082Z
Learning: When reviewing oven-sh/bun code, do NOT treat calls to `JSValue::get_own_truthy` as a “missing JS truthiness” check. Despite the misleading name, `get_own_truthy` only performs an own-property slot lookup (via `get_own`, no prototype walk) and filters out only `undefined` (internally: `!prop.is_undefined()`). Values like `""`, `false`, `0`, and `null` are allowed and should return `Some(...)`. So reviews should not flag uses for not excluding other falsy values; the correct semantic gate to expect is only the `undefined` guard.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31335
File: src/install/PackageManager/PackageManagerDirectories.rs:842-848
Timestamp: 2026-05-24T10:58:13.503Z
Learning: When reviewing Rust code in this repo that indexes/slices a `PathBuffer` (which derefs to a `&[u8]` in `src/bun_core/util.rs`), treat slice indexing like `buf[..n]`, `buf[n..len]`, or `buf[len] = 0` as bounds-checked by Rust. Rust’s slice/indexing machinery will panic on out-of-bounds/overflow rather than write past the buffer. Therefore, do not flag these operations as potential out-of-bounds/unsafe writes, and do not recommend adding `debug_assert!` bounds checks that merely duplicate the existing Rust slice bounds checks (the checks are already enforced at indexing).
Learnt from: robobun
Repo: oven-sh/bun PR: 31579
File: src/sys/lib.rs:9225-9231
Timestamp: 2026-05-29T19:17:51.017Z
Learning: In this repo’s Rust code, don’t treat pattern matching on a borrowed enum/struct field as an incorrect “move-out-of-&” error when the pattern only binds payload fields that are `Copy` (e.g., matching `WriteFileData::Buffer { buffer }` where `buffer: &[u8]`, or `PathOrFileDescriptor::Path(bytes)` / `Fd(fd)` where `bytes: &[u8]` and `Fd` is `Copy`). Rust allows copying `Copy` payloads out of a borrowed aggregate without requiring the aggregate enum/struct itself to be `Copy`. In code review, if the compile error is about moving the aggregate but the bound fields are `Copy`, don’t recommend making the whole enum/struct `Copy` (e.g., don’t suggest `#[derive(Copy)]` on the enum solely for this match); keeping it non-`Copy` may be intentional for future non-`Copy` variants.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/jsc/VirtualMachine.rs:2598-2599
Timestamp: 2026-05-30T00:05:21.707Z
Learning: When reviewing Rust in this repo, do not flag direct `.0` access on `bun_core::String` as an encapsulation / sealed-field violation if it is used to reach the public inner newtype field and then call a public inner method (e.g., `bun_core::String::ascii(msg).0.mark_global()`). This usage is intended to preserve the prior Zig pattern (e.g., `...init(...).mark_global()`). Prefer considering `message.mark_global()`-style alternatives as style-only; only raise an issue if there is an actual API/compilation/semantic problem (e.g., method not public or code won’t compile / behavior changes), not merely because `.0` was accessed.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_fs.rs:6118-6119
Timestamp: 2026-05-30T00:05:31.762Z
Learning: When reviewing migration-only diffs that change implementation details like string types or API renames (e.g., ZigString -> BunString), do not treat pre-existing OOM handling patterns such as `.expect("oom")` as PR-introduced issues. Confirm in the base branch (before the change) that the same `.expect("oom")` usage pattern already existed; only flag it if the PR actually introduces or modifies the OOM-handling code path, not just the surrounding string/type/API migration.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_process.rs:398-398
Timestamp: 2026-05-30T00:05:32.312Z
Learning: In this codebase (oven-sh/bun), during ZigString → BunString migration reviews, treat `BunString::ascii(bytes)` **without** a subsequent `.with_encoding(...)` as byte-level equivalent to the old `ZigString::init(bytes)` **when the original Zig code had no encoding marking**. Therefore, do not flag `BunString::ascii(...)` (no `.with_encoding`) as a new encoding regression in that migration context. If the new code uses `.with_encoding(...)`, then encoding-related regression checks may still apply.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/server/server_body.rs:2002-2006
Timestamp: 2026-05-30T00:05:28.628Z
Learning: In this repo, avoid raising an issue just because code accesses the intentionally public inner tuple field of `bun_core::String` via `.0` (e.g., `some_bun_string.0.mark_utf8()`). Since `.0` is public and `bun_alloc::String::mark_utf8` is a public method, this pattern may be used to preserve base behavior during BunString/StringView migration. Only flag cases when there’s a separate, real problem (e.g., incorrect logic, misuse of the API), not merely the presence of `.0` field access.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/webcore/Blob.rs:6941-6943
Timestamp: 2026-05-30T00:05:38.722Z
Learning: When reviewing this repo’s Rust code for the (BunString) “compile-breaking sealed-internals” concern, do not flag tuple-field access like `bun_string.0.mark_global()` / `bun_string.0.mark_utf8()` as sealed-internals—provided that the tuple field `.0` is publicly visible and the invoked `mark_*` methods are also public. In that case, treat it as a style preference at most. Only raise/retain the sealed-internals issue if, for the codebase version you’re reviewing, the tuple field `.0` is not public (or the target `mark_*` methods are not public), making the access compile-breaking.
The codebase has moved from a port to a standalone Rust implementation; comments citing the prior Zig implementation (file/line cross-references, std.* stdlib mentions, quoted Zig syntax, divergence notes) no longer aid maintenance. Delete them, keeping SAFETY justifications rewritten in terms of the Rust code's own invariants. References to current identifiers (ZigString etc.) are unchanged.
NewServer::init moves the config into the server, so the hot-map registration that runs after init read an empty id from the stale local and registered every server under the same key, panicking on the second Bun.serve in --hot processes. Read allow_hot and id from the server's own config instead. This path was unreachable until hot_map() gained its lazy-init accessor. Also update parsePackedFeaturesList() for the new define_features! shape (internal @storage recursion and core = IDENT aliases).
Contributor
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/bun_core/util.rs (1)
4871-4888:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
posix_spawnp/execvpPATH lookup behavior in the Linux/FreeBSD fallback (src/bun_core/util.rs, ~4871–4888).The code falls back to
getenv_z(...).unwrap_or(b"")and thenwhich()skips empty PATH components (if dir.is_empty() { continue; }). That means:
- Unset
PATHbecomes “no search directories” → alwaysENOENT(instead of a typical system default search path used by many libc implementations).PATHvalues like:/binorfoo::bardo not probe the current directory (empty components are not treated like.).Because the implementation claims a
posix_spawnp-like contract, update the behavior to match commonexecvpsemantics: treat empty PATH components as the current directory, and handle unsetPATHwith the platform’s default search path.🤖 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/bun_core/util.rs` around lines 4871 - 4888, The PATH lookup in the Linux/FreeBSD posix_spawn_bun fallback currently calls getenv_z(...).unwrap_or(b"") and relies on which(...), but that causes empty PATH (unset) or empty components to be treated as “no search” — break the posix_spawnp/execvp contract. Change the logic so that when getenv_z returns None (unset PATH) you substitute the platform’s default search path (e.g. the same default used by execvp on the platform, typically "/bin:/usr/bin"), and ensure which(...) treats empty path components as the current directory (i.e. interpret "" as ".") instead of skipping them; update the code around argv[0], getenv_z(...) and the call to which(...) (and/or which's handling) to implement these two behaviors so bare program names are resolved like execvp/posix_spawnp.
♻️ Duplicate comments (2)
src/brotli/lib.rs (1)
419-433:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
flush()implementation should usefull_flush_opto drain pending encoder state.As noted in a previous review, calling
self.compressor.write(b"", false)routes through the stream's configuredflush_op, which may be a no-op forPROCESS-mode streams (per the inline comment). To honor thebun_io::Write::flush()contract — ensuring all buffered compressed bytes reach the sink before returning — the encoder should run withfull_flush_opinstead.🔧 Proposed fix
fn flush(&mut self) -> Result<(), Error> { - let out = self.compressor.write(b"", false)?; + let op = self.compressor.full_flush_op; + let result = BrotliEncoder::compress_stream( + unsafe { &mut *self.compressor.brotli }, + op, + b"" + ); + if !result.success { + self.compressor.state = CompressionState::Error; + return Err(err!("BrotliCompressionError")); + } + let out = result.output; self.input_writer.write_all(out)?; self.input_writer.flush() }🤖 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/brotli/lib.rs` around lines 419 - 433, The flush() implementation on BrotliWriter currently calls self.compressor.write(b"", false) which uses the stream's configured flush_op and can be a no-op; change it to invoke the compressor with the full_flush_op (e.g. self.compressor.write(b"", full_flush_op())) so the encoder emits all pending compressed bytes before returning, then keep the existing calls to self.input_writer.write_all(out)? and self.input_writer.flush() to forward and flush the sink; update the call site in the BrotliWriter::flush method to use full_flush_op and handle the returned output/errors the same way.src/analytics/lib.rs (1)
117-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the lint allow from the aliasing
pub usearm.Line 119 still applies
#[allow(non_upper_case_globals)]to auseitem, so Clippy reports it as a useless attribute and the current pipeline is failing on this line. The allow should stay only on the realpub staticarm.Suggested fix
(`@storage` $(#[$doc:meta])* $ident:ident, $core:ident) => { $(#[$doc])* - #[allow(non_upper_case_globals)] pub use ::bun_core::Global::features::$core as $ident; };🤖 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/analytics/lib.rs` around lines 117 - 120, In the macro arm matching "(`@storage` $(#[$doc:meta])* $ident:ident, $core:ident) => { ... }" remove the #[allow(non_upper_case_globals)] attribute that sits immediately before the `pub use ::bun_core::Global::features::$core as $ident;` alias, leaving the doc attribute `$(#[$doc])*` intact; ensure the #[allow(non_upper_case_globals)] remains only on the other macro arm that expands to a real `pub static` so Clippy no longer flags the `use` alias as having a useless attribute.
🤖 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/boringssl_sys/boringssl.rs`:
- Around line 442-443: The sk_GENERAL_NAME_free_func is incorrectly typed to
accept a stack pointer; change its signature to unsafe extern "C" fn(*mut
GENERAL_NAME) so the per-element destructor is used, update the trampoline
function sk_GENERAL_NAME_call_free_func to cast and forward a single *mut
GENERAL_NAME to the callback (not *mut struct_stack_st_GENERAL_NAME), and update
the call site (sk_GENERAL_NAME_pop_free and any invocation like
boring::sk_GENERAL_NAME_pop_free(n, boring::sk_GENERAL_NAME_free)) to pass the
per-element free function (sk_GENERAL_NAME_free should be an element destructor
taking *mut GENERAL_NAME) instead of a stack deallocator to avoid passing a
stack pointer into the element-free trampoline.
In `@src/bun_core/util.rs`:
- Around line 2150-2151: ThreadLock's use of Cell<StoredTrace> for locked_at
races with the atomic owning_thread because lock() publishes ownership
(owning_thread.swap) before writing locked_at.set(...) and unlock() releases
ownership before clearing locked_at.set(EMPTY), so the atomic does not
synchronize Cell access; fix by making locked_at a properly synchronized
atomic-backed storage (e.g., change Cell<StoredTrace> to an atomic
representation like AtomicUsize/AtomicPtr or a Mutex/AtomicU64 depending on
StoredTrace size) or by reordering and inserting the proper atomic fences so
that locked_at is fully written before publishing owning_thread in lock() and
cleared only after owning_thread is cleared in unlock(); update ThreadLock,
lock(), unlock(), and any StoredTrace conversions accordingly to ensure
atomic/ordered reads and writes.
---
Outside diff comments:
In `@src/bun_core/util.rs`:
- Around line 4871-4888: The PATH lookup in the Linux/FreeBSD posix_spawn_bun
fallback currently calls getenv_z(...).unwrap_or(b"") and relies on which(...),
but that causes empty PATH (unset) or empty components to be treated as “no
search” — break the posix_spawnp/execvp contract. Change the logic so that when
getenv_z returns None (unset PATH) you substitute the platform’s default search
path (e.g. the same default used by execvp on the platform, typically
"/bin:/usr/bin"), and ensure which(...) treats empty path components as the
current directory (i.e. interpret "" as ".") instead of skipping them; update
the code around argv[0], getenv_z(...) and the call to which(...) (and/or
which's handling) to implement these two behaviors so bare program names are
resolved like execvp/posix_spawnp.
---
Duplicate comments:
In `@src/analytics/lib.rs`:
- Around line 117-120: In the macro arm matching "(`@storage` $(#[$doc:meta])*
$ident:ident, $core:ident) => { ... }" remove the
#[allow(non_upper_case_globals)] attribute that sits immediately before the `pub
use ::bun_core::Global::features::$core as $ident;` alias, leaving the doc
attribute `$(#[$doc])*` intact; ensure the #[allow(non_upper_case_globals)]
remains only on the other macro arm that expands to a real `pub static` so
Clippy no longer flags the `use` alias as having a useless attribute.
In `@src/brotli/lib.rs`:
- Around line 419-433: The flush() implementation on BrotliWriter currently
calls self.compressor.write(b"", false) which uses the stream's configured
flush_op and can be a no-op; change it to invoke the compressor with the
full_flush_op (e.g. self.compressor.write(b"", full_flush_op())) so the encoder
emits all pending compressed bytes before returning, then keep the existing
calls to self.input_writer.write_all(out)? and self.input_writer.flush() to
forward and flush the sink; update the call site in the BrotliWriter::flush
method to use full_flush_op and handle the returned output/errors the same way.
🪄 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: a7939792-4ee1-467a-be2e-bbcb5224af5a
📒 Files selected for processing (297)
scripts/build/features-json.tssrc/analytics/lib.rssrc/analytics/schema.rssrc/api/lib.rssrc/ast/ast_memory_allocator.rssrc/ast/ast_result.rssrc/ast/b.rssrc/ast/base.rssrc/ast/binding.rssrc/ast/char_freq.rssrc/ast/e.rssrc/ast/expr.rssrc/ast/fold_string_addition.rssrc/ast/g.rssrc/ast/import_record.rssrc/ast/known_global.rssrc/ast/lexer_log.rssrc/ast/lexer_tables.rssrc/ast/lib.rssrc/ast/loader.rssrc/ast/new_store.rssrc/ast/nodes.rssrc/ast/op.rssrc/ast/runtime.rssrc/ast/s.rssrc/ast/scope.rssrc/ast/server_component_boundary.rssrc/ast/stmt.rssrc/ast/symbol.rssrc/ast/target.rssrc/ast/transpiler_cache.rssrc/ast/ts.rssrc/ast/use_directive.rssrc/ast_jsc/lib.rssrc/base64/lib.rssrc/boringssl/lib.rssrc/boringssl_sys/boringssl.rssrc/brotli/lib.rssrc/brotli_sys/brotli_c.rssrc/bun.js.rssrc/bun_alloc/BufferFallbackAllocator.rssrc/bun_alloc/MaxHeapAllocator.rssrc/bun_alloc/MimallocArena.rssrc/bun_alloc/NullableAllocator.rssrc/bun_alloc/ast_alloc.rssrc/bun_alloc/baby_vec.rssrc/bun_alloc/basic.rssrc/bun_alloc/fallback.rssrc/bun_alloc/fallback/z.rssrc/bun_alloc/hashbrown_bridge.rssrc/bun_alloc/heap_breakdown.rssrc/bun_alloc/lib.rssrc/bun_alloc/maybe_owned.rssrc/bun_alloc/memory.rssrc/bun_alloc/stack_fallback.rssrc/bun_bin/lib.rssrc/bun_bin/phase_c_exports.rssrc/bun_core/Global.rssrc/bun_core/Progress.rssrc/bun_core/atomic_cell.rssrc/bun_core/bounded_array.rssrc/bun_core/debug.rssrc/bun_core/deprecated.rssrc/bun_core/env.rssrc/bun_core/env_var.rssrc/bun_core/external_shared.rssrc/bun_core/feature_flags.rssrc/bun_core/fmt.rssrc/bun_core/heap.rssrc/bun_core/hint.rssrc/bun_core/lib.rssrc/bun_core/output.rssrc/bun_core/result.rssrc/bun_core/string/HashedString.rssrc/bun_core/string/MutableString.rssrc/bun_core/string/SmolStr.rssrc/bun_core/string/StringBuilder.rssrc/bun_core/string/StringJoiner.rssrc/bun_core/string/escapeRegExp.rssrc/bun_core/string/identifier.rssrc/bun_core/string/immutable.rssrc/bun_core/string/immutable/exact_size_matcher.rssrc/bun_core/string/immutable/unicode.rssrc/bun_core/string/mod.rssrc/bun_core/string/write.rssrc/bun_core/string/wtf.rssrc/bun_core/thread_id.rssrc/bun_core/tty.rssrc/bun_core/util.rssrc/bun_core/windows_sys.rssrc/bun_core/wtf.rssrc/bun_core_macros/lib.rssrc/bun_output_tags/lib.rssrc/bundler/AstBuilder.rssrc/bundler/BundleThread.rssrc/bundler/Chunk.rssrc/bundler/DeferredBatchTask.rssrc/bundler/Graph.rssrc/bundler/HTMLImportManifest.rssrc/bundler/HTMLScanner.rssrc/bundler/IndexStringMap.rssrc/bundler/LinkerContext.rssrc/bundler/LinkerGraph.rssrc/bundler/OutputFile.rssrc/bundler/ParseTask.rssrc/bundler/PathToSourceIndexMap.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/analyze_transpiled_module.rssrc/bundler/barrel_imports.rssrc/bundler/bundle_v2.rssrc/bundler/bundled_ast.rssrc/bundler/cache.rssrc/bundler/defines.rssrc/bundler/entry_points.rssrc/bundler/lib.rssrc/bundler/linker.rssrc/bundler/linker_context/MetafileBuilder.rssrc/bundler/linker_context/OutputFileListBuilder.rssrc/bundler/linker_context/StaticRouteVisitor.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/convertStmtsForChunkForDevServer.rssrc/bundler/linker_context/doStep5.rssrc/bundler/linker_context/findAllImportedPartsInJSOrder.rssrc/bundler/linker_context/findImportedCSSFilesInJSOrder.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForCssChunk.rssrc/bundler/linker_context/generateCompileResultForHtmlChunk.rssrc/bundler/linker_context/generateCompileResultForJSChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessHTMLChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/renameSymbolsInChunk.rssrc/bundler/linker_context/scanImportsAndExports.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/bundler_jsc/PluginRunner.rssrc/bundler_jsc/analyze_jsc.rssrc/bundler_jsc/lib.rssrc/bundler_jsc/options_jsc.rssrc/bundler_jsc/source_map_mode_jsc.rssrc/bunfig/arguments.rssrc/bunfig/bunfig.rssrc/bunfig/lib.rssrc/c-headers-for-zig.hsrc/cares_sys/c_ares.rssrc/clap/args.rssrc/clap/comptime.rssrc/clap/lib.rssrc/clap/streaming.rssrc/clap_macros/lib.rssrc/codegen/bindgen-lib-internal.tssrc/codegen/bindgen-lib.tssrc/codegen/bindgen.tssrc/codegen/bindgenv2/internal/base.tssrc/codegen/bindgenv2/internal/dictionary.tssrc/codegen/bindgenv2/internal/union.tssrc/codegen/class-definitions.tssrc/codegen/cppbind.tssrc/codegen/generate-classes.tssrc/codegen/generate-host-exports.tssrc/codegen/generate-js2native.tssrc/codegen/generate-jssink.tssrc/collections/StaticHashMap.rssrc/collections/array_hash_map.rssrc/collections/array_list.rssrc/collections/bit_set.rssrc/collections/comptime_string_map.rssrc/collections/hive_array.rssrc/collections/identity_context.rssrc/collections/lib.rssrc/collections/linear_fifo.rssrc/collections/multi_array_list.rssrc/collections/pool.rssrc/collections/string_map.rssrc/collections/vec_ext.rssrc/collections/zig_hash_map.rssrc/crash_handler/CPUFeatures.rssrc/crash_handler/handle_oom.rssrc/crash_handler/lib.rssrc/csrf/lib.rssrc/css/compat.rssrc/css/context.rssrc/css/css_modules.rssrc/css/css_parser.rssrc/css/declaration.rssrc/css/dependencies.rssrc/css/error.rssrc/css/generics.rssrc/css/lib.rssrc/css/logical.rssrc/css/media_query.rssrc/css/prefixes.rssrc/css/printer.rssrc/css/properties/align.rssrc/css/properties/animation.rssrc/css/properties/background.rssrc/css/properties/border.rssrc/css/properties/border_image.rssrc/css/properties/border_radius.rssrc/css/properties/box_shadow.rssrc/css/properties/css_modules.rssrc/css/properties/custom.rssrc/css/properties/display.rssrc/css/properties/effects.rssrc/css/properties/flex.rssrc/css/properties/font.rssrc/css/properties/generate_properties.tssrc/css/properties/grid.rssrc/css/properties/list.rssrc/css/properties/margin_padding.rssrc/css/properties/masking.rssrc/css/properties/mod.rssrc/css/properties/outline.rssrc/css/properties/overflow.rssrc/css/properties/position.rssrc/css/properties/prefix_handler.rssrc/css/properties/properties_generated.rssrc/css/properties/properties_impl.rssrc/css/properties/shape.rssrc/css/properties/size.rssrc/css/properties/svg.rssrc/css/properties/text.rssrc/css/properties/transform.rssrc/css/properties/transition.rssrc/css/properties/ui.rssrc/css/rules/container.rssrc/css/rules/counter_style.rssrc/css/rules/custom_media.rssrc/css/rules/document.rssrc/css/rules/font_face.rssrc/css/rules/font_palette_values.rssrc/css/rules/import.rssrc/css/rules/keyframes.rssrc/css/rules/layer.rssrc/css/rules/media.rssrc/css/rules/mod.rssrc/css/rules/namespace.rssrc/css/rules/nesting.rssrc/css/rules/page.rssrc/css/rules/property.rssrc/css/rules/scope.rssrc/css/rules/starting_style.rssrc/css/rules/style.rssrc/css/rules/supports.rssrc/css/rules/tailwind.rssrc/css/rules/unknown.rssrc/css/rules/viewport.rssrc/css/selectors/builder.rssrc/css/selectors/mod.rssrc/css/selectors/parser.rssrc/css/selectors/selector.rssrc/css/small_list.rssrc/css/sourcemap.rssrc/css/targets.rssrc/css/values/alpha.rssrc/css/values/angle.rssrc/css/values/calc.rssrc/css/values/color.rssrc/css/values/color_generated.rssrc/css/values/css_string.rssrc/css/values/easing.rssrc/css/values/gradient.rssrc/css/values/ident.rssrc/css/values/image.rssrc/css/values/length.rssrc/css/values/mod.rssrc/css/values/number.rssrc/css/values/percentage.rssrc/css/values/position.rssrc/css/values/ratio.rssrc/css/values/rect.rssrc/css/values/resolution.rssrc/css/values/size.rssrc/css/values/syntax.rssrc/css/values/time.rssrc/css/values/url.rssrc/css_derive/lib.rssrc/css_jsc/color_js.rssrc/css_jsc/css_internals.rssrc/css_jsc/error_jsc.rssrc/dispatch/lib.rssrc/dns/lib.rssrc/dotenv/env_loader.rssrc/errno/darwin_errno.rssrc/errno/freebsd_errno.rssrc/errno/lib.rssrc/errno/linux_errno.rssrc/errno/windows_errno.rssrc/event_loop/AnyEventLoop.rs
💤 Files with no reviewable changes (6)
- src/bun_core/string/escapeRegExp.rs
- src/ast/s.rs
- src/bun_core/feature_flags.rs
- src/ast_jsc/lib.rs
- src/bun_alloc/basic.rs
- src/bun_alloc/memory.rs
Comment on lines
+442
to
443
| /// Per-stack free callback type used by `sk_GENERAL_NAME_pop_free`. | ||
| pub(crate) type sk_GENERAL_NAME_free_func = unsafe extern "C" fn(*mut struct_stack_st_GENERAL_NAME); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Local Rust bindings/callers =="
rg -n "sk_GENERAL_NAME_(free_func|call_free_func|pop_free|free)|GENERAL_NAME_free" \
src/boringssl_sys/boringssl.rs src/boringssl/lib.rs
echo
echo "== BoringSSL headers =="
fd -p 'x509v3.h' vendor/boringssl/include/openssl | while read -r f; do
echo "-- $f --"
rg -n "GENERAL_NAME|GENERAL_NAME_free|sk_.*GENERAL_NAME|STACK_OF\\(GENERAL_NAME\\)" "$f"
done
fd -p 'stack.h' vendor/boringssl/include/openssl | while read -r f; do
echo "-- $f --"
rg -n "sk_pop_free_ex|OPENSSL_sk_pop_free_ex|OPENSSL_sk_free_func|OPENSSL_sk_call_free_func" "$f"
doneRepository: oven-sh/bun
Length of output: 1687
Use an element destructor here, not a stack destructor.
sk_GENERAL_NAME_pop_free calls free_func once per GENERAL_NAME element, but sk_GENERAL_NAME_free_func/sk_GENERAL_NAME_free are typed as unsafe extern "C" fn(*mut struct_stack_st_GENERAL_NAME) and sk_GENERAL_NAME_call_free_func reinstates that stack type before invoking the callback. As a result, src/boringssl/lib.rs’s boring::sk_GENERAL_NAME_pop_free(n, boring::sk_GENERAL_NAME_free) routes a stack deallocator through the per-element free trampoline, risking heap corruption while parsing peer certificates.
Change the callback type to take *mut GENERAL_NAME and update the trampoline + call site to pass the per-element GENERAL_NAME free binding (not sk_GENERAL_NAME_free).
🤖 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/boringssl_sys/boringssl.rs` around lines 442 - 443, The
sk_GENERAL_NAME_free_func is incorrectly typed to accept a stack pointer; change
its signature to unsafe extern "C" fn(*mut GENERAL_NAME) so the per-element
destructor is used, update the trampoline function
sk_GENERAL_NAME_call_free_func to cast and forward a single *mut GENERAL_NAME to
the callback (not *mut struct_stack_st_GENERAL_NAME), and update the call site
(sk_GENERAL_NAME_pop_free and any invocation like
boring::sk_GENERAL_NAME_pop_free(n, boring::sk_GENERAL_NAME_free)) to pass the
per-element free function (sk_GENERAL_NAME_free should be an element destructor
taking *mut GENERAL_NAME) instead of a stack deallocator to avoid passing a
stack pointer into the element-free trampoline.
Collaborator
Author
There was a problem hiding this comment.
Good catch on the type confusion, but this is faithfully-ported pre-existing behavior, not introduced here: the reference does the identical thing (boringssl.zig:180,202 pass sk_GENERAL_NAME_free to sk_GENERAL_NAME_pop_free). Worth investigating separately — the correct per-element destructor per BoringSSL's DEFINE_STACK_OF conventions is GENERAL_NAME_free — but changing free semantics on the SAN path deserves its own PR with targeted TLS tests, not a drive-by inside this cleanup.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, understood — the Zig reference (boringssl.zig:180,202) has the same pattern, so this is a faithful port of pre-existing behavior, not a regression introduced here. The correct fix (GENERAL_NAME_free as the per-element destructor) would touch the SAN verification path and really does warrant dedicated TLS coverage before merging.
Would you like me to open a GitHub issue to track this? I can file it with the relevant context (the correct destructor, the affected call site in src/boringssl/lib.rs, and a pointer back to this PR) so it doesn't get lost.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to src/**/*.rs : Never let a pointer or slice outlive the memory it points into - audit slices of stack buffers, growable containers across calls, and temporary values
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.rs : Platform-specific code: compile across all targets (linux/macos/windows × x64/aarch64) by running `bun run rust:check-all` when making platform-specific changes
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.
Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31175
File: src/valkey/valkey_protocol.rs:407-408
Timestamp: 2026-05-22T01:23:05.156Z
Learning: In oven-sh/bun Rust crates configured with `edition = "2024"`, `size_of::<T>()` is available via the Rust 2024 prelude. When reviewing, do not flag `size_of::<T>()` calls as missing `use core::mem::size_of` or `use std::mem::size_of` imports in these crates.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31338
File: src/runtime/shell/shell_body.rs:923-926
Timestamp: 2026-05-24T10:04:48.082Z
Learning: When reviewing oven-sh/bun code, do NOT treat calls to `JSValue::get_own_truthy` as a “missing JS truthiness” check. Despite the misleading name, `get_own_truthy` only performs an own-property slot lookup (via `get_own`, no prototype walk) and filters out only `undefined` (internally: `!prop.is_undefined()`). Values like `""`, `false`, `0`, and `null` are allowed and should return `Some(...)`. So reviews should not flag uses for not excluding other falsy values; the correct semantic gate to expect is only the `undefined` guard.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31335
File: src/install/PackageManager/PackageManagerDirectories.rs:842-848
Timestamp: 2026-05-24T10:58:13.503Z
Learning: When reviewing Rust code in this repo that indexes/slices a `PathBuffer` (which derefs to a `&[u8]` in `src/bun_core/util.rs`), treat slice indexing like `buf[..n]`, `buf[n..len]`, or `buf[len] = 0` as bounds-checked by Rust. Rust’s slice/indexing machinery will panic on out-of-bounds/overflow rather than write past the buffer. Therefore, do not flag these operations as potential out-of-bounds/unsafe writes, and do not recommend adding `debug_assert!` bounds checks that merely duplicate the existing Rust slice bounds checks (the checks are already enforced at indexing).
Learnt from: robobun
Repo: oven-sh/bun PR: 31579
File: src/sys/lib.rs:9225-9231
Timestamp: 2026-05-29T19:17:51.017Z
Learning: In this repo’s Rust code, don’t treat pattern matching on a borrowed enum/struct field as an incorrect “move-out-of-&” error when the pattern only binds payload fields that are `Copy` (e.g., matching `WriteFileData::Buffer { buffer }` where `buffer: &[u8]`, or `PathOrFileDescriptor::Path(bytes)` / `Fd(fd)` where `bytes: &[u8]` and `Fd` is `Copy`). Rust allows copying `Copy` payloads out of a borrowed aggregate without requiring the aggregate enum/struct itself to be `Copy`. In code review, if the compile error is about moving the aggregate but the bound fields are `Copy`, don’t recommend making the whole enum/struct `Copy` (e.g., don’t suggest `#[derive(Copy)]` on the enum solely for this match); keeping it non-`Copy` may be intentional for future non-`Copy` variants.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/jsc/VirtualMachine.rs:2598-2599
Timestamp: 2026-05-30T00:05:21.707Z
Learning: When reviewing Rust in this repo, do not flag direct `.0` access on `bun_core::String` as an encapsulation / sealed-field violation if it is used to reach the public inner newtype field and then call a public inner method (e.g., `bun_core::String::ascii(msg).0.mark_global()`). This usage is intended to preserve the prior Zig pattern (e.g., `...init(...).mark_global()`). Prefer considering `message.mark_global()`-style alternatives as style-only; only raise an issue if there is an actual API/compilation/semantic problem (e.g., method not public or code won’t compile / behavior changes), not merely because `.0` was accessed.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_fs.rs:6118-6119
Timestamp: 2026-05-30T00:05:31.762Z
Learning: When reviewing migration-only diffs that change implementation details like string types or API renames (e.g., ZigString -> BunString), do not treat pre-existing OOM handling patterns such as `.expect("oom")` as PR-introduced issues. Confirm in the base branch (before the change) that the same `.expect("oom")` usage pattern already existed; only flag it if the PR actually introduces or modifies the OOM-handling code path, not just the surrounding string/type/API migration.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_process.rs:398-398
Timestamp: 2026-05-30T00:05:32.312Z
Learning: In this codebase (oven-sh/bun), during ZigString → BunString migration reviews, treat `BunString::ascii(bytes)` **without** a subsequent `.with_encoding(...)` as byte-level equivalent to the old `ZigString::init(bytes)` **when the original Zig code had no encoding marking**. Therefore, do not flag `BunString::ascii(...)` (no `.with_encoding`) as a new encoding regression in that migration context. If the new code uses `.with_encoding(...)`, then encoding-related regression checks may still apply.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/server/server_body.rs:2002-2006
Timestamp: 2026-05-30T00:05:28.628Z
Learning: In this repo, avoid raising an issue just because code accesses the intentionally public inner tuple field of `bun_core::String` via `.0` (e.g., `some_bun_string.0.mark_utf8()`). Since `.0` is public and `bun_alloc::String::mark_utf8` is a public method, this pattern may be used to preserve base behavior during BunString/StringView migration. Only flag cases when there’s a separate, real problem (e.g., incorrect logic, misuse of the API), not merely the presence of `.0` field access.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/webcore/Blob.rs:6941-6943
Timestamp: 2026-05-30T00:05:38.722Z
Learning: When reviewing this repo’s Rust code for the (BunString) “compile-breaking sealed-internals” concern, do not flag tuple-field access like `bun_string.0.mark_global()` / `bun_string.0.mark_utf8()` as sealed-internals—provided that the tuple field `.0` is publicly visible and the invoked `mark_*` methods are also public. In that case, treat it as a style preference at most. Only raise/retain the sealed-internals issue if, for the codebase version you’re reviewing, the tuple field `.0` is not public (or the target `mark_*` methods are not public), making the access compile-breaking.
Comment on lines
+2150
to
2151
| // unsynchronized write to `locked_at` is fine — if two | ||
| // threads race here, the `owning_thread.swap` panic fires first. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate ThreadLock definition + relevant methods
rg -n "struct ThreadLock|impl ThreadLock|ThreadLock" src/bun_core/util.rs
# Show the exact lines around the referenced location
sed -n '2100,2300p' src/bun_core/util.rs
# Also show wider context near spawn_sync_inherit_impl for any comment coupling (not required, but helpful)
rg -n "spawn_sync_inherit_impl" src/bun_core/util.rs
sed -n '4700,5050p' src/bun_core/util.rsRepository: oven-sh/bun
Length of output: 22432
ThreadLock still races on locked_at in debug builds
ThreadLock has unsafe impl Sync while locked_at: Cell<StoredTrace> is non-atomic. In lock(), owning_thread.swap(...AcqRel) publishes ownership before locked_at.set(...), so a contender can observe the non-INVALID owner and read locked_at.get() while the owner is still writing it. In unlock(), owning_thread.store(...Release) happens before locked_at.set(EMPTY), so the next acquirer can overlap with the previous thread clearing locked_at. The atomic word does not synchronize Cell<locked_at> access as the comment claims.
🤖 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/bun_core/util.rs` around lines 2150 - 2151, ThreadLock's use of
Cell<StoredTrace> for locked_at races with the atomic owning_thread because
lock() publishes ownership (owning_thread.swap) before writing
locked_at.set(...) and unlock() releases ownership before clearing
locked_at.set(EMPTY), so the atomic does not synchronize Cell access; fix by
making locked_at a properly synchronized atomic-backed storage (e.g., change
Cell<StoredTrace> to an atomic representation like AtomicUsize/AtomicPtr or a
Mutex/AtomicU64 depending on StoredTrace size) or by reordering and inserting
the proper atomic fences so that locked_at is fully written before publishing
owning_thread in lock() and cleared only after owning_thread is cleared in
unlock(); update ThreadLock, lock(), unlock(), and any StoredTrace conversions
accordingly to ensure atomic/ordered reads and writes.
Collaborator
Author
There was a problem hiding this comment.
Not fixing here: this matches the Zig original (src/safety/ThreadLock.zig), which has no synchronization at all on either field — the Rust version is already strictly more ordered. ThreadLock is a debug-only single-threaded-access assertion; the racy locked_at read only happens on the already-panicking diagnostic path, where the trace is best-effort. Making it fully synchronized would add cost to every debug-build lock/unlock for a diagnostic that fires once before abort.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, understood — thanks for the clarification. I’ll withdraw this finding for this PR. Given ThreadLock is a debug-only single-threaded-access assertion and the locked_at trace is best-effort only on the already-panicking path, matching the Zig semantics here is intentional and I won’t ask for synchronization/reordering.
I’ll remember this for future reviews.
🐇💭
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/bun_core/util.rs:2150-2151
Timestamp: 2026-06-05T01:49:45.910Z
Learning: In oven-sh/bun `src/bun_core/util.rs`, do not flag `ThreadLock`'s debug-only `locked_at: Cell<StoredTrace>` access as needing full synchronization merely because it can race with `owning_thread`. This intentionally matches `src/safety/ThreadLock.zig` semantics, where the original has no synchronization on either field. `ThreadLock` is a debug-only single-threaded-access assertion; the `locked_at` trace is best-effort diagnostic data read only on the already-panicking path, and adding synchronization would add debug-build lock/unlock cost for a diagnostic that fires once before abort.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.rs : Platform-specific code: provide Windows equivalent for POSIX-only API additions in the same PR
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.rs : Platform-specific code: compile across all targets (linux/macos/windows × x64/aarch64) by running `bun run rust:check-all` when making platform-specific changes
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 30971
File: src/bun_core/thread_id.rs:124-125
Timestamp: 2026-05-19T08:28:57.054Z
Learning: In the oven-sh/bun `bun_core` crate, reviewers should treat bare `#[thread_local]` statics as intentional and correct (e.g., `static TLS_*: core::cell::Cell<ThreadId>`). Do not request replacing them with the `thread_local!` macro based solely on that they are not using `thread_local!`. This pattern is used to match `thread_local`/Zig-style semantics and avoid `LocalKey` initialization-state/destructor-registration overhead. Only flag/reconsider if this file/crate is not actually enabling the `thread_local` nightly feature (i.e., `#![feature(thread_local)]` is missing) or the code materially deviates from the established pattern.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.
Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31175
File: src/valkey/valkey_protocol.rs:407-408
Timestamp: 2026-05-22T01:23:05.156Z
Learning: In oven-sh/bun Rust crates configured with `edition = "2024"`, `size_of::<T>()` is available via the Rust 2024 prelude. When reviewing, do not flag `size_of::<T>()` calls as missing `use core::mem::size_of` or `use std::mem::size_of` imports in these crates.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31338
File: src/runtime/shell/shell_body.rs:923-926
Timestamp: 2026-05-24T10:04:48.082Z
Learning: When reviewing oven-sh/bun code, do NOT treat calls to `JSValue::get_own_truthy` as a “missing JS truthiness” check. Despite the misleading name, `get_own_truthy` only performs an own-property slot lookup (via `get_own`, no prototype walk) and filters out only `undefined` (internally: `!prop.is_undefined()`). Values like `""`, `false`, `0`, and `null` are allowed and should return `Some(...)`. So reviews should not flag uses for not excluding other falsy values; the correct semantic gate to expect is only the `undefined` guard.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31335
File: src/install/PackageManager/PackageManagerDirectories.rs:842-848
Timestamp: 2026-05-24T10:58:13.503Z
Learning: When reviewing Rust code in this repo that indexes/slices a `PathBuffer` (which derefs to a `&[u8]` in `src/bun_core/util.rs`), treat slice indexing like `buf[..n]`, `buf[n..len]`, or `buf[len] = 0` as bounds-checked by Rust. Rust’s slice/indexing machinery will panic on out-of-bounds/overflow rather than write past the buffer. Therefore, do not flag these operations as potential out-of-bounds/unsafe writes, and do not recommend adding `debug_assert!` bounds checks that merely duplicate the existing Rust slice bounds checks (the checks are already enforced at indexing).
Learnt from: robobun
Repo: oven-sh/bun PR: 31579
File: src/sys/lib.rs:9225-9231
Timestamp: 2026-05-29T19:17:51.017Z
Learning: In this repo’s Rust code, don’t treat pattern matching on a borrowed enum/struct field as an incorrect “move-out-of-&” error when the pattern only binds payload fields that are `Copy` (e.g., matching `WriteFileData::Buffer { buffer }` where `buffer: &[u8]`, or `PathOrFileDescriptor::Path(bytes)` / `Fd(fd)` where `bytes: &[u8]` and `Fd` is `Copy`). Rust allows copying `Copy` payloads out of a borrowed aggregate without requiring the aggregate enum/struct itself to be `Copy`. In code review, if the compile error is about moving the aggregate but the bound fields are `Copy`, don’t recommend making the whole enum/struct `Copy` (e.g., don’t suggest `#[derive(Copy)]` on the enum solely for this match); keeping it non-`Copy` may be intentional for future non-`Copy` variants.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/jsc/VirtualMachine.rs:2598-2599
Timestamp: 2026-05-30T00:05:21.707Z
Learning: When reviewing Rust in this repo, do not flag direct `.0` access on `bun_core::String` as an encapsulation / sealed-field violation if it is used to reach the public inner newtype field and then call a public inner method (e.g., `bun_core::String::ascii(msg).0.mark_global()`). This usage is intended to preserve the prior Zig pattern (e.g., `...init(...).mark_global()`). Prefer considering `message.mark_global()`-style alternatives as style-only; only raise an issue if there is an actual API/compilation/semantic problem (e.g., method not public or code won’t compile / behavior changes), not merely because `.0` was accessed.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_fs.rs:6118-6119
Timestamp: 2026-05-30T00:05:31.762Z
Learning: When reviewing migration-only diffs that change implementation details like string types or API renames (e.g., ZigString -> BunString), do not treat pre-existing OOM handling patterns such as `.expect("oom")` as PR-introduced issues. Confirm in the base branch (before the change) that the same `.expect("oom")` usage pattern already existed; only flag it if the PR actually introduces or modifies the OOM-handling code path, not just the surrounding string/type/API migration.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/node/node_process.rs:398-398
Timestamp: 2026-05-30T00:05:32.312Z
Learning: In this codebase (oven-sh/bun), during ZigString → BunString migration reviews, treat `BunString::ascii(bytes)` **without** a subsequent `.with_encoding(...)` as byte-level equivalent to the old `ZigString::init(bytes)` **when the original Zig code had no encoding marking**. Therefore, do not flag `BunString::ascii(...)` (no `.with_encoding`) as a new encoding regression in that migration context. If the new code uses `.with_encoding(...)`, then encoding-related regression checks may still apply.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/server/server_body.rs:2002-2006
Timestamp: 2026-05-30T00:05:28.628Z
Learning: In this repo, avoid raising an issue just because code accesses the intentionally public inner tuple field of `bun_core::String` via `.0` (e.g., `some_bun_string.0.mark_utf8()`). Since `.0` is public and `bun_alloc::String::mark_utf8` is a public method, this pattern may be used to preserve base behavior during BunString/StringView migration. Only flag cases when there’s a separate, real problem (e.g., incorrect logic, misuse of the API), not merely the presence of `.0` field access.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 31590
File: src/runtime/webcore/Blob.rs:6941-6943
Timestamp: 2026-05-30T00:05:38.722Z
Learning: When reviewing this repo’s Rust code for the (BunString) “compile-breaking sealed-internals” concern, do not flag tuple-field access like `bun_string.0.mark_global()` / `bun_string.0.mark_utf8()` as sealed-internals—provided that the tuple field `.0` is publicly visible and the invoked `mark_*` methods are also public. In that case, treat it as a style preference at most. Only raise/retain the sealed-internals issue if, for the codebase version you’re reviewing, the tuple field `.0` is not public (or the target `mark_*` methods are not public), making the access compile-breaking.
- define_features!: drop the allow(non_upper_case_globals) from the pub use re-export arm where the lint cannot apply (useless_attribute x47) - HardcodedModule: alias maps use bun_collections::HashMap per the disallowed-types policy (wyhash instead of SipHash) - shell_parser, js_printer: byte paths/text Display via bstr::BStr instead of String::from_utf8_lossy - shell_parser: drop_in_place(&raw mut ...) per borrow_as_ptr - Loader: derive Default instead of the manual impl - Yarn printer: JSON debug env check via bun_core::getenv_z - pnpm: remove empty is_prod() placeholder branch - WebSocketUpgradeClient: SslCtxOwned::into_raw via ManuallyDrop instead of mem::forget - PostgresSQLConnection: remove redundant AnyPostgresError::from - streams: return result instead of Ok(result?)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
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 (2)
src/bun_core/string/MutableString.rs (1)
516-521:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the UTF-16→UTF-8 size bound before calling the unsafe writer.
bytes.len() * 2is not a safe upper bound here. A single UTF-16 code unit can expand to 3 UTF-8 bytes (for example U+0800..=U+FFFF outside surrogate pairs), so this tail can be undersized beforecopy_utf16_into_utf8writes into it. The same assumption is also used in the buffered branch below, so non-ASCII BMP input can corrupt memory or drop data depending on howcopy_utf16_into_utf8handles a short output buffer.🔧 Suggested direction
- // SAFETY: copy_utf16_into_utf8 writes <= bytes.len*2; trimmed below. + // SAFETY: reserve for the worst-case UTF-8 expansion of arbitrary UTF-16. let tail = - unsafe { crate::vec::writable_slice(&mut self.context.list, bytes.len() * 2) }; + unsafe { crate::vec::writable_slice(&mut self.context.list, bytes.len() * 3) };You should make the same worst-case-capacity fix in the small-buffer path as well, or chunk/loop until the full UTF-16 input is encoded.
🤖 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/bun_core/string/MutableString.rs` around lines 516 - 521, The allocation for the UTF-16→UTF-8 write is using bytes.len() * 2 which is too small (UTF-16 units can expand up to 3 UTF-8 bytes); update the reservation before calling unsafe crate::vec::writable_slice so the tail length uses the correct worst-case bound (bytes.len() * 3) or otherwise ensure you loop/chunk until all UTF-16 input is encoded; apply the same fix to the buffered branch as well so functions like copy_utf16_into_utf8, writable_slice, and the variables tail/decoded use a safe output capacity to avoid buffer overruns or data loss.src/bun_core/string/immutable.rs (1)
2477-2483:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the length bug in
concat_if_needed's reuse check.This fast path compares and advances by
args.len()instead ofarg.len(). With multiple chunks it can falsely concludedestalready matches the requested concatenation and skip rebuilding stale contents.Suggested fix
- if args.len() > remain.len() { + if arg.len() > remain.len() { break 'brk true; } - if eql_long(&remain[0..args.len()], arg, true) { - remain = &remain[args.len()..]; + if eql_long(&remain[0..arg.len()], arg, true) { + remain = &remain[arg.len()..]; } else { break 'brk 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/bun_core/string/immutable.rs` around lines 2477 - 2483, In concat_if_needed, the reuse fast path incorrectly uses args.len() where it should use the current chunk's length arg.len(), causing incorrect matches across multiple chunks; update the three spots that reference args.len() (the length comparison, the slice passed to eql_long, and the slice advance remain = &remain[...]) to use arg.len() instead so the check and slicing operate on the current chunk's length.
🤖 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/bun_core/string/immutable.rs`:
- Around line 2559-2561: Replace the hardcoded false passed into the
QuoteEscapeFormat construction with the actual
QuoteEscapeFormatFlags::ascii_only value so ASCII-only escaping is honored;
locate where QuoteEscapeFormat (or its constructor/formatter call) is created
(the place currently passing the literal false) and change that argument to
flags.ascii_only, and run/update tests that assert ASCII-only escaping behavior
to confirm non-ASCII characters are escaped when requested.
In `@src/bun_core/string/immutable/unicode.rs`:
- Around line 904-909: The public macro to_utf16_literal! should not be a plain
alias to the ASCII-only w! macro because that allows non-ASCII literals to be
mis-encoded; replace the alias with one of two fixes: either (A) implement a
proper UTF‑16 literal macro that converts a string literal into a u16 array of
UTF‑16 code units (so to_utf16_literal!("$s") expands to the correct u16
sequence), or (B) stop exporting the alias and instead make to_utf16_literal!
validate and reject non-ASCII input at compile time (use a macro that checks
bytes and emits compile_error! for non-ASCII), and keep the ASCII-only w!
separate; update or remove #[macro_export] accordingly and modify any callsites
that intended ASCII behavior to use w! explicitly.
---
Outside diff comments:
In `@src/bun_core/string/immutable.rs`:
- Around line 2477-2483: In concat_if_needed, the reuse fast path incorrectly
uses args.len() where it should use the current chunk's length arg.len(),
causing incorrect matches across multiple chunks; update the three spots that
reference args.len() (the length comparison, the slice passed to eql_long, and
the slice advance remain = &remain[...]) to use arg.len() instead so the check
and slicing operate on the current chunk's length.
In `@src/bun_core/string/MutableString.rs`:
- Around line 516-521: The allocation for the UTF-16→UTF-8 write is using
bytes.len() * 2 which is too small (UTF-16 units can expand up to 3 UTF-8
bytes); update the reservation before calling unsafe crate::vec::writable_slice
so the tail length uses the correct worst-case bound (bytes.len() * 3) or
otherwise ensure you loop/chunk until all UTF-16 input is encoded; apply the
same fix to the buffered branch as well so functions like copy_utf16_into_utf8,
writable_slice, and the variables tail/decoded use a safe output capacity to
avoid buffer overruns or data loss.
🪄 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: 5b37b878-77ce-45fe-b5d8-0b5a663355d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (297)
scripts/build/features-json.tssrc/analytics/lib.rssrc/analytics/schema.rssrc/api/lib.rssrc/ast/ast_memory_allocator.rssrc/ast/ast_result.rssrc/ast/b.rssrc/ast/base.rssrc/ast/binding.rssrc/ast/char_freq.rssrc/ast/e.rssrc/ast/expr.rssrc/ast/fold_string_addition.rssrc/ast/g.rssrc/ast/import_record.rssrc/ast/known_global.rssrc/ast/lexer_log.rssrc/ast/lexer_tables.rssrc/ast/lib.rssrc/ast/loader.rssrc/ast/new_store.rssrc/ast/nodes.rssrc/ast/op.rssrc/ast/runtime.rssrc/ast/s.rssrc/ast/scope.rssrc/ast/server_component_boundary.rssrc/ast/stmt.rssrc/ast/symbol.rssrc/ast/target.rssrc/ast/transpiler_cache.rssrc/ast/ts.rssrc/ast/use_directive.rssrc/ast_jsc/lib.rssrc/base64/lib.rssrc/boringssl/lib.rssrc/boringssl_sys/boringssl.rssrc/brotli/lib.rssrc/brotli_sys/brotli_c.rssrc/bun.js.rssrc/bun_alloc/BufferFallbackAllocator.rssrc/bun_alloc/MaxHeapAllocator.rssrc/bun_alloc/MimallocArena.rssrc/bun_alloc/NullableAllocator.rssrc/bun_alloc/ast_alloc.rssrc/bun_alloc/baby_vec.rssrc/bun_alloc/basic.rssrc/bun_alloc/fallback.rssrc/bun_alloc/fallback/z.rssrc/bun_alloc/hashbrown_bridge.rssrc/bun_alloc/heap_breakdown.rssrc/bun_alloc/lib.rssrc/bun_alloc/maybe_owned.rssrc/bun_alloc/memory.rssrc/bun_alloc/stack_fallback.rssrc/bun_bin/lib.rssrc/bun_bin/phase_c_exports.rssrc/bun_core/Global.rssrc/bun_core/Progress.rssrc/bun_core/atomic_cell.rssrc/bun_core/bounded_array.rssrc/bun_core/debug.rssrc/bun_core/deprecated.rssrc/bun_core/env.rssrc/bun_core/env_var.rssrc/bun_core/external_shared.rssrc/bun_core/feature_flags.rssrc/bun_core/fmt.rssrc/bun_core/heap.rssrc/bun_core/hint.rssrc/bun_core/lib.rssrc/bun_core/output.rssrc/bun_core/result.rssrc/bun_core/string/HashedString.rssrc/bun_core/string/MutableString.rssrc/bun_core/string/SmolStr.rssrc/bun_core/string/StringBuilder.rssrc/bun_core/string/StringJoiner.rssrc/bun_core/string/escapeRegExp.rssrc/bun_core/string/identifier.rssrc/bun_core/string/immutable.rssrc/bun_core/string/immutable/exact_size_matcher.rssrc/bun_core/string/immutable/unicode.rssrc/bun_core/string/mod.rssrc/bun_core/string/write.rssrc/bun_core/string/wtf.rssrc/bun_core/thread_id.rssrc/bun_core/tty.rssrc/bun_core/util.rssrc/bun_core/windows_sys.rssrc/bun_core/wtf.rssrc/bun_core_macros/lib.rssrc/bun_output_tags/lib.rssrc/bundler/AstBuilder.rssrc/bundler/BundleThread.rssrc/bundler/Chunk.rssrc/bundler/DeferredBatchTask.rssrc/bundler/Graph.rssrc/bundler/HTMLImportManifest.rssrc/bundler/HTMLScanner.rssrc/bundler/IndexStringMap.rssrc/bundler/LinkerContext.rssrc/bundler/LinkerGraph.rssrc/bundler/OutputFile.rssrc/bundler/ParseTask.rssrc/bundler/PathToSourceIndexMap.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/analyze_transpiled_module.rssrc/bundler/barrel_imports.rssrc/bundler/bundle_v2.rssrc/bundler/bundled_ast.rssrc/bundler/cache.rssrc/bundler/defines.rssrc/bundler/entry_points.rssrc/bundler/lib.rssrc/bundler/linker.rssrc/bundler/linker_context/MetafileBuilder.rssrc/bundler/linker_context/OutputFileListBuilder.rssrc/bundler/linker_context/StaticRouteVisitor.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/convertStmtsForChunkForDevServer.rssrc/bundler/linker_context/doStep5.rssrc/bundler/linker_context/findAllImportedPartsInJSOrder.rssrc/bundler/linker_context/findImportedCSSFilesInJSOrder.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForCssChunk.rssrc/bundler/linker_context/generateCompileResultForHtmlChunk.rssrc/bundler/linker_context/generateCompileResultForJSChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessHTMLChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/renameSymbolsInChunk.rssrc/bundler/linker_context/scanImportsAndExports.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/bundler_jsc/PluginRunner.rssrc/bundler_jsc/analyze_jsc.rssrc/bundler_jsc/lib.rssrc/bundler_jsc/options_jsc.rssrc/bundler_jsc/source_map_mode_jsc.rssrc/bunfig/arguments.rssrc/bunfig/bunfig.rssrc/bunfig/lib.rssrc/c-headers-for-zig.hsrc/cares_sys/c_ares.rssrc/clap/args.rssrc/clap/comptime.rssrc/clap/lib.rssrc/clap/streaming.rssrc/clap_macros/lib.rssrc/codegen/bindgen-lib-internal.tssrc/codegen/bindgen-lib.tssrc/codegen/bindgen.tssrc/codegen/bindgenv2/internal/base.tssrc/codegen/bindgenv2/internal/dictionary.tssrc/codegen/bindgenv2/internal/union.tssrc/codegen/class-definitions.tssrc/codegen/cppbind.tssrc/codegen/generate-classes.tssrc/codegen/generate-host-exports.tssrc/codegen/generate-js2native.tssrc/codegen/generate-jssink.tssrc/collections/StaticHashMap.rssrc/collections/array_hash_map.rssrc/collections/array_list.rssrc/collections/bit_set.rssrc/collections/comptime_string_map.rssrc/collections/hive_array.rssrc/collections/identity_context.rssrc/collections/lib.rssrc/collections/linear_fifo.rssrc/collections/multi_array_list.rssrc/collections/pool.rssrc/collections/string_map.rssrc/collections/vec_ext.rssrc/collections/zig_hash_map.rssrc/crash_handler/CPUFeatures.rssrc/crash_handler/handle_oom.rssrc/crash_handler/lib.rssrc/csrf/lib.rssrc/css/compat.rssrc/css/context.rssrc/css/css_modules.rssrc/css/css_parser.rssrc/css/declaration.rssrc/css/dependencies.rssrc/css/error.rssrc/css/generics.rssrc/css/lib.rssrc/css/logical.rssrc/css/media_query.rssrc/css/prefixes.rssrc/css/printer.rssrc/css/properties/align.rssrc/css/properties/animation.rssrc/css/properties/background.rssrc/css/properties/border.rssrc/css/properties/border_image.rssrc/css/properties/border_radius.rssrc/css/properties/box_shadow.rssrc/css/properties/css_modules.rssrc/css/properties/custom.rssrc/css/properties/display.rssrc/css/properties/effects.rssrc/css/properties/flex.rssrc/css/properties/font.rssrc/css/properties/generate_properties.tssrc/css/properties/grid.rssrc/css/properties/list.rssrc/css/properties/margin_padding.rssrc/css/properties/masking.rssrc/css/properties/mod.rssrc/css/properties/outline.rssrc/css/properties/overflow.rssrc/css/properties/position.rssrc/css/properties/prefix_handler.rssrc/css/properties/properties_generated.rssrc/css/properties/properties_impl.rssrc/css/properties/shape.rssrc/css/properties/size.rssrc/css/properties/svg.rssrc/css/properties/text.rssrc/css/properties/transform.rssrc/css/properties/transition.rssrc/css/properties/ui.rssrc/css/rules/container.rssrc/css/rules/counter_style.rssrc/css/rules/custom_media.rssrc/css/rules/document.rssrc/css/rules/font_face.rssrc/css/rules/font_palette_values.rssrc/css/rules/import.rssrc/css/rules/keyframes.rssrc/css/rules/layer.rssrc/css/rules/media.rssrc/css/rules/mod.rssrc/css/rules/namespace.rssrc/css/rules/nesting.rssrc/css/rules/page.rssrc/css/rules/property.rssrc/css/rules/scope.rssrc/css/rules/starting_style.rssrc/css/rules/style.rssrc/css/rules/supports.rssrc/css/rules/tailwind.rssrc/css/rules/unknown.rssrc/css/rules/viewport.rssrc/css/selectors/builder.rssrc/css/selectors/mod.rssrc/css/selectors/parser.rssrc/css/selectors/selector.rssrc/css/small_list.rssrc/css/sourcemap.rssrc/css/targets.rssrc/css/values/alpha.rssrc/css/values/angle.rssrc/css/values/calc.rssrc/css/values/color.rssrc/css/values/color_generated.rssrc/css/values/css_string.rssrc/css/values/easing.rssrc/css/values/gradient.rssrc/css/values/ident.rssrc/css/values/image.rssrc/css/values/length.rssrc/css/values/mod.rssrc/css/values/number.rssrc/css/values/percentage.rssrc/css/values/position.rssrc/css/values/ratio.rssrc/css/values/rect.rssrc/css/values/resolution.rssrc/css/values/size.rssrc/css/values/syntax.rssrc/css/values/time.rssrc/css/values/url.rssrc/css_derive/lib.rssrc/css_jsc/color_js.rssrc/css_jsc/css_internals.rssrc/css_jsc/error_jsc.rssrc/dispatch/lib.rssrc/dns/lib.rssrc/dotenv/env_loader.rssrc/errno/darwin_errno.rssrc/errno/freebsd_errno.rssrc/errno/lib.rssrc/errno/linux_errno.rssrc/errno/windows_errno.rssrc/event_loop/AnyEventLoop.rs
💤 Files with no reviewable changes (6)
- src/ast_jsc/lib.rs
- src/bun_core/string/escapeRegExp.rs
- src/bun_alloc/memory.rs
- src/bun_core/feature_flags.rs
- src/ast/s.rs
- src/bun_alloc/basic.rs
…guards
- range_of_identifier: include the final codepoint's width when an
identifier runs to end-of-input, stop skipping the byte after a
bracketed \u{...} escape (zero the cursor width after the manual
advance), and handle an identifier that starts with a bracketed
escape; add unit tests
- spawn_sync_inherit_impl: fail with FileNotFound on empty argv instead
of panicking on the unix paths (matches the Windows arm)
- LinearFifo std::io::Write: follow the trait contract for
fixed-capacity buffers (partial write / Ok(0) when full) instead of
reporting ErrorKind::OutOfMemory
- w! macro: promote the ASCII-only debug_assert to a const assert so
non-ASCII literals are a compile error in every profile
- errno: add a win32_errno_name translation test
- clap test helper: rename 'allocations' to 'expected_owned_results'
This was referenced Jun 5, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jun 5, 2026
…eErrors (#31874) ### What Fixes a GC use-after-free found by fuzzing (ASAN fingerprint `heap-use-after-free`, read of size 1 at offset 144 inside a 152-byte region: that is exactly `BuildMessage.logged` inside a freed `Box<BuildMessage>`, confirmed via DWARF layout). ### Root cause JSC roots JSValues through the conservative stack scan. Values that live only in a malloc'd `Vec<JSValue>` buffer are invisible to the GC. `process_fetch_log` (module fetch failure path) created one `BuildMessage`/`ResolveMessage` wrapper per log message and pushed each into a heap `Vec<JSValue>` before handing the slice to `create_aggregate_error`. Every `create` call in that loop allocates JS cells, so a collection can run mid-loop; the wrappers created in earlier iterations have no reachable reference at that point, get swept, and their finalizers free the native `BuildMessage`. Anything that touches the stale cells afterwards (printing the unhandled rejection, reading `.message` off `error.errors[i]`) reads freed memory. The original Zig implementation used an on-stack `var errors_stack: [256]JSValue` specifically so the conservative scan would root these; the port replaced it with a heap `Vec`. The fixing lines are the `Vec<JSValue>` to `[JSValue; 256]` changes in `src/jsc/VirtualMachine.rs` (`process_fetch_log`) and `src/jsc/lib.rs` (`LogJsc::to_js`, same pattern, also feeds `create_aggregate_error`). `ast_jsc::log_to_js` already used the on-stack array and documents why. Same bug shape in `bake/production.rs`: `css_chunk_js_strings` held freshly created `JSString`s in a heap `Vec` across `preload_bundled_module` calls (which evaluate JS modules). That one is unbounded in length, so each value is kept `protect()`ed (RAII `ProtectedJSValue`) until consumed. An audit of the other `Vec<JSValue>` sites in the tree found them safe (values rooted elsewhere via `MarkedArgumentBuffer`, `protect()`, `Strong`, or no allocation while held). ### Reproduction A module with 257 duplicate `const` declarations produces 256 log messages. Importing it 16 times with `BUN_JSC_slowPathAllocsBetweenGCs=100` crashed the unfixed ASAN debug build 6/6 runs (ASAN UAF or `ASSERTION FAILED: isSymbol()` from swept-and-reused cells, depending on timing). The fuzzer hit the same window on a Worker thread with its `Bun.gc(true)` suffix. ### Tests `test/js/bun/resolve/build-error.test.ts`: "import with many build errors keeps AggregateError entries alive across GC". Fails on the unfixed ASAN debug build (child aborts, 6/6), passes on the fixed build (15/15 runs). It passes under a release build either way since the UAF needs ASAN (or unlucky reuse) to become observable; the ASAN CI suites are the enforcing ones. `test/js/bun/transpiler/transpiler-error-gc-uaf.test.ts` covers the `LogJsc::to_js` site directly: `Bun.Transpiler().transformSync` with 256 parse errors under `BUN_JSC_useZombieMode` and `BUN_JSC_collectContinuously` crashes the unfixed ASAN build (zapped StructureID while marking the errors array) and passes on this branch. Existing `plugins.test.ts`, `transpiler.test.js`, `dev-and-prod.test.ts` (5 PROD variants) and `framework-router.test.ts` pass. ### Linked issue Fixes #23181 That report is the same scenario: a loop of dynamic imports over files where some fail with build errors, with a try/catch around each. A script mirroring its `extract-messages.ts` shape (sequential `await import()` of modules with multiple build errors, catch block touching the error) crashes the unfixed ASAN build 3/3 runs under GC pressure and completes cleanly 3/3 on this branch. The original report was against v1.2.23, whose implementation of this path differed, so the exact 1.2.23 mechanism can't be re-verified, but the reported workload no longer crashes on current Bun with this change. ### Supersedes #30671 That earlier PR fixed the same two sites (`process_fetch_log`, `LogJsc::to_js`) but is now conflicting with main after the comment rewrites in #31783. This PR carries the same changes rebased on current main, absorbs its transpiler regression test, and additionally fixes the same unrooted-heap-Vec shape in bake's production build.
This was referenced Jun 5, 2026
This was referenced Jun 6, 2026
robobun
added a commit
that referenced
this pull request
Jun 6, 2026
The rebase over #31783 reverted the CoreFoundation/CoreServices no-deinit comments back to commented-out Zig blocks; restore main's wording, which also carries the leaked-for-process-lifetime invariant the Send SAFETY comment references.
robobun
added a commit
that referenced
this pull request
Jun 6, 2026
Restore main's wording for the two reverted hunks (module doc, PathWatcher::new doc) and replace the remaining Zig-era identifiers in comments with the real Rust names (deinit -> FSEventsWatcher::drop / remove_watch, addWatch/removeWatch, onFSEvent, unregisterWatcher, walkAndAdd, addOne, getOrPut, getFdPath, onPathUpdatePosix, runExitCallbacks, Zig enum-literal syntax).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolve the audited TODO(port)/TODO(b2)/PORT NOTE backlog
What this PR does
The Rust port accumulated a large backlog of
TODO(port)/TODO(b2-*)markers and"PORT NOTE" comments — a mix of stale organizational notes, completed-but-unverified
debts, real small bugs, and genuinely hard lifetime/GC restructures. This PR drains
that backlog: every one of the 2,327 audited markers (plus 1,342 PORT NOTE comment
lines across 346 files) was individually resolved — fixed, verified-and-documented,
deleted as stale, or split into a tracked follow-up work order.
Net diff: 1,025 files, +13,985 / −14,705.
Method
Audit-driven, per-file passes: each group of files got a fix pass, two independent
adversarial reviews, and a repair pass addressing reviewer findings; a second phase
drained cross-file escalations and leftover markers the same way.
Summary of actions
Notable code fixes by subsystem
no longer borrows registry Scope across the threadpool; pre-hashed maps switched to
identity hashing; detached-parent lifecycle-script completion uses the atomic
increment the Zig source specified; dead ScriptRunEnvironment removed.
printer validation);
Strong::getreads the handle slot like the Zig original;member-binding tracking wired into JSCell accessors; hot-reload error traces wired.
test.eacharguments are now GC-rooted for the duration of labelformatting/binding; html_rewriter end-tag callbacks use RAII protection; missing
globalThis.wasmSourceByteswiring for compiled-wasm entrypoints; server requesterror logging prints the JS stack trace; stream byte-blob fast path uses an
uninitialized-then-copied Uint8Array like upstream.
bun initfull-stack template runsbun installwith stdin ignored(matching upstream behavior) via a new no-stdin spawn helper with per-platform
handling;
open_urlactually spawns the opener; missing-entrypoint fallback to helprestored; completions install command delegates to the full implementation.
minified output uses it; identifier-range lookup is a faithful full-Unicode port;
per-call heap allocation removed from the printer's fmt path; macro enablement
matches the wasm gate.
single-lookup map entries.
renamer teardown matches upstream defer; several
'staticlifetime erasures replacedwith real borrows or owned copies.
and telemetry-enabled checks now match the spec.
fixed with a heap fallback; dead APIs removed; Zig-PRNG-faithful tests ported.
collapsing to "Unexpected"; long-path error split (FileNotFound vs NameTooLong).
verification-debt closure — full per-entry list in the group reports.
What deliberately remains
101
TODO(...)markers remain in-tree, each claimed by a written work order.These are the changes that should not land blind in a bulk branch: GC rooting of bare
JSValue fields, crate-wide arena-lifetime threading in bun_css, cross-thread
ownership restructures (installer task queue, copy_file, dns caching backrefs),
Stacked-Borrows provenance hardening, Windows fd-ownership modeling, and a ~1,400-line
diff test-suite port. Each gets a dedicated, individually reviewable follow-up PR.
Zero "PORT NOTE" strings remain. A follow-up sweep on this branch also removes all
remaining Zig references from comments (~18k lines: file/line cross-references,
std.* stdlib mentions, quoted Zig syntax, divergence notes) — this codebase is a
standalone Rust implementation and its comments now read that way. SAFETY
justifications were rewritten in terms of the Rust code's own invariants rather
than deleted. References to current identifiers (ZigString etc.) are unchanged.
Verification
caught and fixed by the compile pass: a unit-struct FFI-alignment marker in
uws_sysrewritten to the zero-length-array idiom, missing parens in twodebug_assert!s, and aCompileResultSlots::get_mutaccessor added so theDevServer HTML ownership transfer borrows mutably without new unsafe.
Bun.$shell,bun test. ThesetTimeoutsuite's3 leak-threshold failures reproduce identically on main's debug build
(pre-existing, not from this branch).
cargo check(windows/macos/linux × x64/aarch64) run before push.parsePackedFeaturesList()updated for the reshaped
define_features!macro, and a hot-reload serverregistration bug —
NewServer::initmoves the config into the server, so thehot-map insert after init read an empty id and panicked on the second
Bun.serveunder--hot. The path was unreachable before this branch'shot_map()lazy-init fix exposed it; registration now reads the server's ownconfig, and
HTTPServerAgent.test.tspasses.Caveats worth knowing during review: a handful of fixes are in
#[cfg(windows)]/wasm32 arms that only the cross-target check pass exercises, and the GC-adjacent
changes (test.each rooting, html_rewriter protection, Strong::get) deserve close
reading — pointers to the riskiest items are in the campaign's synthesis notes.