Skip to content

node:fs: report the caller's path, as given, as Dirent.parentPath of root entries - #41975

Open
robobun wants to merge 2 commits into
mainfrom
robobun/b76711e5/readdir-parentpath-root
Open

robobun wants to merge 2 commits into
mainfrom
robobun/b76711e5/readdir-parentpath-root

Conversation

@robobun

@robobun robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • fs.readdirSync(p, { recursive: true, withFileTypes: true }) and fs.promises.readdir gave root entries a normalized parentPath: "./d/../d" came back as "d", "d/" as "d". Node reports the caller's string for root entries and path.join(root, sub) for nested ones. Non-recursive readdir already echoed the caller's string on POSIX.
  • The cause is readdir_with_entries_recursive_{async,sync} (src/runtime/node/node_fs.rs). They computed every entry's parentPath as dirname(join(root, rel/name)), and join normalizes. On Windows the sync paths (recursive and not) also started from the slice_z copy, which is \\?\-prefixed and normalized, so C:\x\sub\.. came back as C:\x.

Fix

  • Compute parentPath once per directory with a small dirent_parent_path(root, rel) helper: the caller's bytes (args.path.slice()) at the root, join(root, rel) below it. For nested entries that is byte-identical to the old dirname(join(root, rel/name)), since name is one component.
  • The non-recursive path uses args.path.slice() too, so sync, async, and Windows agree.
  • Self-reviewed as part of a larger diff (cp walker, watchers, readdir); the review asked to land this part on its own and had no implementation concerns on it. The watcher part is node:fs: hand fs.watch and fs.watchFile paths to the OS as given on POSIX #41986.
  • Verified: test/js/node/fs/fs.test.ts ("Dirent.parentPath of a root entry is the caller's path as given") fails on 1.4.3-canary and passes with this change, on Linux and on Windows. Also ran the readdir/Dirent tests in fs.test.ts, dir.test.ts, glob.test.ts, and the vendored test-fs-readdir* node tests on both.

Background

  • Dirent.parentPath is the directory argument that produced the entry. Node's recursive readdir is a JS queue walker: root entries get the path argument untouched, deeper ones get path.join(parent, name), so only nested values are normalized.
  • Bun's recursive readdir opens subdirectories with openat relative to the root fd, so the listing itself never depended on this string; only the reported parentPath did.
Notes
  • Found by a differential run of bun 1.4.2 / canary against node v26.3.0 (readdirSync('./d/../d', {recursive, withFileTypes}): bun d, node ./d/../d; d/link/..: bun d, node the caller's string). Bun's recursive listing is kernel-correct where node's JS walker is lexical (node throws ENOENT for d/link/.. with recursion); that part is unchanged.
  • The same report had two other members that were prototyped and then left out after review: making the fs.cp* JS walker append child names instead of path.join (fixes a symlink/.. source but changes the strings a filter callback sees for ./src-style roots, where node passes src/x), and using the resolved path verbatim in fs.watchFile (fixes a\b being watched as a/b on POSIX, but fs.watch has the same join and wants the same treatment in one change).
  • test/bundler/compile-asset-bunfs.test.ts already notes "parentPath is the caller's string verbatim" for the standalone-graph readdir; this makes the real-filesystem paths agree with it.

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

…root entries

Recursive readdir withFileTypes computed the root entries' parentPath as
dirname(join(root, name)), which normalizes: "./d/../d" came back as "d"
and "d/" as "d". Node reports the caller's string for root entries and
path.join(root, sub) for nested ones. Compute parentPath once per
directory from the caller's path instead. On Windows the sync paths also
used the normalized `\\?\`-stripped copy instead of the caller's string.
@robobun

robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on bun 1.4.3-canary (Linux x64 and Windows x64) against node v26.3.0:

fs.mkdirSync("d/sub", { recursive: true }); fs.writeFileSync("d/a.txt", "");
fs.readdirSync("./d/../d", { recursive: true, withFileTypes: true }).map(e => e.parentPath);
// node: [ './d/../d', './d/../d', 'd/sub'... ]   bun: [ 'd', 'd', ... ]
fs.readdirSync("d/", { recursive: true, withFileTypes: true })[0].parentPath;
// node: 'd/'   bun: 'd'
  • Test: test/js/node/fs/fs.test.ts "Dirent.parentPath of a root entry is the caller's path as given". Fails with USE_SYSTEM_BUN=1 on both platforms, passes with bun bd test on both.
  • Also green locally: readdir/Dirent tests in fs.test.ts, dir.test.ts, glob.test.ts, readdirSync-recursive-error-leak.test.ts, vendored test-fs-readdir* / test-fs-opendir (Linux and Windows).
  • CI (build 112938): every lane that runs this diff's tests is green, including fs.test.ts on both Windows lanes. The two red jobs are unrelated to node:fs readdir: test/cli/install/bun-patch.test.ts ("label longer than 1024 bytes", bun install exits 1) on Windows 11 aarch64 and test/js/bun/s3/s3.test.ts (R2 large-upload timeouts) on Windows 2019 x64. Both are reported separately.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ed11af34-c2ad-4a59-820d-c307fea6e9f4

📥 Commits

Reviewing files that changed from the base of the PR and between dd7e6ac and 2b8b385.

📒 Files selected for processing (1)
  • src/runtime/node/node_fs.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


Walkthrough

Changes

Dirent.parentPath now preserves the caller-provided root path for non-recursive results and uses joined paths for nested recursive results. Recursive synchronous and asynchronous walkers compute each directory parent path once and reuse it. Regression tests cover path variants and both APIs.

Readdir path handling

Layer / File(s) Summary
Non-recursive parent path source
src/runtime/node/node_fs.rs
Non-recursive helpers and call sites no longer pass a separate basename. Dirent.parentPath uses the caller-provided path.
Recursive parent path reuse and regression coverage
src/runtime/node/node_fs.rs, test/js/node/fs/fs.test.ts
Synchronous and asynchronous recursive walkers compute one parent path per directory and reuse it for entries. Tests cover root path spellings and nested recursive paths across synchronous and promise-based APIs.

Suggested reviewers: dylan-conway, jarred-sumner, cirospaciari

Priority: ➖ Normal — Schedule the filesystem compatibility fix because it corrects caller-preserved root paths across synchronous, asynchronous, recursive, POSIX, and Windows readdir behavior.

Merge Risk: ⚪ Minimal · up to 2b8b3

Dirent.parentPath now preserves the caller’s root path spelling while recursive nested entries retain joined parent paths. The change is covered for synchronous and promise-based readdir behavior, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the primary change: preserving the caller-provided path in root Dirent.parentPath values.
Description check ✅ Passed The description explains the problem, implementation, scope, and verification results. It does not use the exact template headings, but it provides the required information, including how the change w…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — focused Node compat fix that also simplifies the per-entry path computation into a per-directory one.

What was reviewed:

  • Verified root_basename in the async path is the caller's bytes (copied from args.path.slice() at task creation, line 2340), so both sync and async now echo the unnormalized string at the root.
  • Checked the shared spill buffer in the async path: dirent_parent_path's result is clone_utf8'd before the loop reuses spill for entry-name joins, so no aliasing.
  • Confirmed dropping without_nt_prefix is safe here — the new source is args.path.slice(), not the slice_z copy that carried the NT prefix; the helper is still used elsewhere in this file.
  • Test uses tempDir, path.sep/path.join for cross-platform, sorts entries, and covers sync + promises × flat + recursive across three unnormalized root spellings.
Extended reasoning...

Overview

This PR fixes Dirent.parentPath in fs.readdir/readdirSync with withFileTypes: true to match Node.js: root-level entries report the caller's path string verbatim (trailing slashes, .., ./ preserved), and nested entries in recursive mode report path.join(root, subdir). The old code computed dirname(join(root, entry_rel_name)) per entry, which normalized the root. The refactor introduces a small dirent_parent_path() helper that returns the caller's bytes at the root and join(root, rel) below it, computed once per directory instead of once per entry. The now-unused basename parameter is dropped from readdir_with_entries and its Windows _u16 twin. A test in fs.test.ts covers three unnormalized root spellings across sync/promises × flat/recursive.

Security risks

None. This is a display-string change to Dirent.parentPath; the actual directory iteration still uses openat relative to the root fd and is unaffected. No untrusted-input parsing, no path-containment check depends on this string, and no auth/crypto/permissions code is touched.

Level of scrutiny

Moderate. It's a Node compat fix in src/runtime/node/, which per repo guidance means Node's observed behavior is the spec — and the PR description cites exactly that (Node's JS queue walker passes the root untouched and path.join for nested). REVIEW.md's "fix the whole class" rule is satisfied: the non-recursive path, sync recursive, async recursive, and the Windows UTF-16 branch all now source parentPath from args.path.slice() / root_path symmetrically. I verified async_task.root_path is an owned copy of args.path.slice() (line 2340), so the async path echoes the same bytes as sync. For nested entries the new join(root, rel) is equivalent to the old dirname(join(root, rel/name)) since name is a single component, so no behavior change there.

Other factors

The change is a net simplification (removes per-iteration join+dirname and the dirent_path_prev cache-compare dance). The test is well-formed per test/CLAUDE.md: tempDir from harness, path.sep/path.join for cross-platform separators, sorted output for determinism, added to the existing fs.test.ts alongside the neighboring "Dirent has the expected fields" test. No CODEOWNERS cover these files. The bug hunter ran to dry_streak with no findings. The PR description notes verification on both Linux and Windows and against the vendored test-fs-readdir* node tests.

@robobun

robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:39 AM PT - Sep 8th, 2026

❌ @robobun, your commit 2b8b385 has 3 failures in Build #112938 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41975

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

bun-41975 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants