node:fs: validate rm options before the ERR_FS_EISDIR check

[robobun]

Problem

• fs.rmSync(dir, { force: "x" }) throws ERR_FS_EISDIR (Path is a directory: rm returned EISDIR (is a directory) <dir>). Node v26.3.0 throws ERR_INVALID_ARG_TYPE (The "options.force" property must be of type boolean. Received type string ('x')). Same for any other invalid option (recursive: 0, an own recursive: undefined, maxRetries: "x", retryDelay: -1, ...) and for a non-object options (rmSync(dir, "x")), and through all three entry points: fs.rm delivers the EISDIR to the callback and fs.promises.rm rejects with it.
• Cause: the JS wrappers (rmSync in src/js/node/fs.ts, rm in src/js/node/fs.promises.ts, which the callback fs.rm also went through) lstat the target and throw ERR_FS_EISDIR first, and only then call the native fs.rm, which is where the options were validated (args::Rm::from_js in src/runtime/node/node_fs.rs). Node's validateRmOptions validates the options before its lstat, so with a directory target the option error wins there. With a file target the native validation was reached and Bun already reported the type error, so this is purely the wrappers' ordering.
• Smaller consequence of the same structure: node's callback fs.rm(path, badOptions, cb) throws synchronously; Bun handed the error to the callback because fs.rm was implemented as promises.rm(...).then(...).

Fix

• fs.promises.ts: port node's validateRmOptions (validateObject, spread over the defaults, then force, recursive, retryDelay as int32 >= 0, maxRetries as uint32, in node's order) and run it before the lstat / ERR_FS_EISDIR check in all three flavors: validateRmOptionsSync (used by rmSync) and rmValidated (the async lstat + native call, used by promises.rm and callback rm). The two copies of the EISDIR check in fs.ts and fs.promises.ts collapse into these; they are shared with fs.ts through the module's existing private $data export. The fixing change is the order: options first, lstat second.
• fs.ts: rmSync becomes fs.rmSync(path, validateRmOptionsSync(path, options)); callback rm calls validateRmOptions synchronously (so invalid options throw, as in node) and then rmValidated.
• Why this is right: for node:fs node's behavior is the contract, and this is node's own structure (options, then lstat, then remove), using the shared validators that already render node's messages. Accepted inputs do not change except in the node direction: the native parser already rejected everything validateRmOptions rejects, apart from an array or function as the options bag and retryDelay above int32 range, which node rejects too. The normalized object handed to the native binding is the same set of own keys the wrappers passed before (the old { ...options } spread), so the native parser, fs.rmdir, and the internal callers that pass literals to the binding are unaffected. The native validation and its test-fs-rm.js hook are left in place: the binding still validates its own input, and node:fs: report mkdir/readdir/rm boolean option type errors like node #39328 is fixing its messages; the native rm also still produces node's force/ENOENT (syscall: "lstat") behavior, which is why an lstat failure in the wrappers is still left for it to report.
• Verified:
  • test/js/node/fs/promises.test.js, new rm option validation runs before the ERR_FS_EISDIR check block: 12 option cases (each checked through rmSync, callback rm throwing synchronously, and promises.rm, asserting error class, code and node's message text, and that the directory survives), plus a case that valid options still remove files, still report ENOENT/lstat and still report ERR_FS_EISDIR. The 12 cases fail on the released build, all pass with this change; the whole file passes.
  • A 240-result matrix (20 option values x directory / file / missing / non-string path x 3 entry points) against a local node v26.3.0: 35 results matched node before, 197 after. The 43 still differing are all in the non-string-path column and are not touched here: Bun's native path error has its own text, and in the callback and promise forms node validates the path before the options (rmSync matches node: options first). Excerpt below.
  • Vendored test-fs-rm.js, test-fs-rmSync-special-char.js, test-fs-rmdir-*.js, test-fs-promises.js, test-fs-error-messages.js pass; test/js/node/fs/fs.test.ts (512 tests) and the rest of test/js/node/fs pass, except abort-signal-leak-read-write-file.test.ts, whose 100k-iteration fixture needs about 7 minutes on this debug+ASAN container against a 5 minute budget and does not involve rm.
• Related open PRs: node:fs: report mkdir/readdir/rm boolean option type errors like node #39328 fixes the native parser's messages (its rm tests accept either delivery channel and assert node's text, so the two land in either order); fs: run node:fs callbacks from the event loop, not a promise reaction #33366 rewrites the same callback rm wrapper for a different reason (callback dispatch); the overlap is the few lines of that wrapper, and its rm arm would keep the validateRmOptions call up front.

Background

• ERR_FS_EISDIR is node's JS-level refusal to rm a directory without recursive: true. It is not produced by the OS: node's validateRmOptions (lib/internal/fs/utils.js) lstat's the path and throws it itself, after validating the options; the removal only runs afterwards. Bun's native rm does not perform this check, which is why the wrappers do it in JS.
• In node, { ...defaults, ...options } is what makes an own key holding undefined an error (rmSync(p, { recursive: undefined }) is rejected, an absent key is not). Bun's native parser mirrors that with own-property lookups, and the normalized object the wrappers now pass has all four keys as own properties.
• $data on the node:fs/promises export object is a private-symbol property (invisible to user code) already used to hand FileHandle and its symbols to internal/fs/streams and node:http2; node:fs requires node:fs/promises at load, so sharing the helpers this way loads nothing extra on the rm path.

Matrix excerpt (directory target; node v26.3.0 / before / after)

rmSync(dir, { force: "x" })
  node:   ERR_INVALID_ARG_TYPE: The "options.force" property must be of type boolean. Received type string ('x')
  before: ERR_FS_EISDIR: Path is a directory: rm returned EISDIR (is a directory) <base>/dir
  after:  ERR_INVALID_ARG_TYPE: The "options.force" property must be of type boolean. Received type string ('x')

rmSync(dir, { recursive: 0 })
  node:   ERR_INVALID_ARG_TYPE: The "options.recursive" property must be of type boolean. Received type number (0)
  before: ERR_FS_EISDIR: Path is a directory: rm returned EISDIR (is a directory) <base>/dir
  after:  ERR_INVALID_ARG_TYPE: The "options.recursive" property must be of type boolean. Received type number (0)

rmSync(dir, { maxRetries: "x" })
  node:   ERR_INVALID_ARG_TYPE: The "options.maxRetries" property must be of type number. Received type string ('x')
  before: ERR_FS_EISDIR: Path is a directory: rm returned EISDIR (is a directory) <base>/dir
  after:  ERR_INVALID_ARG_TYPE: The "options.maxRetries" property must be of type number. Received type string ('x')

rmSync(dir, "x")
  node:   ERR_INVALID_ARG_TYPE: The "options" argument must be of type object. Received type string ('x')
  before: ERR_FS_EISDIR: Path is a directory: rm returned EISDIR (is a directory) <base>/dir
  after:  ERR_INVALID_ARG_TYPE: The "options" argument must be of type object. Received type string ('x')

fs.rm(file, { force: "x" }, cb)
  node:   thrown synchronously: ERR_INVALID_ARG_TYPE: The "options.force" property must be of type boolean. Received type string ('x')
  before: passed to cb:         ERR_INVALID_ARG_TYPE: The "options.force" property must be of type boolean.
  after:  thrown synchronously: ERR_INVALID_ARG_TYPE: The "options.force" property must be of type boolean. Received type string ('x')

The fs.rm callback and fs.promises.rm rows for the directory target are the same as the rmSync rows above, before and after.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 2 minutes

Limit details: You’ve used all 5 included reviews currently available under your plan.

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: 0553cac1-42c0-4d8e-b2f7-e6daa88a76dc

📥 Commits

Reviewing files that changed from the base of the PR and between aec33f5 and 25a0c19.

📒 Files selected for processing (3)

• src/js/node/fs.promises.ts
• src/js/node/fs.ts
• test/js/node/fs/promises.test.js

---

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

[robobun]

Status: reproduced, fixed, CI green (Buildkite build 99329); ready for review.

• Reproduced on the released build and on a debug build of main (aec33f581d): with an existing directory as the target, fs.rmSync, fs.rm and fs.promises.rm all report ERR_FS_EISDIR for { force: "x" }, { recursive: 0 }, { maxRetries: "x" }, { retryDelay: "x" } and "x" as options; node v26.3.0 reports ERR_INVALID_ARG_TYPE / ERR_OUT_OF_RANGE for each.
• Test: test/js/node/fs/promises.test.js, rm option validation runs before the ERR_FS_EISDIR check (12 cases fail on the released build, pass with this branch).
• The native parser's messages are a separate change, node:fs: report mkdir/readdir/rm boolean option type errors like node #39328; the two are independent.

[claude]

LGTM — focused Node-compat fix that reorders rm option validation ahead of the ERR_FS_EISDIR lstat check.

What was reviewed

• validateRmOptions matches node's order (validateObject → spread over defaults → force/recursive/retryDelay/maxRetries); confirmed validateObject rejects null, arrays, and callables (NodeValidator.cpp:641).
• Callback fs.rm still validates the callback before options, and the 2-arg overload (options = undefined) hits the early-return default path.
• Internal callers (mkdtempDisposable/mkdtempDisposableSync) call the native binding directly with literals and bypass the new validator.
• Tests: 12 option cases × 3 entry points with exact message assertions, plus positive coverage that valid options still remove, ENOENT still reports syscall: "lstat", and ERR_FS_EISDIR still fires.

Extended reasoning...

Overview

This PR fixes error-reporting order in fs.rm/fs.rmSync/fs.promises.rm: node validates the options bag before lstat'ing the target, so an invalid option should produce ERR_INVALID_ARG_TYPE/ERR_OUT_OF_RANGE even when the target is a directory. Bun was doing the lstat first and reporting ERR_FS_EISDIR instead. The fix ports node's validateRmOptions into fs.promises.ts, factors the two duplicated lstat/EISDIR blocks into shared helpers (validateRmOptionsSync, rmValidated, rmEisdirError), and shares them with fs.ts via the existing private $data symbol. A secondary fix: callback fs.rm now throws invalid-option errors synchronously (as node does) rather than routing them through the callback.

Security risks

None. This is argument-validation ordering in a Node-compat wrapper; no new syscalls, no path handling changes, no privilege boundaries. The lstat and native rm calls are the same ones the previous code made.

Level of scrutiny

Medium. It touches the JS-side wrappers for a filesystem-mutating API across three entry points, and there are user-visible behavior changes (null/array/function options and out-of-int32 retryDelay now reject; callback form throws synchronously). All changes move Bun toward node's documented behavior, are backed by a 240-case matrix diff against node v26.3.0, and the vendored test-fs-rm.js / test-fs-rmdir-*.js / test-fs-promises.js still pass per the description. I verified jsFunction_validateObject rejects null/arrays/callables so the validateObject(options, "options") call behaves as the tests assert, and that the two internal fs.rm/fs.rmSync call sites go through the native binding directly and are unaffected.

Other factors

• The refactor removes duplication (two copies of the EISDIR block collapse into one rmEisdirError + two thin flavor helpers) rather than adding it, and reuses the established $data private-symbol channel already used for FileHandle.
• Test coverage is strong per the repo's review guidance: exact error name/code/message across all three entry points, ordering cases (force before recursive, retryDelay before maxRetries), the own-recursive: undefined edge case, and a positive case confirming valid options still remove files, ENOENT still carries syscall: "lstat", and directories still hit ERR_FS_EISDIR. Tests use tempDir, no network, no sleeps.
• No CODEOWNERS cover these files. The PR notes overlap with #39328 (native message text) and #33366 (callback dispatch rewrite), and both compose cleanly with this change.

[robobun]

To confirm the behavior changes the review lists, since they are the user-visible part of this: an array or function as the options bag, and retryDelay above the int32 range, are now rejected, and callback fs.rm throws on invalid options instead of passing them to the callback. Each of those was checked against node v26.3.0, which does the same (validateObject, validateInt32(retryDelay, ..., 0), and validateRmOptions running synchronously in fs.rm). Nothing in the repo's tests or builtins passes any of the newly rejected values; everything else the wrappers accepted before is still accepted.