Repository navigation
node:fs: apply the string encoding in write/writeSync and other Node compat fixes - #32813
Conversation
…compat fixes
- fs.write / fs.writeSync / filehandle.write with a string argument now
apply the encoding. The argument parser already read it into
args.encoding, but the string had been converted to UTF-8 two lines
earlier and write_inner never looked at it, so utf16le / hex / base64
/ latin1 / ascii all wrote raw UTF-8 bytes. The position slot is now
also consumed whatever its type (null, undefined, a non-number),
matching Node's binding, so the encoding in the following slot is
reached.
- fs.readdir with { encoding: "buffer" } returns real Node Buffer
entries instead of plain Uint8Arrays.
- fs.Dir implements Symbol.dispose / Symbol.asyncDispose, so
`using` / `await using` work. Disposing an already-closed Dir is a
no-op, like Node.
- An unrecognized open-flag string throws ERR_INVALID_ARG_VALUE,
Node's code, instead of ERR_INVALID_ARG_TYPE.
|
Warning Review limit reached
More reviews will be available in 14 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
|
Updated 5:56 AM PT - Jun 28th, 2026
❌ @robobun, your commit 27ee839 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32813That installs a local version of the PR into your bun-32813 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Neither bot suggestion applies as-is, so I have not added
The "Out of scope" section of the description now links both. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this bundles four distinct behavior changes — including a rework of the fs.write string-arg parsing/encoding path — so it's worth a human pass before merging.
Extended reasoning...
Overview
This PR fixes four separate node:fs Node-compat divergences:
src/runtime/node/node_fs.rs—args::Write::from_js: always consume the position slot (regardless of type) and re-encode the string buffer withStringOrBuffer::from_js_with_encodingonce the encoding argument is known. This is the headline fix (silent data corruption when writing strings with a non-UTF-8 encoding).src/runtime/node/node_fs.rs—ret::Readdir::to_js: swapto_jsforto_node_bufferso{ encoding: "buffer" }returnsBuffer[]instead ofUint8Array[].src/js/node/fs.ts— addSymbol.dispose/Symbol.asyncDisposeto theDirclass.src/runtime/node/types.rs— switch three flag-string error sites fromERR_INVALID_ARG_TYPEtoERR_INVALID_ARG_VALUE.
Tests are added in fs.test.ts and dir.test.ts covering all four, with expected bytes verified against Node v26.3.0.
Security risks
None identified. No auth, crypto, or permissions code is touched; the changes are confined to argument parsing, return-value construction, and error codes in the local-filesystem API.
Level of scrutiny
Moderate-to-high. Items 2–4 are essentially mechanical (a one-call swap, two prototype methods that mirror the existing Symbol.asyncIterator guard, and an error-code constant change). Item 1, however, changes control flow in a hot, widely-exercised parser: it now unconditionally eats the position slot and replaces args.buffer mid-parse with a freshly-encoded slice. The drop semantics look correct (the old StringOrBuffer is dropped on assignment, and the new EncodedSlice owns its bytes so the async to_thread_safe path is a no-op), and the test table is thorough — but argument-shape changes to fs.write/writeSync have a large blast radius and the PR itself notes intentional divergences left for a maintainer (uppercase flag aliases, withFileTypes + buffer encoding).
Other factors
- No CODEOWNERS coverage for the touched paths.
- No prior human review on the timeline; CI build is in progress.
- The PR description is detailed and the vendored Node test suites are reported to still pass, but four unrelated fixes in one PR makes it harder to bisect if a regression slips through.
Given the non-trivial parser change in a core write path, I'm deferring rather than auto-approving.
There was a problem hiding this comment.
I didn't find any issues, but the args::Write::from_js change reworks argument-cursor semantics in the core fs write path (data-integrity-critical), so this warrants a human look — CI also reported failures on f10e441 that should be confirmed green after the retrigger.
Extended reasoning...
Overview
Four bundled node:fs Node-compat fixes across 5 files:
src/runtime/node/node_fs.rs:args::Write::from_jsnow always consumes the position slot (not just when it's a number) and re-encodes the string viaStringOrBuffer::from_js_with_encodingonce the encoding argument is known.ret::Readdir::to_jsswapsto_js→to_node_bufferso{ encoding: 'buffer' }returnsBuffer[].src/runtime/node/types.rs:FileSystemFlags::from_jsswitches three unrecognized-flag-string error sites fromERR_INVALID_ARG_TYPEtoERR_INVALID_ARG_VALUE.src/js/node/fs.ts: addsSymbol.dispose/Symbol.asyncDisposeto theDirclass (no-op when already closed).- Tests added to
fs.test.tsanddir.test.tscovering each fix across sync/async/promise/callback entry points.
Security risks
None identified. No auth, crypto, or permissions code is touched. The changes affect argument parsing and result-type construction for local filesystem APIs only.
Level of scrutiny
Medium-high. Three of the four fixes are mechanical (error-code swap, to_node_buffer one-liner, two prototype methods). The fourth — the args::Write parser change — is in the hot path that decides what bytes fs.write/fs.writeSync/filehandle.write actually put on disk. The new behavior (always eat the position slot, only honor non-negative numbers, then re-encode the original bv JSValue with the parsed encoding) reads correctly against Node's WriteString semantics and the sibling Buffer arm, and the reassigned args.buffer is an owned EncodedSlice that survives to_thread_safe() in the async path. But it's a subtle cursor-semantics change in data-integrity-critical code, and bundling it with three unrelated fixes makes it worth a maintainer's eyes.
Other factors
- robobun reported CI failures on f10e441; the latest commit is a "ci: retrigger" with no updated status visible in the timeline yet. A human should confirm the build is green before merging.
- The PR description is thorough, the test table is byte-exact against Node v26.3.0, and the out-of-scope notes (interaction with #30484, case-insensitive flags) are clearly delineated.
- No prior human review on the thread; only bot comments so far.
CI status (final, build 66270,
|
| host | result |
|---|---|
bingus |
24 failed, 0 passed |
darwin-test-arm64-5 |
2 failed |
darwin-test-arm64-3, darwin-test-arm64-4, 66783.local, 66790.local, 79509.local |
all passed |
The darwin-aarch64-build-bun step that produces the artifact passed, so bingus just cannot fetch it. Whoever owns the macOS agents probably wants it out of rotation. (65289: 280 passed / 2 failed on darwin-test-arm64-5. 65325: 285 passed / 1 failed on bingus. 65961: 1 failed on bingus.)
2. A Docker image / CPU-architecture mismatch in a WebSocket test. The :darwin: 14 aarch64 - test-bun shard's only failing test file is test/js/web/websocket/autobahn.test.ts, and its failure is Docker on the agent host:
Failed to start service autobahn: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
The Autobahn conformance suite needs a container whose image is x86-only and the agent (66783.local) is an arm64 Mac, so the container never starts. That test, file, and subsystem are untouched by this PR, which changes fs argument parsing, a Buffer constructor, a Dir prototype method, error codes, and tests; a filesystem diff cannot change a Docker image's architecture. (test/js/node/fs/fs.test.ts passed on that shard.)
I used the one empty retrigger already and the robobun BuildKite token is read-only (the retry/rebuild API returns 403), so I will not push more empty commits. Retrying those two jobs from the BuildKite UI should clear the first as soon as it lands off bingus; the second needs the autobahn image pulled for linux/arm64 (or the test pinned to platform: linux/amd64), which is independent of this PR.
Node treats the "buffer" encoding name in
fs.writeSync(fd, string, position, encoding) as UTF-8:
fs.writeSync(fd, "\u00e9", 0, "buffer") // Node writes c3 a9
The re-encoding step added here for the other encodings also fired for
Encoding::Buffer, and the string encoder's Buffer arm is a verbatim
latin1 / low-byte copy, so the same call would have written the single
byte e9. Exclude Encoding::Buffer from the re-encode so the original
UTF-8 conversion is kept, and add the case to the writeSync table.
|
Heads up: the |
The "buffer" writeSync case added in the previous commit only uses an 8-bit (latin1-backed) string, which exercises construct_from_u8. A 16-bit string goes through construct_from_u16, whose Latin1 | Buffer | Ascii arm is a low-byte narrow, so on the broken code writeSync(fd, "a\u00e9\u4e2d", 0, "buffer") wrote 61 e9 2d instead of Node's UTF-8 61 c3 a9 e4 b8 ad. Add that row so both string widths are pinned.
There was a problem hiding this comment.
I didn't find issues with the changes, but this bundles four fixes including a rework of args::Write::from_js argument parsing (data-correctness-critical) and a buffer-ownership swap in ret::Readdir::to_js, plus the latest robobun status shows build-rust failures on Build #65947 — worth a human look before merge.
Extended reasoning...
Overview
Four independent Node.js compat fixes in node:fs: (1) fs.write/writeSync/filehandle.write now actually apply the string encoding argument by re-encoding via StringOrBuffer::from_js_with_encoding after the position slot is consumed; (2) readdir with { encoding: 'buffer' } now returns Buffer[] via to_node_buffer instead of Uint8Array[] via to_js; (3) fs.Dir gains Symbol.dispose/Symbol.asyncDispose; (4) unrecognized flag strings now throw ERR_INVALID_ARG_VALUE instead of ERR_INVALID_ARG_TYPE. New test coverage in fs.test.ts and dir.test.ts with Node-v26.3.0-derived expected bytes.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input parsing surface. The changes are confined to Node-compat argument handling and JS-value construction.
Level of scrutiny
Moderate-to-high. The args::Write::from_js change rewrites how the position slot is consumed and adds a re-encode step on a hot, data-correctness-critical path (silent wrong-bytes-written was the original bug). I traced bv back to line ~3746 and confirmed the re-encode is sync (with to_thread_safe() applied afterward via Write::to_thread_safe), and that to_node_buffer returns JSValue (not JsResult) so the dropped ? + ZERO check is correct. Still, this is exactly the kind of argument-parser change where a maintainer familiar with the ArgumentsSlice cursor semantics and the async StringOrBuffer lifecycle should sign off.
Other factors
- The most recent in-place robobun CI status update (6:26 PM PT Jun 27, Build #65947 on
3d403252) reportsbuild-rustfailures onx64andaarch64-musl, which is a different failure mode than the Darwin artifact-download flake the author analyzed earlier. A newer commite74c0b66was pushed afterward but its CI result isn't reflected in the timeline yet. - The author notes the
types.rshunk is superseded by #32966 and will need to be dropped on rebase — a maintainer should decide on sequencing. - Four logically independent fixes in one PR increases the review surface beyond what I'd auto-approve.
|
Two clarifications on the points the review above raised: The #32966 sequencing: either order works. #32966 rewrites all of |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Where are the missing node.js test/js/node/test/parallel tests covering this? use bun node:test:cp to copy them.
…xposes Node's test/parallel/test-fs-write.js is the upstream regression test for the write/writeSync string-encoding bug this PR fixes (its nodejs/node#38168 block asserts writeSync(fd, "abcd", 0, "hex") returns 2). Vendoring it surfaced three more gaps in fs.write's string path, each fixed at the layer Node fixes it: - validateEncoding: Node rejects an odd-length string written with the hex encoding (ERR_INVALID_ARG_VALUE, "is invalid for data of length N") from write, writeSync, and filehandle.write. The C++ validator already existed in NodeValidator.cpp, exported from internal/validators with no callers; wire it into the three string write paths where Node calls it. - fs.write was missing writeSync's typeof-string check, so a String object (and every other non-string, non-view value) reached the native parser through the string signature instead of throwing ERR_INVALID_ARG_TYPE. - fs.write branched on $isTypedArrayView, which excludes DataView, so fs.write(fd, dataView, offset, length, cb) was parsed as the (position, encoding) string signature. Use Node's isArrayBufferView predicate, which the writeSync sibling below already uses. The vendored test carries one local patch, marked with a leading "// Bun:" comment per the existing convention: upstream runs it under V8's --expose_externalize_string flag and calls the createExternalizableString / externalizeString / isOneByteString globals it exposes, a V8 string representation with no JavaScriptCore equivalent. Those calls are removed; every fs assertion (the writeSync latin1 / utf8 / ucs2 writes and their read-backs, the encoding validation, and the argument type checks) is unchanged from upstream.
|
Good question. I diffed Node's
The vendored file is byte-for-byte identical to
For the other two fixes there is nothing upstream to import: With the push, all 17 of the vendored |
There was a problem hiding this comment.
I didn't find any bugs, but this bundles four separate fixes including native Rust changes to the fs.write argument parser (re-encoding/buffer ownership) and the readdir buffer conversion, so it's worth a human pass — particularly the node_fs.rs hunk and the sequencing with #32966/#30484.
Extended reasoning...
Overview
This PR fixes four distinct node:fs Node-compat divergences: (1) fs.write/writeSync/filehandle.write silently ignoring the string encoding argument — a data-corruption bug fixed by re-encoding the string in args::Write::from_js (src/runtime/node/node_fs.rs) and reworking how the position slot is consumed; (2) readdir({encoding:'buffer'}) returning Uint8Array[] instead of Buffer[], fixed by swapping to_js → to_node_buffer in ret::Readdir::to_js; (3) fs.Dir missing Symbol.dispose/Symbol.asyncDispose; (4) wrong error code for unrecognized flag strings in FileSystemFlags::from_js. The JS side also adds validateEncoding calls, switches $isTypedArrayView → isArrayBufferView in fs.write, and adds a non-string type check. A vendored Node test-fs-write.js is added with one documented local patch.
Security risks
None identified. The changes are Node-compat behavioral fixes to argument parsing, encoding, and error codes; no auth, crypto, permissions, or untrusted-input handling is touched beyond what already existed.
Level of scrutiny
Medium-high. The headline fix rewrites part of the native fs.write argument parser and replaces args.buffer mid-parse via StringOrBuffer::from_js_with_encoding, which has memory-ownership implications (the original StringOrBuffer is dropped and replaced). The to_node_buffer swap likewise changes how readdir entry bytes are handed to JSC. These are well-reasoned and well-tested, but they sit on hot, ownership-sensitive paths in production fs code where a subtle mistake would be a use-after-free or double-free rather than a test failure.
Other factors
Test coverage is strong (table-driven byte-exact tests against Node v26.3.0 output, plus the vendored upstream test-fs-write.js), CI is green except for unrelated macOS agent artifact-download timeouts, and the bug-hunting system found nothing. However, the PR bundles four independent fixes, explicitly overlaps with two other open PRs (#30484 and #32966) that will require rebase coordination, and the author notes leave a maintainer decision open (case-insensitive flag strings). That combination — native ownership-touching changes plus cross-PR sequencing — puts this outside the bar for bot-only approval.
The previous commit vendored Node's test-fs-write.js with a local "// Bun:" patch removing V8's --expose_externalize_string test helpers. test/js/node/test/parallel/CLAUDE.md says vendored Node tests must not be modified, and test/js/node/test/common/index.js already polyfills a V8-flag-exposed global in exactly this situation (--expose-gc becomes `globalThis.gc = () => Bun.gc(true)`), so do the same for --expose_externalize_string: createExternalizableString / createExternalizableTwoByteString / externalizeString / isOneByteString are provided there instead. JavaScriptCore has no string externalization to force, so the create and externalize helpers are identity and no-op; isOneByteString answers the representation-independent question V8's helper answers (are all code units <= 0xFF), which is true for "umlaut sechzig" and false for "Zhongwen 1", the two cases the test asserts. test-fs-write.js is now byte-for-byte identical to upstream, so future `bun node:test:cp` re-syncs stay clean, and any other vendored Node test that uses the flag works without a local patch.
…ncoding
Node's fsPromises write calls validateStringAfterArrayBufferView before
validateEncoding, so a non-string, non-ArrayBufferView value rejects
with ERR_INVALID_ARG_TYPE. The validateEncoding call added to
filehandle.write had no such guard (write and writeSync have it), so
filehandle.write({ length: 3 }, 0, "hex") rejected with
ERR_INVALID_ARG_VALUE instead: the C++ validator's object fall-through
reads .length off arbitrary objects, sees an odd value, and throws the
encoding error before the native parser can reject the type. Add the
same typeof-string check the two sibling entry points already perform,
in the same position Node performs it.
There was a problem hiding this comment.
Thanks for the fix in 27ee839 — that resolves the one issue I flagged. No further bugs found, but given this touches the native args::Write parser and re-encodes strings on a hot fs write path (plus three other independent fixes), it's worth a human pass before merge.
Extended reasoning...
Overview
This PR bundles four independent node:fs Node-compat fixes plus three more gaps surfaced by vendoring Node's test-fs-write.js:
fs.write/writeSync/filehandle.writeignore the string encoding — the headline fix, in native Rust (args::Write::from_jsinnode_fs.rs). Re-encodes the string buffer after the encoding argument is parsed, with a carve-out forEncoding::Utf8 | Encoding::Buffer. Also changes how the position slot is consumed (always advance, only use if non-negative number).readdirwith{encoding: 'buffer'}returnsUint8Array[]notBuffer[]— one-line swap fromto_jstoto_node_bufferinret::Readdir::to_js.fs.Dirnot disposable — addsSymbol.dispose/Symbol.asyncDisposeto the JSDirclass.- Unrecognized flag string throws wrong error code — three call sites in
types.rsswitched fromthrow_invalid_argumentstoErrorCode::INVALID_ARG_VALUE. - Three more
fs.writegaps infs.ts/fs.promises.ts: wire upvalidateEncoding, addtypeof buffer !== 'string'guard, switch$isTypedArrayView→types.isArrayBufferViewfor DataView support.
Plus: vendored test-fs-write.js (215 lines, byte-identical to upstream) and polyfills for V8's --expose_externalize_string globals in test/js/node/test/common/index.js.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input parsing changes. The changes tighten validation (more errors thrown for invalid input) and fix encoding application; they don't widen any attack surface.
Level of scrutiny
Medium-high. The native args::Write::from_js change is on a hot, critical I/O path used by every string write in node:fs. The re-encoding logic has subtle edge cases (the Encoding::Buffer carve-out, the position-slot consumption change, ownership of the re-encoded StringOrBuffer). The JS-side changes are more mechanical but spread across three entry points (write, writeSync, filehandle.write) that need to stay consistent — my earlier review caught one such inconsistency, now fixed.
The readdir Buffer swap, Dir dispose methods, and error-code changes are individually simple, but bundling four+ independent fixes into one PR increases the review surface.
Other factors
- Test coverage is strong: a table-driven test of
writeSyncargument shapes with byte-exact expectations from Node v26.3.0, async/filehandle variants, the vendored upstreamtest-fs-write.js, and targeted tests for each of the other three fixes. The author also confirmed the existing vendoredtest-fs-*suite still passes. - My prior feedback was addressed: commit 27ee839 added the missing
typeofguard tofilehandle.writeand a regression test covering all three entry points. - CI: the only repeated failure is an artifact-download timeout on specific macOS agents, unrelated to the diff.
- Overlap with #32966: the
types.rshunk is acknowledged as superseded by that PR; whichever lands second rebases it away. - The
common/index.jspolyfill follows the existing--expose-gcpattern in the same file and keeps the vendored test pristine, which matches the stated convention for that directory.
Given the breadth (native + JS, four independent behavior changes on core fs paths) and the subtlety of the encoding/position-slot parsing change, this exceeds my threshold for unreviewed approval even though I found no remaining issues.
## Problem
`node:fs` `open()` flag strings are matched case-insensitively. `"W"`
and `"A+"` are silently accepted (and create/open the file) where Node
throws `ERR_INVALID_ARG_VALUE`.
```js
import * as fs from "node:fs"; import * as os from "node:os";
const R = fs.mkdtempSync(os.tmpdir() + "/fsfl-");
for (const f of ["W", "A+"]) {
try { const fd = fs.openSync(R + "/flag_" + f, f); fs.closeSync(fd); console.log(f, "opened"); }
catch (e) { console.log(f, e.code); }
}
```
Node v26.3.0:
```
W ERR_INVALID_ARG_VALUE
A+ ERR_INVALID_ARG_VALUE
```
Bun (main):
```
W opened
A+ opened
```
A typo'd flag (`"W"` for `"w"`) is a programmer error that Node catches
immediately and Bun turns into a real file open with some mode.
## Cause
Node's
[`stringToFlags`](https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L733)
is a case-sensitive, strict-equality, exhaustive switch over 22
primitive strings; anything else is `ERR_INVALID_ARG_VALUE`.
`FileSystemFlags::from_js` in `src/runtime/node/types.rs` diverged in
four ways:
1. `FILE_SYSTEM_FLAGS_MAP` listed uppercase spellings (`"W"`, `"A+"`,
`"RS"`, ...) as explicit keys.
2. A leading digit routed the string through `parse_int`, so `"577"` /
`"0"` / `"65536"` became raw `open(2)` flags.
3. The strings it did reject got `ERR_INVALID_ARG_TYPE`, not
`ERR_INVALID_ARG_VALUE`.
4. It accepted `String` wrapper objects (`new String("w")`), which fail
Node's strict `switch`.
`mode_from_js` had the same class of gap against Node's
[`parseFileMode`](https://github.com/nodejs/node/blob/v26.3.0/lib/internal/validators.js#L104)
(regex the string as `/^[0-7]+$/`, `parseInt` it into a Number, then
`validateUint32`):
5. It stripped a `"0o"` prefix and let `parse_int` accept signs and `_`
separators, so `"0o755"`, `"+755"`, and `"7_5_5"` were accepted.
6. A valid octal string past `u32::MAX` collapsed into
`ERR_INVALID_ARG_VALUE`; Node range-checks with `validateUint32` and
throws `ERR_OUT_OF_RANGE`.
7. It called `ZigString::slice()`, the 8-bit-only view, on the primitive
string. A non-Latin-1 mode string is stored 16-bit by JSC, so this
tripped the view's debug assertion (`panic: ZigString::slice() on UTF-16
string; use to_slice()`) on debug builds, and in release read the first
`len` bytes out of a `2 * len`-byte UTF-16 buffer: `fs.openSync(p, "w",
"\u3737")` was accepted and created the file with mode `7` (the low byte
of `0x3737` is `0x37`, `"7"`) where Node throws.
## Fix
`src/runtime/node/types.rs`
- Drop the uppercase entries from `FILE_SYSTEM_FLAGS_MAP` and the
numeric-string fallback. Require a primitive string
(`is_string_literal`, not the string-like `is_string`) and reject any
non-matching flag with Node's `ERR_INVALID_ARG_VALUE` message.
- Read mode strings through `ZigString::to_slice()`, the
encoding-agnostic view, so bitness of the backing buffer is irrelevant
(the same helper the flag path already used). Validate as `[0-7]+` on a
primitive string, then route the parsed value through the same
`validators::validate_uint32` the numeric branch uses, mirroring Node's
structure exactly.
`src/jsc/bindings/ErrorCode.cpp`, `src/jsc/JSGlobalObject.rs`
- Node's `ERR_INVALID_ARG_VALUE` renders the received value with
`util.inspect` (`Received 'W'`), not `determineSpecificType` (`Received
type string ('W')`). The C++ `INVALID_ARG_VALUE` overloads already use
the right formatter (`JSValueToStringSafe(..., quotesLikeInspect)`); the
Rust side had no equivalent and would have used a divergent
double-quoting one. Add a small `Bun__ErrorCode__inspectForErrorMessage`
bridge so both sides share it.
`src/runtime/node/node_fs.rs`
- Delete the two now-dead `"Invalid flag"` `ok_or_else` re-throws in the
`readFile`/`writeFile` argument parsers: `FileSystemFlags::from_js` only
returns `None` for `null`/`undefined` now, and those sites already
`get_truthy` the property.
- Gate `Mkdir::from_js`'s positional-mode branch on
`is_string_literal()`. Node branches on `typeof options === 'object'`,
so a `String` wrapper is an options bag there and is never parsed as a
mode.
## Deliberate user-visible behavior changes
These are all node-matching, but a reviewer should see them spelled out:
- Previously-accepted flag strings (`"W"`, `"A+"`, `"RS"`, `"577"`,
`"0o644"`, `new String("w")`, ...) and mode strings (`"0o755"`,
`"+755"`, `"7_5_5"`, `"\u3737"`, ...) now throw `ERR_INVALID_ARG_VALUE`.
- An octal mode string past `u32::MAX` (`"777777777777"`) now throws
`ERR_OUT_OF_RANGE` instead of `ERR_INVALID_ARG_VALUE`.
- `chmodSync(p, new String("755"))` / `fchmodSync` now throw
`ERR_INVALID_ARG_TYPE` (previously accepted); this is Node's
`parseFileMode` behavior.
- `mkdirSync(p, new String("755"))` now uses the default mode instead of
parsing the wrapper's content as `0o755`; this is Node's behavior (it
treats the wrapper as an options bag). Note Bun had already diverged
from Node here before this PR (wrong mode rather than a throw).
## Intentionally excluded
- `fs.open(path, { flags, mode })` treating an object second argument as
an options bag is a separate Bun extension (Node has no such overload);
unchanged.
- `readFileSync(p, { flag: "" })` silently defaults where Node throws.
Pre-existing and a different layer (options-bag falsy handling). Node's
own `readFileSync` and `writeFileSync` disagree with each other here
(`writeFileSync` does `options.flag || 'w'`, the same falsy-defaults
rule as Bun's), so the fix is per-function and left for a follow-up
rather than folded into this one.
- `mode_from_js` returns `mode_int & 0o777`, so Bun drops the
setuid/setgid/sticky bits Node keeps (Node's `"7755"` lands on disk as
`7755`, Bun's as `755`). Pre-existing, untouched by this PR, and it
affects every mode consumer.
- Node v26.3.0 crashes on an internal `args[2]->IsInt32()` assertion for
mode strings whose value lands in `(i32::MAX, u32::MAX]`, e.g.
`"37777777777"` (exactly `u32::MAX`) aborts the node process. That is an
upstream Node bug, not a contract to replicate; Bun keeps accepting
those values and a test pins it.
## Verification
75 new tests in `test/js/node/fs/fs.test.ts`: every valid flag spelling;
the uppercase / numeric / empty / boolean / `String`-wrapper invalid
cases across `openSync`, callback `open`, `promises.open`,
`readFileSync`, and `writeFileSync` (asserting Node's exact message via
`util.inspect`); and the mode cases across `openSync`, `mkdirSync`, and
`chmodSync`, including the overflow, 16-bit-storage, and
`String`-wrapper boundaries. 38 of them fail on an unpatched build; 0
after. Every assertion was checked against Node v26.3.0 directly rather
than taken from analysis.
<details>
<summary>Additional verification</summary>
- The Node upstream tests `test-fs-readfile-flags`,
`test-fs-open-numeric-flags`, `test-fs-open`, `test-fs-open-mode-mask`,
and `test-file-validate-mode-flag` all pass.
- The new error paths are clean under
`BUN_JSC_validateExceptionChecks=1`, including a `Proxy` whose traps
throw during value inspection.
- The 16-bit regression test is umask-independent: it builds a 16-bit
`"755"` via `TextDecoder("utf-16le")` and asserts it produces the same
on-disk mode as the 8-bit `"755"` literal in the same process. It fails
against an unpatched debug build (assertion panic), an unpatched release
build (the raw byte read sees the interleaving `0x00` bytes and
rejects), and an intermediate revision of this PR that wrongly rejected
all 16-bit strings.
- The `mkdirSync` wrapper test compares against an options-less
`mkdirSync` in the same process for the same reason: under the default
umask, Node's options-bag default (`0o777`) and a parsed `0o755` both
land on disk as `755`, so an absolute-mode assertion cannot distinguish
them.
</details>
Supersedes #32330, which narrowed the accepted range of the
numeric-string flag path that this PR removes entirely (Node rejects
every numeric-string flag, not only out-of-range ones); its boundary
values are carried forward as test cases here. #32813 has since merged
to main, and this branch is rebased on top of it. The rebase produced
exactly one merge conflict: #32813's `types.rs` hunk (which changed the
error code on rejection while keeping the lenient acceptance). It was
resolved in favor of this PR's rewrite of `FileSystemFlags::from_js`,
which is a strict superset of that hunk (it produces the same
`ERR_INVALID_ARG_VALUE`, with Node's exact message, for those inputs and
for the ones #32813 still accepted). The rest of #32813 (the
write/writeSync string-encoding fix) is in separate code and is fully
preserved.
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Jarred Sumner <[email protected]>
fs.writeSync(fd, "XY", null, "bogus") threw ERR_INVALID_ARG_VALUE and wrote nothing. fs.write and filehandle.write did the same. Node.js through v26 writes the string as UTF-8. For a null or undefined position, Bun 1.3.14 wrote UTF-8 too, and 1.4.0 started to throw (#32813). args::Write::from_js parsed the encoding with Encoding::assert, the port of Node's assertEncoding. Node's WriteString reads that argument with ParseEncoding(args[3], UTF8), which never throws. The parser now does the same: a primitive string that names a known encoding selects it, and every other value means UTF-8. Node.js v27 rejects an unknown name in its JS validateEncoding (nodejs/node#66247). That check is not part of this change.
Four
node:fsdivergences from Node, found by a differential run against Node v26.3.0. The headline is silent data corruption: theencodingargument offs.writeSync(fd, string, position, encoding)was parsed and then ignored.1.
fs.write/fs.writeSync/filehandle.writeignore the string encodingSame for
hex,base64,latin1,ascii, and for the asyncfs.write(fd, string, position, encoding, cb)andfilehandle.write(string, position, encoding), which share the argument parser.Cause:
args::Write::from_jsinsrc/runtime/node/node_fs.rsparses the encoding intoargs.encoding, but the string was already converted to UTF-8 byStringOrBuffer::from_jsa few lines earlier, andwrite_inner/pwrite_inneronly ever look atargs.buffer.slice(). A second bug hid the first for some spellings: the parser only advanced past the position slot when it held a number, sofs.writeSync(fd, "abc", null, "utf16le")never even reached the encoding argument. Node'sWriteStringbinding takes the position fromargs[2]regardless of its type and the encoding strictly fromargs[3].Fix: consume the position slot whatever its type (using it only when it is a number, and only when non-negative, matching the sibling Buffer arm), then, once the encoding is known, re-encode the string with
StringOrBuffer::from_js_with_encoding. That helper is the same pathwriteFileSync(p, s, "utf16le")(which already worked) andBuffer.from(s, enc)use, and it already matches Node byte-for-byte for every encoding. The one encoding name the re-encode must not apply to is"buffer": Node'sStringBytes::WritetreatsBUFFERthe same asUTF8, while the in-tree string encoder'sBufferarm is a verbatim latin1 / low-byte copy, sowriteSync(fd, "\u00e9", 0, "buffer")would have written 1 byte instead of Node's 2. The re-encode therefore skips bothEncoding::Utf8andEncoding::Buffer, keeping the original UTF-8 conversion for both, and the test table covers the"buffer"name for both 8-bit and 16-bit source strings. The one exception is the"buffer"encoding name: Node writes UTF-8 for it (writeSync(fd, "\u00e9", 0, "buffer")writesc3 a9), while the encoder'sBufferarm is a verbatim latin1 / low-byte copy, so it is excluded from the re-encode and keeps the original UTF-8 conversion.2.
fs.readdirwith{ encoding: "buffer" }returnsUint8Array[], notBuffer[]Cause:
ret::Readdir::to_jsusedMarkedArrayBuffer::to_js, which builds a plainUint8Array. The siblingStringOrBuffer::to_js(used byreadFile,readlink,realpath) already usesto_node_buffer, which goes throughJSBufferSubclassStructure.Fix: use
to_node_bufferthere too. Both transfer ownership of the mimalloc-backed bytes to JSC via the same deallocator, so it is a drop-in. This covers the sync, async, callback, and recursive variants, which all funnel throughReaddir::to_js.3.
fs.Diris not disposableNode's
Dirprototype hasSymbol.disposeandSymbol.asyncDispose(and both are no-ops when the directory is already closed, so they compose with an explicitclose()).Fix: add both to the
Dirclass insrc/js/node/fs.ts, delegating tocloseSync()/close()behind an already-closed guard, using the samethis.#handle >= 0check the class'sSymbol.asyncIteratoralready uses.4. An unrecognized flag string throws
ERR_INVALID_ARG_TYPENode's
stringToFlagsthrowsERR_INVALID_ARG_VALUEfor every unrecognized flag string (including the empty string).FileSystemFlags::from_jsused theERR_INVALID_ARG_TYPEhelper. Code that switches onerr.codegets the wrong branch.Fix: switch the three string-flag error sites to
ErrorCode::INVALID_ARG_VALUE. The messages are unchanged.5. Three more
fs.writegaps, surfaced by vendoring Node'stest-fs-write.jsNode's
test/parallel/test-fs-write.jswas missing from the vendored Node test suite. It is Node's own regression test for this exact bug class (its nodejs/node#38168 block assertsfs.writeSync(fd, 'abcd', 0, 'hex')returns 2). Vendoring it surfaced three more gaps onfs.write's string path, all fixed here:validateEncodingwas never called. Node rejects an odd-length string written with thehexencoding (ERR_INVALID_ARG_VALUE,is invalid for data of length N) fromwrite,writeSync, andfilehandle.write. The C++ validator already existed inNodeValidator.cppand was exported frominternal/validatorswith no callers; this wires it into the three string write paths where Node calls it. Messages are byte-identical to Node v26.3.0.fs.writewas missingwriteSync'stypeof buffer !== "string"check, sonew String('x')(and every other non-string, non-view value) reached the native parser through the string signature instead of throwingERR_INVALID_ARG_TYPE.fs.writebranched on$isTypedArrayView, which excludesDataView, sofs.write(fd, dataView, offset, length, cb)was silently parsed as the(position, encoding)string signature. It now uses Node'sisArrayBufferViewpredicate, matching thewriteSyncsibling one function below.Tests
test/js/node/fs/fs.test.ts: a table ofwriteSync(fd, string, ...)argument shapes whose expected bytes were each produced by Node v26.3.0; the asyncfs.write+filehandle.writevariants;readdir{ encoding: "buffer" }across the sync / recursive / promise / callback entry points; the flag error code foropenSyncandopen.test/js/node/fs/dir.test.ts:using,await using, and disposing an already-closedDir.test/js/node/test/parallel/test-fs-write.js: Node's upstream test, vendored viabun node:test:cp. On the unfixed build it fails on its first assertion ('ümlaut sechzig'written withlatin1reads back as'ümlaut sechzig'); with the fix it exits 0. The file is byte-for-byte identical tonodejs/node@mainwith no local patch. Upstream runs it under V8's--expose_externalize_stringflag; thecreateExternalizableString/externalizeString/isOneByteStringhelpers it needs are V8 test globals with no JavaScriptCore equivalent (present upstream since at least Node v22), so they are polyfilled intest/js/node/test/common/index.jsnext to the existing--expose-gcpolyfill. The test file stays unmodified, and any other vendored Node test that uses that flag works without a local patch.All fail on the current build and pass with the fix. The vendored Node suites
test-fs-write-sync,test-fs-write-sync-optional-params,test-fs-write-optional-params,test-fs-write-buffer,test-fs-open,test-fs-open-numeric-flags,test-fs-readdir*,test-fs-opendir, andtest-fs-promises-file-handle-{write,append-file,dispose}still pass.Out of scope, noted for completeness
readdirSync(dir, { withFileTypes: true, encoding: "buffer" }): Node returnsDirent[]withBuffernames; Bun ignoreswithFileTypeswhen the encoding isbufferand returns a flat array (now ofBuffers instead ofUint8Arrays). Matching Node needs aDirentvariant whosenameis aBuffer, a larger change than the one-line swap here. That half isfs.readdir({ withFileTypes: true, encoding: 'buffer' })returnsUint8Array[]instead ofDirent[]#30482, and fs.readdir: return Dirent with Buffer name when encoding is "buffer" #30484 already has a fix open for it (theDirentname representation, theargs::Readdir::tagprecedence, andNodeDirent.cpp). The two PRs touch disjoint code: fs.readdir: return Dirent with Buffer name when encoding is "buffer" #30484 does not changeret::Readdir::Buffersconversion, and this PR does not touchDirent, so neither replaces the other."A+"opens; Node throwsERR_INVALID_ARG_VALUE). The uppercase aliases are hand-enumerated inFILE_SYSTEM_FLAGS_MAPand date back to the original implementation, so I left that deliberate leniency for a maintainer to decide on rather than silently tightening it. node:fs: validate open() flag and mode strings like Node #32966 now does exactly that: it rewritesFileSystemFlags::from_jsto match Node'sstringToFlags(case-sensitive, exhaustive, alwaysERR_INVALID_ARG_VALUE) and subsumes the smalltypes.rserror-code change in this PR, so whichever of the two lands second rebases that hunk away. The rest of this PR is independent of node:fs: validate open() flag and mode strings like Node #32966.