jsc: replace MarkedArrayBuffer::from_bytes with an owning constructor - #31986
Closed
robobun wants to merge 3 commits into
Closed
jsc: replace MarkedArrayBuffer::from_bytes with an owning constructor#31986robobun wants to merge 3 commits into
robobun wants to merge 3 commits into
Claude / Claude Code Review
completed
Jun 8, 2026 in 23m 43s
Code review found 3 potential issues
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/regression/marked-array-buffer-ownership-soundness.test.ts:44 |
Fixture --locked + opaque assertion will fail unhelpfully on workspace dep changes |
| 🟡 Nit | src/jsc/array_buffer.rs:991-993 |
SAFETY comment overstates owns_buffer invariant |
Annotations
Check warning on line 44 in test/regression/marked-array-buffer-ownership-soundness.test.ts
claude / Claude Code Review
Fixture --locked + opaque assertion will fail unhelpfully on workspace dep changes
The fixture's committed `Cargo.lock` records the dependency lists of ~80 `bun_*` path crates (bun_jsc transitively pulls in bun_bundler, bun_install, bun_http, etc.), so with `--locked` any PR that adds/removes/bumps a dep in any of those crates' Cargo.toml will fail this test with "the lock file needs to be updated but --locked was passed" — exit 101, no `E0599`/`from_bytes` in the output, so the assertion at lines 61-65 reports only `{rejectsFromBytes: false}` with no hint that the fix is `car
Check warning on line 993 in src/jsc/array_buffer.rs
claude / Claude Code Review
SAFETY comment overstates owns_buffer invariant
The SAFETY comment claims "`owns_buffer` is only set by `from_owned_bytes`", but `owns_buffer` is `pub` and `src/runtime/api/bun/subprocess/Readable.rs:313-317` constructs a `MarkedArrayBuffer { …, owns_buffer: true, … }` struct literal directly. The underlying invariant still holds there (it goes through `ArrayBuffer::from_owned_bytes`), so this is not a soundness issue — but since this PR's stated goal is to route every caller through the typed constructor, it'd be cleanest to migrate that sit
Loading