Conversation
…-long path The argument parser rejects a path of MAX_PATH_BYTES or more before any syscall runs. It built that error with `Tag::open` and then blanked the `syscall` field, so every operation reported `err.syscall === undefined` and a message that said `open`. Node names the syscall the operation issues: `stat`, `scandir`, `lstat` for rm and realpath, and so on. `ArgumentsSlice` gains a `syscall` tag. Each fs binding sets it to the syscall its operation issues (`NodeFSFunctionEnum::syscall`) before it parses, and the parser builds the error from it. The sync, callback and promise forms now all carry the same `syscall` and message as the operation's other errors. `fs.watch` names `watch`, as in node.
|
Status: reproduced with |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesThe filesystem bindings now attach operation-specific syscall tags to path-length errors. Filesystem path error reporting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, behavior, scope, known differences, and verification. It provides the information required by the template, although it uses Problem and Fix headings instead of the template headings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node compat fix threading the operation's syscall tag into the pre-syscall ENAMETOOLONG.
What was reviewed:
NodeFSFunctionEnum::syscall()match covers all 40 variants; mappings line up with the Node-verified table in the description (readdir→scandir, rm/realpath-emulated→lstat, utimes→utime, etc.).ArgumentsSlice.syscalldefaults toopen, so non-fs callers of the path parser (from_bun_stringat types.rs:1174, Bun.file/S3) keep their prior message and gain a definederr.syscall— no regression.- All entry points that parse fs paths set the tag before parsing: macro-generated sync/async,
parse_async_args, and the hand-written cp/cpSync/readdirRecursive/watch/watchFile bindings. - Test matrix asserts
err.syscalland message prefix across 31 callback ops plus sync/promises/watch, so it fails on the old blank-syscall behavior.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility gap: when Bun's node:fs argument parser rejects an over-long path before any syscall runs, the resulting ENAMETOOLONG error previously had err.syscall === undefined and a message that always said open. The fix threads a bun_sys::Tag through ArgumentsSlice so the parser can name the syscall the operation would have issued. Six files change: ArgumentsSlice gains a syscall field defaulting to open; NodeFSFunctionEnum gains a const fn syscall() mapping each op to its representative tag; the sync/async binding entry points (macro-generated and hand-written) set the tag before parsing; Valid::path_too_long takes the tag instead of hardcoding open and blanking the field; seven bun_sys::Tag constants are widened to pub; and the test file is expanded from bare code/path checks to per-operation syscall and message-prefix assertions across callback, sync, promise, and watch forms.
Security risks
None. This change only affects which string appears in err.syscall and the error message for a path-length validation error that already existed. No new input is parsed, no new syscalls are issued, no allocation or lifetime changes, no user-controlled data flows anywhere new. Widening pub(crate) to pub on inert integer-newtype constants has no security implication.
Level of scrutiny
Low-to-moderate. This is a mechanical Node-compat improvement: threading a Copy enum value from the binding entry point to the error constructor. The mapping table is the only place a mistake could hide, and the PR description documents it was derived by running Node 26.3.0 against a 5001-byte path and matching each op's reported syscall — the same tag each op already uses for its ENOENT. The exhaustive match means a new NodeFSFunctionEnum variant will fail to compile until mapped. The ArgumentsSlice default of open preserves prior behavior for every caller that doesn't set it (Bun-native Bun.file, S3, static routes), which the PR description calls out explicitly.
Other factors
Test coverage is strong: the existing it.each over 31 callback operations now asserts err.syscall and the ENAMETOOLONG: name too long, <syscall> ' message prefix for each, and new cases cover statSync/readdirSync/realpathSync/realpathSync.native/cpSync/renameSync, fs.promises.readFile/readdir, and fs.watch. These assertions fail on the pre-fix behavior (undefined syscall), satisfying the fails-for-the-right-reason requirement. The known remaining divergences (truncate, fs.promises.realpath, opendir message shaping, watchFile throwing) are pre-existing, documented in the PR notes, and out of scope. No CODEOWNERS cover the changed paths, no prior review comments exist, and the bug hunt exited on dry_streak with no findings.
|
Updated 10:12 PM PT - Aug 26th, 2026
✅ @robobun, your commit 9c702a9b80fe38db09b0e85ae2cb5b0261d3a0a1 passed in 🧪 To try this PR locally: bunx bun-pr 40603That installs a local version of the PR into your bun-40603 --bun |
This branch has not been deployed
2 participants
Problem
ENAMETOOLONGwitherr.syscall === undefined, and a message that saysopenfor every operation:fs.stat(p, cb)calls back withENAMETOOLONG: name too long, open '/aaa...'. Node names the syscall the operation issues (stat,scandir,lstat, ...), inerr.syscalland in the message, like any other syscall error from that operation.Valid::path_too_long(src/runtime/node/types.rs:1228). The argument parser raises this error before any syscall runs, so it did not know the operation. It built the error withTag::openand blankedsyscall. node:fs: reject the promise instead of throwing when a path is too long for any syscall #38383 moved the delivery to the callback and left this as a known gap.Fix
ArgumentsSlice(src/jsc/CallFrame.rs) gainssyscall: bun_sys::Tag, defaultopen. The parser builds theENAMETOOLONGfrom it, for the sync throw and for the deferred async rejection alike.NodeFSFunctionEnum::syscall()(src/runtime/node/node_fs.rs) maps each operation to the tag its own errors carry: the same one itsENOENTnames today (readdirisscandir,rmand the emulatedrealpatharelstat,utimesisutime).run_sync,run_asyncandparse_async_argsset it before they parse.cpuseslstat,fs.watchuseswatch,watchFileusesstat.Tagconstants inbun_sysbecomepubso the runtime crate can name them.test/js/node/fs/fs-path-length.test.ts(31 callback operations now checkerr.syscalland the message prefix, plus sync, promise andfs.watchcases; 33 fail on 1.4.1, all pass with the fix). Also fs.test.ts, cp.test.ts, promises.test.js, dir.test.ts, fs-mkdir.test.ts, fs.watch.test.ts, 34 portedtest-fs-*files, clippy, andcargo checkfor the Windows and macOS targets.Background
ArgumentsSliceis the cursor a native binding walks over a call's arguments. node:fs bindings setwill_be_asyncon it so path strings are copied thread-safe, and node:fs: reject the promise instead of throwing when a path is too long for any syscall #38383 addeddeferred_errorso a path errno met while parsing for an async call becomes a rejected promise instead of a throw.bun_sys::Tagis the syscall name on abun_sys::Error.to_system_errorturns it intoerr.syscalland into the, <syscall> '<path>'part of the node-style message.PathBufferright before the syscall, so the parser rejects a path ofMAX_PATH_BYTESor more up front. The kernel's limit is the same (PATH_MAX), so the earlyENAMETOOLONGis the error the syscall would have produced. Only its name was missing.Notes
Node 26.3.0 and this branch,
err.syscallfor a 5001-byte path on Linux, callback form (identical for sync and promise forms):Differences that remain, all pre-existing and out of scope here:
truncate: node's truncate is open + ftruncate and namesopen. Bun issues truncate(2) and namestruncate, forENOENTas well as for this error.fs.promises.realpath: bun routes it through the emulated realpath (lstat), node through the native one (realpath).to_system_error(sys: stop truncating Node-style error messages at 4096 bytes when path/dest are long #38201 fixes that).opendirreshapes the stat error's message in JS and only does so on an un-truncated message, so its message still namesstatuntil sys: stop truncating Node-style error messages at 4096 bytes when path/dest are long #38201 lands. Itserr.syscallisopendiralready.fs.watchFile(tooLong)does not throw in node. Bun throwsENAMETOOLONG; it now namesstat.Other callers of the path parser (
Bun.file,Bun.write, S3 keys, static routes) keep the defaultopen: their message already saidopen, anderr.syscallis now"open"instead ofundefined.The two S3 tests in
test/js/bun/s3/s3.test.tsthat fail in this container fail because its egress proxy denies the S3 host. They do not touch path parsing.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-path-length.test.ts