fs: honor length and position in write/writeSync when offset is undefined

[robobun]

Problem

• fs.writeSync(fd, buf, undefined, 8, 0) and callback fs.write write the whole buffer at the current file position and return a bytesWritten larger than length. Node writes 8 bytes at position 0.
• Same cause, more shapes: a numeric offset with no length ignores the position; writeSync(fd, buf, 17) returns 0 instead of throwing ERR_OUT_OF_RANGE; an out-of-range length or a function offset writes anyway; write with a bad offset calls back instead of throwing.
• Cause: the native argument parser stopped at the first slot it could not use, so nothing after an undefined offset or non-numeric length was read, and the offset check against the buffer only ran when a length was given.
• filehandle.write was already correct; it normalizes its arguments in JS first.

Fix

• The native parser reads offset, length and position as three independent slots with Node's defaults (nullish offset is 0, non-number length is the rest of the buffer, non-number position is the current file position) and always checks the offset against the buffer.
• Property to check: what one slot gets never depends on whether an earlier slot was skipped, which is how Node's writeSync reads them.
• Two JS changes: writeSync sends null down the options path like Node, so it still discards the trailing arguments; write turns a callback in the offset slot into offset 0.
• Verification: new tests run an argument table with Node v26.3.0 expectations through writeSync, callback write and filehandle.write, plus the error cases. All four fail on the current release and pass here. The wording of the length range error is deliberately unchanged.

Background

• fs.write(fd, buffer[, offset[, length[, position]]]): offset and length pick a slice of the buffer, position says where in the file it goes. A non-number position means "wherever the fd is now", so a lost position shows up as an append.
• Bun's write and writeSync are a thin JS layer over a native binding. The JS layer handles the options-object form; the binding parses the positional form and throws the range errors.
• In Node, null in the offset slot means an empty options object, so writeSync(fd, buf, null, 8, 0) writing the whole buffer at the current position is correct and unchanged here.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts

Original description

Reproduction

const fs = require("fs");
const buf = Buffer.alloc(64); for (let i = 0; i < 64; i++) buf[i] = i;
const fd = fs.openSync("out.bin", "w+");
fs.writeSync(fd, "PPPPPPPPPPPP");                  // 12 bytes, file offset now 12
const n = fs.writeSync(fd, buf, undefined, 8, 0);  // 8 bytes at position 0
fs.closeSync(fd);
console.log("bytesWritten", n, "file length", fs.readFileSync("out.bin").length);
fs.write(fs.openSync("out.bin", "w+"), buf, undefined, 8, 0, (e, n2) => console.log("fs.write bytesWritten", n2));

$ node repro.js        # v26.3.0
bytesWritten 8 file length 12
fs.write bytesWritten 8

$ bun repro.js         # 1.4.0, before
bytesWritten 64 file length 76
fs.write bytesWritten 64

Bun wrote the whole buffer at the current file position: length and position were silently ignored, and callers got bytesWritten > length. The same parser produced a few more divergences from Node, all visible with the same 16 byte buffer written into a file that already holds 12 bytes of P:

call	Node	Bun before
writeSync(fd, buf, undefined, 8, 0)	8 bytes at 0	16 bytes appended
writeSync(fd, buf, 4, undefined, 0)	12 bytes at 0	12 bytes appended (position ignored)
writeSync(fd, buf, undefined, "8", 0)	16 bytes at 0	16 bytes appended
writeSync(fd, buf, 17)	ERR_OUT_OF_RANGE ("offset")	returns 0, nothing written
writeSync(fd, buf, undefined, 17) / (..., undefined, -1)	ERR_OUT_OF_RANGE ("length")	writes 16 bytes
writeSync(fd, buf, () => {}, 8, 0)	ERR_INVALID_ARG_TYPE	writes 16 bytes
write(fd, buf, 17, cb)	throws synchronously	calls back with 0

filehandle.write was already correct (it normalizes the arguments in JS before calling the binding), and null in the offset slot already matched Node (Node routes it through the options path, which discards the trailing arguments).

Cause

The buffer branch of args::Write::from_js (src/runtime/node/node_fs.rs) was written as a chain that breaks out of the parse at the first slot it cannot use: an undefined/null/function offset, or a non-numeric length. Every slot after that point was never read, and the offset <= byteLength check lived inside the length branch, so it only ran when a length was passed.

Node (lib/fs.js writeSync, v26.3.0 L893-L900) treats each slot independently: offset == null is 0 and otherwise validated, a non-number length is byteLength - offset, a non-number position means the current file position, and validateOffsetLengthWrite always runs.

Fix

• node_fs.rs: parse the three slots positionally with Node's defaults, and validate the offset against the buffer unconditionally. As in Node, only a number counts as a length, so a bigint length now takes the default the same way a bigint position does since fs: short write in createWriteStream overwrites head of file (NaN position coerced to 0) #36135 (writeSync(fd, buf, undefined, 8n, 0) writes the whole buffer at 0 in both). The position slot goes through the i52::offset_from_js that fs: short write in createWriteStream overwrites head of file (NaN position coerced to 0) #36135 added. The string branch is written in the same per-slot style; its behavior is unchanged.
• fs.ts writeSync: null takes the options path like Node (and like fs.write / filehandle.write already did in Bun), so it keeps discarding length and position now that the native parser no longer bails out on it.
• fs.ts write: a function in the offset slot (fs.write(fd, buf, cb)) becomes offset 0, which is Node's rule for the callback form. The native parser previously special-cased functions for this; now it rejects them, which is what Node's writeSync does.

Intentionally unchanged: the explicit-length range check and its message (Bun reports >= 0 and <= N where Node picks one bound). With that one wording difference normalized, a 30-shape matrix (the table above plus {}, null, string/bigint/NaN/fractional values in each slot) produces byte-identical output under Node v26.3.0 and this branch for both writeSync and callback write.

Verification

New tests in test/js/node/fs/fs.test.ts run the same argument table (expectations taken from Node v26.3.0) through writeSync, callback write and filehandle.write, plus the error cases above. All four new tests fail on the current release and pass with this change; the rest of fs.test.ts, promises.test.js and the ported test-fs-write* / test-fastutf8stream-* Node tests still pass.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 37794186-a31e-4000-8407-6446634cc123

📥 Commits

Reviewing files that changed from the base of the PR and between 1d28cc4 and 15c9099.

📒 Files selected for processing (3)

• src/js/node/fs.ts
• src/runtime/node/node_fs.rs
• test/js/node/fs/fs.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: reproduced on the 1.4.0 release and on a debug build of main (fs.writeSync(fd, buf, undefined, 8, 0) returned 64 and appended the whole buffer; callback fs.write the same), with Node v26.3.0 as the reference for every case in the new table. Rebased onto main after #36135 landed in the same parser (position now goes through i52::offset_from_js; bigint length follows Node like bigint position does). The first revision passed CI in full; waiting on CI for the rebased one.

[robobun]

^{Updated 4:41 PM PT - Aug 11th, 2026}

✅ @robobun, your commit 15c9099634772d07a46ea37afbe6c87e24545216 passed in Build #92567! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37647

That installs a local version of the PR into your bun-37647 executable, so you can run:

bun-37647 --bun

[claude]

I reviewed this and didn't find any bugs. The argument-parsing rewrite traces cleanly against Node's per-slot semantics and the tests are thorough, but since this changes what fs.write/writeSync actually put on disk for several argument shapes, a human sign-off on the Node-compat semantics would be worthwhile.

What was reviewed:

• Write::from_js buffer branch: buf_len - args.offset can't underflow (guarded by the now-unconditional offset > buf_len check); args.length still defaults to u64::MAX on non-numeric length, which the downstream write already clamps.
• String branch is behavior-equivalent to the old next()/eat() sequence.
• JS-side callers: filehandle.write and the fs.promises writeSync path pass fully-normalized numeric args to the binding, so they're unaffected by the parser change; the new null-as-options routing in writeSync preserves pre-PR behavior now that the native parser no longer bails on it.

Extended reasoning...

Overview

The PR fixes fs.write / fs.writeSync to honor length and position when offset is undefined (or otherwise defaulted), matching Node v26.3.0. It touches three files: the native args::Write::from_js parser in src/runtime/node/node_fs.rs (refactored from a break-on-first-unusable-slot chain to positional per-slot parsing, net −14 lines), two small adjustments in src/js/node/fs.ts (route null offset through the options path in writeSync; convert a function offset to 0 in callback write), and ~150 lines of table-driven tests in test/js/node/fs/fs.test.ts running the same argument matrix through writeSync, callback write, and filehandle.write.

Security risks

None. Input validation is strictly tightened (offset is now unconditionally bounds-checked against byteLength, and a function offset in writeSync now throws instead of being silently accepted). No new untrusted-input surface.

Level of scrutiny

High. fs.write/writeSync is production-critical Node compat surface, and this is a behavior change — it alters which bytes are written and where for several argument combinations that previously silently diverged from Node. The interaction between three JS-layer entry points (each with its own pre-normalization) and the shared native parser is subtle enough that a maintainer should confirm the chosen Node v26 semantics.

Other factors

I verified: next_eat() semantics match the intent; the buf_len - args.offset subtraction is guarded; validate_integer produces the ERR_INVALID_ARG_TYPE the new test expects for a function offset; the string branch is byte-for-byte equivalent to the old code; and the two other binding callers (filehandle.write in fs.promises.ts:635 and the pipeTo fast path at fs.promises.ts:1352) already pass fully-normalized numeric args, so they're unaffected. The PR description is unusually thorough (Node comparison table, explicit list of what's intentionally unchanged, cross-reference to adjacent open PRs), and the tests are strong — each case asserts both bytesWritten and the resulting file contents so a wrong-position write can't pass. No prior review comments to address.

[claude]

I reviewed this PR and didn't find any bugs. The argument-parsing rewrite looks correct and the test table is thorough, but since it restructures the native fs.write/writeSync argument parser that every caller goes through, a human look would still be worthwhile.

What was reviewed:

• Traced each JS caller (write, writeSync, filehandle.write) into the new native parser to confirm the removed is_function() special-case is now covered in fs.ts and that a function offset in writeSync reaches validate_integer → ERR_INVALID_ARG_TYPE.
• Checked buf_len - args.offset cannot underflow (guarded by the preceding offset > buf_len throw) and that the i32::MAX length cap is preserved.
• Confirmed i52::offset_from_js still returns None for bigint/function positions, so the existing "treats a bigint position as the current offset" test stays green.
• The string branch is a straight de-nesting with no behavior change.

Extended reasoning...

Overview

This PR fixes Node.js compatibility in fs.write / fs.writeSync argument parsing: when offset is undefined, Bun was bailing out of the parse and ignoring the trailing length and position arguments, writing the entire buffer at the current file position. The fix rewrites the buffer branch of args::Write::from_js in src/runtime/node/node_fs.rs to parse each slot independently with Node's per-slot defaults (matching lib/fs.js L893-L900 in Node v26.3.0), moves the offset <= byteLength check out so it runs unconditionally, and de-nests the string branch in the same style. src/js/node/fs.ts gets two small adjustments: writeSync now routes null through the options path (so it keeps discarding trailing args now that native no longer bails on it), and callback write maps a function-in-offset-slot to 0 since native no longer special-cases functions. ~160 lines of table-driven tests cover writeSync, callback write, and filehandle.write against Node v26.3.0 output.

Security risks

None identified. This is argument coercion/validation for a local filesystem write; no auth, crypto, network, or untrusted-data parsing surface. The new code adds stricter validation (unconditional offset bounds check, ERR_INVALID_ARG_TYPE for a function offset in writeSync) rather than loosening any.

Level of scrutiny

Medium-high. fs.write/writeSync is a hot, widely-used Node compat path, and this rewrites its argument parser rather than patching one branch. The change is well-contained (one from_js function plus two small JS shims), the PR body cites the exact Node source lines it mirrors, and the test table exercises every slot combination plus the error paths — but any regression here would be visible to a lot of user code, so it warrants a human pass rather than bot-only approval.

Other factors

I walked through the arithmetic: args.offset is validated >= 0 before u64::try_from().expect(), the offset > buf_len guard precedes buf_len - args.offset so the subtraction cannot underflow, and length as u64 follows a length >= 0 check. validate_integer throws ERR_INVALID_ARG_TYPE on non-numbers, which produces the tested behavior for writeSync(fd, buf, () => {}, ...). i52::offset_from_js uses get_number(), so bigint/function positions still yield None and the neighbouring "treats a bigint position as the current offset" test is unaffected. The removal of is_big_int() from the length check is intentional and covered by the new 8n test row (Node's typeof length !== 'number' also rejects bigint). The string-overload branch is a mechanical de-nesting; the position-eat-then-encoding order is unchanged. CI is still building and there are no prior human reviews to defer to.