Repository navigation
Conversation
|
Updated 3:46 AM PT - Jun 19th, 2026
❌ @robobun, your commit 0b4a1a4 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32279That installs a local version of the PR into your bun-32279 --bun |
|
Status: diff is ready; CI failures are unrelated. Needs a maintainer. Reproduced with: bun -e 'const m=new Map([[1,1]]); Object.defineProperty(m,"size",{value:3000000000}); console.log(m)'Test: Rebased onto main at 0b4a1a4 (took main's CI (build #63487, complete): 284 passed / 2 failed. Both failures are darwin-aarch64 network/external-service flakes unrelated to this diff:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (4)
WalkthroughReplaces all coerce_to_i32 replacement sweep
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 377-379: The test file currently has an assertion for Map with
Infinity size saturation at lines 377-379 but lacks the corresponding assertion
for Set. Add a matching test case after the mapWithInfiniteSize test that
creates a Set object with a size property set to Infinity, and verify that
Bun.inspect handles the saturation the same way by outputting the capped maximum
integer value (2147483647) instead of Infinity. This ensures both the Map and
Set formatter paths are tested at this boundary condition.
🪄 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: f19e8ef1-fa81-466d-bf06-457773febb6c
📒 Files selected for processing (6)
src/jsc/ConsoleObject.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/node/types.rssrc/runtime/server/ServerWebSocket.rssrc/sourcemap_jsc/JSSourceMap.rstest/js/bun/util/inspect.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical swap to the saturating coercion helper, each site cross-referenced against the Zig original.
Extended reasoning...
Overview
Nine one-line swaps from JSValue::coerce_to_i32 (ECMA ToInt32 modular wrap) to JSValue::coerce::<i32> (saturating truncation, NaN→0) across ConsoleObject.rs, PasswordObject.rs, ServerWebSocket.rs, JSSourceMap.rs, and node/types.rs, plus three new assertions in inspect.test.js. I confirmed in src/jsc/JSValue.rs:1883-1898 that CoerceTo for i32 is exactly the saturating fast-path described (f64 as i32, falling back to coerce_to_i32 only for non-numbers), so for number inputs this is purely a wrap→saturate behavior change and for non-numbers it is identical.
Security risks
None. Inputs are display lengths, hash cost parameters (still range-checked immediately after), a WebSocket close code, sourcemap line/column lookups, and an fs flag already guarded by isInt32. No auth, crypto-key, or memory-safety surface is touched; the argon2/bcrypt cost values are still bounds-checked before use.
Level of scrutiny
Low–medium. The diff is mechanical and each call site is annotated against its .zig sibling in the PR description. The only sites where observable behavior could change for non-pathological inputs are the argon2 timeCost/memoryCost paths (a value ≥ 2³¹ now saturates and passes the >= 1 / >= 8 checks instead of wrapping negative and throwing), which is the documented Zig-parity behavior and still user-intentional. The FileSystemFlags site is gated by is_int32() so it is a no-op.
Other factors
New regression tests cover the headline Map/Set case; the bug-hunting pass found nothing; no CODEOWNERS apply to the touched paths; no outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — mechanical swap to the saturating coerce::<i32> helper to match the Zig reference, with regression tests for the observable Map/Set case.
Extended reasoning...
Overview
This PR replaces JSValue::coerce_to_i32 (ECMA ToInt32 modular wrap) with JSValue::coerce::<i32> (saturating truncation, NaN→0) at nine call sites across five Rust files, plus four new test assertions in inspect.test.js. I verified in src/jsc/JSValue.rs that coerce::<i32> fast-paths numbers with saturating f64 as i32 and falls through to coerce_to_i32 for non-numbers, exactly as the PR description claims.
Security risks
None. The change only affects how out-of-i32-range or non-finite numeric inputs are clamped before existing range validation. In every touched site the value is either (a) immediately range-checked and rejected (PasswordObject cost/timeCost/memoryCost, all guarded by is_number()), (b) already guarded by is_int32() so the swap is a no-op (FileSystemFlags), (c) a display-only length (ConsoleObject), or (d) a lookup key where wrap vs. saturate both produce a miss (JSSourceMap line/column, ServerWebSocket close code). No new code paths, no auth/crypto algorithm changes — only the integer coercion of already-validated parameters.
Level of scrutiny
Low. This is a mechanical correction aligning the Rust port with its Zig reference (the PR cites each .zig line). The only user-observable change is the headline fix: Map(2147483647) instead of Map(-1294967296) for pathological size overrides, which is covered by the new regression assertions. The other sites differ only in which error message fires for absurd inputs like cost: Infinity.
Other factors
The author audited remaining coerce_to_i32 sites and confirmed udp_socket.rs/dns.rs correctly use the wrapping form per their Zig references. CodeRabbit's only suggestion (add a Set + Infinity parity assertion) was addressed in 9cb92f1 and the thread is resolved. The bug-hunting system found nothing. Test commands in the description show the relevant suites passing.
When a Map or Set has a size getter returning a number >= 2^31 (via
defineProperty, proxy, or subclass), Bun.inspect/console.log printed the
ECMA ToInt32 modular-wrapped value (e.g. Map(-1294967296)) instead of
saturating to i32::MAX like the original Zig implementation.
The Rust port called JSValue::coerce_to_i32 (JSC ToInt32, modular wrap)
where the Zig reference called JSValue.coerce(i32, ...) (saturating
truncation: NaN->0, overflow->min/max). The Rust equivalent is
JSValue::coerce::<i32>.
Auditing every coerce_to_i32 call site against its .zig sibling found
seven more sites with the same swap; all are corrected here:
- ConsoleObject.rs: Map/Set size (the reported bug)
- PasswordObject.rs: bcrypt cost, argon2 timeCost/memoryCost
- ServerWebSocket.rs: close code
- JSSourceMap.rs: findEntry line/column
- types.rs: FileSystemFlags numeric flag (no behavior change; already
guarded by isInt32)
udp_socket.rs and dns.rs were checked and already match their Zig
reference (which uses coerceToInt32).
76a1d25 to
0b4a1a4
Compare
There was a problem hiding this comment.
LGTM — mechanical coerce_to_i32 → coerce::<i32> swap restoring Zig-reference parity; my earlier note on the argon2 timeCost: Infinity edge case is now documented in the description as intentional.
Extended reasoning...
Overview
Swaps JSValue::coerce_to_i32 (ECMA ToInt32, modular wrap) for JSValue::coerce::<i32> (saturating truncation) at 7 call sites across 4 files, restoring parity with the Zig reference implementation that the Rust port was meant to mirror. Adds 4 test assertions in inspect.test.js covering Map/Set with oversized/infinite size. I verified in src/jsc/JSValue.rs that coerce::<i32> does f64 as i32 (saturating, NaN→0) for number values, matching the PR's claim and the Zig coerce(i32, ...) semantics.
Security risks
None material. PasswordObject.rs is crypto-adjacent but the change only affects argument coercion, not crypto primitives. The argon2 timeCost/memoryCost: Infinity case now saturates to i32::MAX instead of wrapping to 0 and throwing — I flagged this in a prior inline comment, and the author confirmed it's intentional 1.3.x/Zig parity (the missing upper bound is pre-existing and orthogonal; e.g. timeCost: 1e8 already hangs on both old and new). bcrypt cost is unaffected (bounded 4–31). ServerWebSocket close() code and JSSourceMap line/column are both guarded by is_number() and have no security implications from saturation vs. wrap.
Level of scrutiny
Low. This is a mechanical helper-name correction following an established pattern (match the .zig reference), with a thorough per-site audit table in the description. The headline fix (negative Map/Set size in console output) is purely cosmetic. All sites are already gated by is_number() checks, so the only behavioral delta is the saturate-vs-wrap edge for out-of-i32-range numbers.
Other factors
CI build #63487 is green (prior build had only unrelated darwin agent expirations and one streams-leak flake). All review threads are resolved: coderabbit's Set+Infinity test parity nit was addressed in 0b4a1a4, and my PasswordObject behavior-change note was addressed by documenting it in the PR description. No bugs found by the bug-hunting system.
|
Stale PR review: keep open, rework. The bug is real and still on main.
The |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-19, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Repro
Cause
ConsoleObject.rsreads the Map/Set.sizeproperty and coerces it toi32viaJSValue::coerce_to_i32, which is the JSCToInt32path (ECMA modular wrap:3000000000becomes-1294967296). The Zig reference (ConsoleObject.zig:2761,2868) usedlength_value.coerce(i32, ...), which is the saturating-truncation helper (NaN→0, overflow→i32::MIN/MAX). The Rust equivalent of Zig'scoerce(i32, ...)isJSValue::coerce::<i32>, notcoerce_to_i32.Low real-world impact since a real
Map.prototype.sizecannot exceed available memory, but it is observable viadefineProperty, asizegetter on a subclass, or a Proxy, and the negative number is plainly wrong.Fix
Switch the two sites in
ConsoleObject.rstocoerce::<i32>.Auditing every remaining
coerce_to_i32call site against its.zigsibling turned up more sites with the same swap; all are corrected to match the Zig reference:ConsoleObject.rssizeConsoleObject.zig:2761,2868PasswordObject.rscost, argon2timeCost/memoryCostPasswordObject.zig:44,63,77ServerWebSocket.rsclose()codeServerWebSocket.zig:1080JSSourceMap.rsfindEntryline/columnJSSourceMap.zig:198,199udp_socket.rsanddns.rswere checked and already match (the Zig reference usescoerceToInt32there). Reverse-direction spot checks of existingcoerce::<i32>sites found no mismatches.node/types.rsFileSystemFlagsoriginally had the same swap but was superseded by #32506 on main (now usesvalidate_int32); the rebase took main's version.Note on
PasswordObject.rsFor argon2
timeCost/memoryCost, this intentionally reverts to 1.3.x behavior:timeCost: Infinitynow saturates toi32::MAXand passes the>= 1guard instead of wrapping to0and throwing. Neither version has an upper-bound check (e.g.timeCost: 1e8already runs for an impractical duration on both 1.3.x and main), so the lack of an upper bound is a pre-existing, orthogonal concern. bcryptcostis unaffected since it already has> 31 → throw.Verification
New assertions in
test/js/bun/util/inspect.test.jscover Map/Set withsize=3000000000andInfinity, expectingMap(2147483647)/Set(2147483647).USE_SYSTEM_BUN=1 bun test test/js/bun/util/inspect.test.js -t "^inspect$"→ fail (Map(-1294967296))bun bd test test/js/bun/util/inspect.test.js→ 72 passbun bd test test/js/bun/util/password.test.ts→ 68 passbun bd test test/js/node/module/module-sourcemap.test.js→ 3 pass