node:fs: report an invalid encoding before an invalid path

[robobun]

Problem

• fs.readFileSync(123n, "bogus") has an invalid path and an invalid encoding. Bun throws TypeError [ERR_INVALID_ARG_TYPE]: path must be a string or a file descriptor. Node v26.3.0 throws ERR_INVALID_ARG_VALUE for the encoding.
• The same holds for writeFile, appendFile, readdir, readlink, realpath, mkdtemp and watch, in every form, for every kind of invalid path.
• Cause: each argument parser in src/runtime/node/node_fs.rs throws the path error at once. Node runs getOptions(options), which asserts the encoding, before getValidatedPath(path).

Fix

• When the path is invalid, the parser now asserts the encoding before it reports the path error (or_encoding_error). An invalid encoding wins, as in Node.
• The parsers still read the path first. A test from PinnedArrayBuffer: copy a resizable ArrayBuffer whose bytes are read after the call #40641 pins that order: a resizable ArrayBuffer path is captured before an encoding getter can shrink it. A call with a valid path runs as before.
• fs.watch validates a URL path and ignore in its JS wrapper. The wrapper now asserts the encoding first, so it wins over the other options too.
• Verified: test/js/node/fs/fs.test.ts, 27 new tests. All fail on canary 1.4.3. Node v26.3.0 passes all 426 calls of the tables.

Background

• getOptions (lib/internal/fs/utils.js) is Node's helper for options. It turns a string into { encoding } and asserts the encoding.
• ArgumentsSlice is the cursor of the fs bindings over the JS arguments. The new peek_at(index) reads options while the cursor is on the path.
• take_exception removes the pending JS exception and returns it. throw_value throws it again. The helper holds the path error that way during the encoding check.

Notes

Repro (Node v26.3.0 prints ERR_INVALID_ARG_VALUE four times, canary 1.4.3 prints ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_TYPE, ERR_INVALID_URL_SCHEME, ENAMETOOLONG):

const fs = require("node:fs");
const t = f => { try { f(); } catch (e) { console.log(e.code); } };
t(() => fs.readFileSync(123n, "bogus"));
t(() => fs.mkdtempSync({}, { encoding: "bogus" }));
t(() => fs.watch(new URL("http://example.com/x"), "bogus"));
t(() => fs.readdirSync("a".repeat(70000), "bogus"));

No user reported this. A differential run against Node found it.

Node source. readlinkSync shows the order in four lines: https://github.com/nodejs/node/blob/v26.3.0/lib/fs.js#L1761-L1764. getOptions is at https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L324-L345.

Kinds of invalid path in the test. A bigint, an object, null, undefined, a boolean, a symbol, a string with a NUL byte and an http: URL, each with "bogus" and with { encoding: "bogus" }. A bad fd (-1, 1.5, NaN), an empty Uint8Array and a path that is too long behave the same way under Node and under this branch. I checked those by hand.

fs.watch with a second invalid option. Node reports the encoding before persistent, recursive, signal and ignore too, with a valid path or an invalid one. fs.watch(dir, { encoding: "bogus", ignore: 5 }) threw the ignore error in Bun, and { encoding: "bogus", persistent: 1 } threw the persistent error. One test covers 10 such calls. The native watcher parser is unchanged: the JS wrapper asserts the encoding before it reaches that parser.

Probe. 32 calls x 7 invalid paths x 3 invalid encodings is 672 calls. Canary 1.4.3 matches Node on 129. This branch matches on 597. The 75 that still differ:

• opendirSync, opendir and fs.promises.opendir (54 calls). Node checks the path first there, and Bun checks the encoding first. The cause is in src/js/node/fs.ts, not in these parsers. It is tracked as a separate task.
• fs.promises.watch (21 calls). Node returns an async iterator and reports every error on the first next(). Bun throws at the call. fs.promises.watch: implement as a real async generator #34718 covers that.

Why the parsers do not assert the encoding first. My first version did that. It broke the test sync fs calls read a Buffer path captured at call time when an option getter shrinks its resizable ArrayBuffer: the encoding getter ran before the parser captured the path. This version keeps that test green. It also keeps the encoding getter at one read per call.

Also checked on this branch. A throwing encoding getter wins over an invalid path, as in Node. An invalid path with a valid encoding throws the same path error as before, with its stack. The probes run clean under BUN_JSC_validateExceptionChecks=1.

Self-review. I reviewed the diff by hand. One concern came up and is fixed: the pending exception can be the termination exception of the VM, which must keep unwinding. or_encoding_error returns at once in that case (has_pending_termination_exception), as fetch and the Valkey client do.

Not changed here: data before the path. fs.writeFileSync(123n, 456) reports the path in Bun and data in Node (also the callback and promise forms). Node validates data before it opens the path. That order does not involve the encoding, so it is not part of this PR.

Not changed here. getOptions also validates signal before the path, and it rejects an options value that is not a string or an object. #41928 adds that second check to parse_encoding_arg. With this PR it then runs before the path error too, because or_encoding_error calls parse_encoding_arg. The signal check keeps its place after the path.

utf-16le. Bun's ENCODING_MAP does not know Node's utf-16le alias (#43373 fixes that). Until it lands, fs.readFileSync(123n, "utf-16le") reports the encoding on this branch, and Node reports the path. With a valid path that call already throws the encoding error on main.

Related PRs. #41928, #39330 and #42812 edit the options code of the same parsers. This PR does not touch those lines, only the path line above them. #38432 changes the message of the encoding error. The new tests accept both messages: they check the code, the name, and that the message names the encoding.

Suites run with the debug build. test/js/node/fs/fs.test.ts (591 pass, 8 skip, 0 fail), test/js/node/watch/fs.watch.test.ts, promises.test.js, dir.test.ts, fs-path-length.test.ts, fs-promises-writeFile-async-iterator.test.ts. 37 vendored Node tests pass, among them test-fs-assert-encoding-error.js, test-fs-read-file-assert-encoding.js, test-fs-null-bytes.js, test-fs-whatwg-url.js, test-fs-error-messages.js, test-fs-watch-encoding.js, test-fs-mkdtemp.js, test-fs-readdir.js, test-fs-readfile.js, test-fs-realpath.js and test-fs-promises.js. abort-signal-leak-read-write-file.test.ts hits its 300 s limit on a debug build (100,000 iterations). A 3,000 iteration run of the same loop leaves 3 AbortSignal objects.

---

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

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for one more day.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 3 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: cc2ae238-d3fb-4f90-918a-77f35a592358

📥 Commits

Reviewing files that changed from the base of the PR and between 9b7c982 and 9237368.

📒 Files selected for processing (4)

• src/js/internal/fs/watch.ts
• src/jsc/CallFrame.rs
• 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 with the script in the description. fs.readFileSync(123n, "bogus") throws ERR_INVALID_ARG_VALUE (the encoding) under Node v26.3.0 and ERR_INVALID_ARG_TYPE (the path) under Bun canary 1.4.3.
• The 27 new tests in test/js/node/fs/fs.test.ts fail on canary and pass on this branch. The same tables pass 426 of 426 calls under Node v26.3.0.
• Review findings so far: the fs.watch option order is fixed in 745dd62. The data before path order in writeFile and the utf-16le alias (node: accept the utf-16le encoding alias and reject utf16-le #43373) are named in the Notes and not changed here.
• CI: build 118661 has no failed test. 179 of 181 jobs passed and 2 wait for an agent. The red tests of build 118604 were timeouts on the x64-asan lane in tests that this change does not touch (sourcetextmodule-leak, require-cache, html-rewriter-leak, setTimeout). fs.test.ts passed on every lane in both builds.
• The sibling change from the same comparison with Node is node:fs: write UTF-8 when fs.write gets an unknown encoding name #43435 (fs.write with an unknown encoding name). The two PRs do not touch the same lines.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟣 src/runtime/node/node_fs_watcher.rs — fs.watch callers with a valid path, a bogus encoding and a non-boolean persistent (or verbose) still get the persistent error after merging, while Node reports ERR_INVALID_ARG_VALUE for the encoding. The PR only asserts the encoding on the invalid-path branch (node_fs_watcher.rs:662); the valid-path branch at node_fs_watcher.rs:675-698 validates persistent and verbose before reading encoding. Fix: read and assert encoding before persistent/verbose/recursive in Arguments::from_js so Node's getOptions-first order holds for every invalid option, not only an invalid path.

  Extended reasoning...

  Node's fs.watch runs getOptions(options) first (assertEncoding), then kFSWatchStart validates the path, then validateBoolean(persistent) and validateBoolean(recursive). Bun: watch.ts:168 calls native fs.watch(path, options, listener). Arguments::from_js at node_fs_watcher.rs:655 parses the path; on success or_encoding_error at :662 is a no-op. Line 675 reads persistent and throws 'persistent must be a boolean' at :679 before encoding is read at :695. So fs.watch(dir, { encoding: 'bogus', persistent: 'yes' }) throws the persistent error in Bun and the encoding error in Node. The same applies to verbose at :686. The PR's stated goal is that an invalid encoding wins as in Node, and REVIEW.md requires the whole class (every check that getOptions runs before) be fixed in the same PR. The dismissing finder confirmed the lines are unchanged from base but did not weigh that this is the same class the PR claims to close and the new test matrix only covers invalid paths for watch. Population: any watch caller with two bad options; the new tests would not detect the gap. Remedy: hoist the encoding…

  Verification: pre-existing. Trigger: fs.watch(validDir, { encoding: "bogus", persistent: 1 }) (or any truthy non-boolean persistent/verbose). Mechanism verified in /home/claude/bun/src/runtime/node/node_fs_watcher.rs: or_encoding_error at :662 only asserts the encoding when the path parse failed — in /home/claude/bun/src/runtime/node/node_fs.rs:3220-3222 `let Err(bun_jsc::JsError::Thrown) = path…

[robobun]

^{Updated 6:04 PM PT - Sep 19th, 2026}

✅ @robobun, your commit 9237368a09e5ce66f07e8abbd8e0069e4e505e89 passed in Build #118661! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 43563

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

bun-43563 --bun

[claude]

I reviewed the latest push (the fs.watch rework that moves the encoding assertion into the JS wrapper and drops the native watcher change) and found no bugs; since this manipulates pending-exception state in the native fs argument parsers, a human look is still worthwhile.

What was reviewed:

• or_encoding_error in src/runtime/node/node_fs.rs: the termination guard runs before take_exception, a throwing encoding getter propagates via ? (path error dropped), and throw_value re-throws only when nothing is pending — consistent with JSGlobalObject::throw_value/take_exception semantics.
• peek_at offsets: 1 for path/options parsers and 2 for writeFile/appendFile (path, data, options); slices start at argument 0 in node_fs_binding.rs, and a callback in the options slot parses as a plain object with no encoding, so the default is used.
• watch.ts: assertEncoding(options?.encoding) after the string/function normalization but before URL conversion; top-level $newRustFunction matches the dgram.ts pattern, and the net diff no longer touches node_fs_watcher.rs.

Extended reasoning...

Overview

The PR changes the error-precedence contract of node:fs argument parsing so that an invalid encoding option is reported before an invalid path, matching Node's getOptions()-before-getValidatedPath() order. It touches src/runtime/node/node_fs.rs (six from_js parsers plus the new or_encoding_error helper and a signature change to parse_encoding_arg), adds ArgumentsSlice::peek_at in src/jsc/CallFrame.rs, moves the fs.watch encoding assertion into src/js/internal/fs/watch.ts, and adds a table-driven describe block to test/js/node/fs/fs.test.ts covering sync, callback, and promise forms. The second push removed the earlier node_fs_watcher.rs change and the try/catch around fileURLToPath, so the net diff is now four files.

Security risks

None identified. The change only affects which of two argument-validation errors is surfaced first; it does not add new input processing, allocation, or I/O before validation. The path is still captured before the encoding getter can run, which preserves the resizable-ArrayBuffer capture behavior pinned by the existing test from #40641. The JS-side assertEncoding call goes through a native intrinsic rather than user-overridable machinery.

Level of scrutiny

Moderate. The interesting part is or_encoding_error, which takes a pending exception off the VM, runs user-reachable JS (an encoding getter), and re-throws. I checked that has_pending_termination_exception is consulted before take_exception, that a getter error propagates cleanly via ? while the held path_error JSValue is simply dropped, and that throw_value will not clobber an exception that is already pending. The peek_at index for WriteFile/AppendFile (2) matches the (path, data, options) layout, and slices are initialized from argument 0 in node_fs_binding.rs. This is subtle enough that I would not claim a human never needs to look, but it is small and well-contained.

Other factors

The prior inline note about writeFile/appendFile reporting the path error before an invalid data argument (Node checks data first) was flagged as pre-existing and non-blocking; the latest push does not change that behavior, and it is not restated here. The new tests assert code, name, and that the message names the encoding, across 8 invalid path kinds x 2 option shapes for 26 entry points, and the fs.watch test covers a second invalid option with both a valid and an invalid path. The bug-hunting run ended on dry_streak with no findings. No CODEOWNERS entry covers the changed paths.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[robobun]

On the fs.watch finding (a bogus encoding together with a non-boolean persistent or verbose): confirmed, and fixed in 745dd62.

• Node v26.3.0 reports the encoding before persistent, recursive, signal and ignore, with a valid path or an invalid one. Bun threw the persistent, verbose or ignore error first.
• The fix is in the JS wrapper, not in Arguments::from_js. The wrapper validates a URL path and ignore before it reaches the native parser, so a change to the native order alone still leaves fs.watch(dir, { encoding: "bogus", ignore: 5 }) wrong. The wrapper now asserts the encoding first. The native watcher parser is unchanged again.
• New test: watch, with a second invalid option (10 calls, all pass under Node v26.3.0, 8 fail on canary 1.4.3).

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.