Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors Node filesystem and path runtime: shared UID/GID and timestamp argument parsers, centralized Linux/Android copy_file_range with EINTR policy and fallbacks, unified utimes/lutimes helper, path-scanning core for basename/extname, macro-generated enum labels, a type-detection simplification, and added tests. ChangesNode Runtime Consolidation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:26 AM PT - Sep 8th, 2026
❌ @robobun, your commit c1e08db has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32021That installs a local version of the PR into your bun-32021 --bun |
|
@robobun adopt |
|
Adopted. Audited the full diff against main for behavior changes and found none; details and the mechanical equivalence checks are in the comments below. Added Current state: rebased onto main at c1e08db (conflict-free, 323+/653- in src; the Windows arm of the shared utimes helper follows #40860's |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node/node_fs.rs (1)
8083-8142:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
lutimes()now reports the wrong syscall on Windows failures.
utimes_with()hardcodessys::Tag::utime, andlutimes()now funnels through it. On Windows that changes the exposederr.syscallfromlutimetoutimeforfs.lutimes()failures.Suggested fix
fn utimes_with( &mut self, args: &args::Utimes, + syscall: sys::Tag, #[cfg(windows)] uv_utime: unsafe extern "C" fn( *mut uv::Loop, *mut uv::fs_t, *const core::ffi::c_char, @@ return if let Some(errno) = rc.errno() { Err(sys::Error { errno, - syscall: sys::Tag::utime, + syscall, path: args.path.slice().into(), ..Default::default() }) } else { Ok(()) @@ pub fn utimes(&mut self, args: &args::Utimes, _: Flavor) -> Maybe<ret::Utimes> { #[cfg(windows)] - return self.utimes_with(args, uv::uv_fs_utime); + return self.utimes_with(args, sys::Tag::utime, uv::uv_fs_utime); #[cfg(not(windows))] - self.utimes_with(args, Syscall::utimens) + self.utimes_with(args, sys::Tag::utime, Syscall::utimens) } pub fn lutimes(&mut self, args: &args::Lutimes, _: Flavor) -> Maybe<ret::Lutimes> { #[cfg(windows)] - return self.utimes_with(args, uv::uv_fs_lutime); + return self.utimes_with(args, sys::Tag::lutime, uv::uv_fs_lutime); #[cfg(not(windows))] - self.utimes_with(args, Syscall::lutimens) + self.utimes_with(args, sys::Tag::lutime, Syscall::lutimens) }As per coding guidelines: "Node.js compatibility: match Node's full error contract including error code, constructor class, message text, properties, and check ordering."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/node/node_fs.rs` around lines 8083 - 8142, The Windows branch in utimes_with() always sets sys::Tag::utime, so when lutimes() calls utimes_with() failures report the wrong syscall; update utimes_with() to accept an explicit sys::Tag (or a boolean flag) and use that tag when constructing the Error (i.e., pass sys::Tag::lutime from lutimes() and sys::Tag::utime from utimes()), or alternatively detect which wrapper called it and set errno.syscall accordingly; change the signatures/call sites (utimes(), lutimes(), and utimes_with()) to thread the correct tag through so errors on Windows report sys::Tag::lutime for lutimes() failures.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 8083-8142: The Windows branch in utimes_with() always sets
sys::Tag::utime, so when lutimes() calls utimes_with() failures report the wrong
syscall; update utimes_with() to accept an explicit sys::Tag (or a boolean flag)
and use that tag when constructing the Error (i.e., pass sys::Tag::lutime from
lutimes() and sys::Tag::utime from utimes()), or alternatively detect which
wrapper called it and set errno.syscall accordingly; change the signatures/call
sites (utimes(), lutimes(), and utimes_with()) to thread the correct tag through
so errors on Windows report sys::Tag::lutime for lutimes() failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef6ee141-6998-469a-861e-af590e172bf8
📒 Files selected for processing (3)
src/runtime/node/node_fs.rssrc/runtime/node/path.rssrc/runtime/node/types.rs
|
On the CodeRabbit finding that bun/src/runtime/node/node_fs.rs Lines 8157 to 8163 in 09703da
That divergence from Node's error contract is real but pre-existing and cross-platform, and fixing it here would contradict this PR's zero-behavior-change contract. Tracked in #32047. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/fs/fs-chown-utimes.test.ts`:
- Line 10: The test suite describe blocks for "chown/fchown/lchown argument
validation" (and the other describe at the same file) should be switched to
concurrent execution to speed up tests: replace describe("chown/fchown/lchown
argument validation", ...) with describe.concurrent("chown/fchown/lchown
argument validation", ...) and do the same for the second describe block
referenced in the comment so both describe blocks run tests in parallel safely
(each test already uses tmpdirSync()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1057555e-b5da-443a-a082-55d9fb630623
📒 Files selected for processing (1)
test/js/node/fs/fs-chown-utimes.test.ts
d9ec253 to
29ece58
Compare
|
Rebased onto current main and force-pushed (29ece58). Two things landed on main inside the functions this PR consolidates, so the conflict resolution was not purely textual:
After the rebase, every function main changed that this PR does not consolidate (futimes, lchown, chown, fchown, the stat family, readdir, rm, open, link) is byte-identical to main. Verified locally: cargo check, |
![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 is a large refactor of production-critical fs/path runtime code (copy_file_range fallback chains, basename/extname/parse loop merges, utimes dispatch) where the correctness argument rests on exact behavioral equivalence — worth a human pass.
Extended reasoning...
Overview
This PR consolidates duplicated argument-parsing and syscall-dispatch code across three Rust runtime files, net −500 lines:
node_fs.rs: extractsid_from_js(uid/gid reader, 4 copies → 1),times_from_js(atime/mtime reader, 3 copies → 1),copy_file_range_with_fallbacks(merges the two near-identical Linux copy loops fromfs.copyFileandfs.cp, parameterized by anEintrPolicyenum to preserve their one intentional difference),utimes_with(merges utimes/lutimes, parameterized by syscall tag and platform fn pointer), and anode_fs_function_enum!macro that derives both the enum andheap_label()from one list.path.rs: merges posix/win32basenameandextnameinto const-genericbasename_t<T, IS_WINDOWS>/extname_t<T, IS_WINDOWS>, and extracts the shared reverse-scan loop used by extname and parse intoscan_last_component_t.types.rs: replaces a 14-armJSTypeenumeration with theis_array_buffer_like()predicate.- Adds
test/js/node/fs/fs-chown-utimes.test.tspinning argument validation and the utimes/lutimes symlink-follow split.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input parsing surface is changed; the modifications are internal deduplication of already-shipped code paths. Argument validation (uid/gid range, time-like coercion) is moved verbatim into helpers.
Level of scrutiny
High. These files implement Node's fs and path modules in a production JS runtime. The consolidations are mechanical in intent but each carries subtle invariants whose breakage would be user-visible: the EintrPolicy split (one caller retries EINTR, the other surfaces it), the scopeguard's ftruncate/fchmod/close ordering on every exit path of the merged copy helper, the loop-bound equivalence (> -1 vs >= start where start = 0) in the posix basename merge, and the drive-letter prologue now gated on a const-generic flag. I traced each of these and they look correct, and robobun's audit plus full CI corroborate, but the blast radius if any one is off (silent data corruption in copy, wrong path parsing) is large enough that a human should sign off.
Other factors
- The only reviewer feedback (CodeRabbit's
describe.concurrentnit) is addressed and resolved. - The PR has been rebased over two intervening changes (#32964, #32052) with conflict resolution described in detail; that resolution itself is part of what a human should verify.
- CI was green on all executed lanes per robobun; the only red was infra (queue-expired darwin shards).
- No CODEOWNERS check performed, but core runtime fs code typically warrants maintainer review regardless.
|
For the human pass: two mechanical checks back the equivalence claim after the rebase, in addition to the per-function audit in the comments above.
The part of the rebase that is hardest to eyeball, keeping |
|
@robobun are there certainly no behavioural changes here? |
|
I found none, and none are intended, with one caveat on the word "certainly": the claim rests on a line-by-line equivalence audit plus the existing test surface, not an exhaustive proof. What I checked at each place where behavior could have moved:
Two mechanical checks on top of the reading: every function main has modified that this PR does not consolidate (futimes, lchown, chown, fchown, the stat family, readdir, rm, open, link) is byte-identical to main on this branch, and intersecting the lines main added to these files with the lines this branch deletes leaves only the utimes/lutimes lines that moved into the parameterized helper. Empirically, the node path suites, fs.test.ts (including the syscall-name suite), cp.test.ts, promises.test.js, and the ported upstream node tests pass on this branch on every CI platform. If you want me to walk through any specific hunk, say which. |
29ece58 to
417c6e9
Compare
|
Rebased onto current main and force-pushed (417c6e9). The only conflict was a placement collision: main's #33844 added The refactor content itself did not change this round. None of main's 8 commits to these files since the previous rebase touched any of the consolidated regions (utimes/lutimes, the copy loops, the arg readers, the enum macro, basename/extname/parse, the |
There was a problem hiding this comment.
No issues found in automated review, but this is a ~700-line consolidation touching fd-ownership/scopeguard ordering in the Linux copy path and const-generic merges of the path.rs reverse-scan loops — worth a human pass on the equivalence claims before merge.
Extended reasoning...
Overview
Consolidates duplicated code across three node-compat runtime files: node_fs.rs (shared id_from_js/times_from_js arg readers, merged utimes_with parameterized by sys::Tag + function pointer, unified copy_file_range_with_fallbacks with an EintrPolicy enum, macro-generated NodeFSFunctionEnum/heap_label), path.rs (const-generic basename_t/extname_t/scan_last_component_t replacing separate posix/win32 bodies), and types.rs (is_array_buffer_like() replacing a 14-variant match). Net ~−370 lines. Adds test/js/node/fs/fs-chown-utimes.test.ts pinning the shared arg-validation surface and utimes/lutimes symlink dispatch.
Security risks
None identified. No new external-input parsing, no auth/crypto/permissions surface. The change is internal deduplication of existing validation and syscall-dispatch code; error paths and errno sets are preserved per the diff.
Level of scrutiny
High. This is production-critical node:fs and node:path runtime code, and the copy_file_range_with_fallbacks extraction moves fd ownership + the ftruncate/fchmod/close scopeguard into a helper — exactly the acquisition/release-pairing and single-owner class CLAUDE.md flags as most-blocked. The path.rs merge relies on loop-bound equivalences (i >= start with start=0 ≡ i > -1; posix parse passing initial_start_part=0 while bounding at start) that are subtle enough to warrant a human trace even though robobun's audit and the upstream node test tables cover them.
Other factors
- The bug-hunting system found no issues.
- robobun has posted a detailed per-hunk behavioral-equivalence audit, two mechanical revert/identity checks against main, and confirmed the
err.syscallnaming from #32964 is preserved via the parameterized helper (covered by that PR's test suite). - alii is already engaged and asked directly about behavior changes; robobun answered thoroughly but no human approval has followed.
- The PR is explicitly zero-behavior-change by design, so its own tests pass before and after; correctness rests on the audit + existing suite rather than a fail-before test.
- Given scope, the fd-lifecycle move, and an already-engaged human reviewer, this exceeds the auto-approve bar of simple/mechanical/obvious.
|
Those two areas are the ones I'd point a human reviewer to as well. On the fd-ownership/scopeguard point specifically, the concrete thing to check is placement: in both original call sites the The path.rs loop bounds and |
|
@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage. |
|
On it. Main has moved ~830 commits since the last rebase (28 of them touch these three files), so this is a fresh rebase plus re-verification. First pass: every consolidation target is still duplicated on main except |
417c6e9 to
05d6938
Compare
|
@alii rebased onto current main and force-pushed (05d6938); the PR is conflict-free again and still worth landing rather than closing: every duplicated block it removes is still duplicated on main (the uid/gid and atime/mtime readers, both Linux copy loops, utimes/lutimes, posix/win32 basename and extname, the parse scan loops, the 14-arm buffer match). The one exception is What the rebase had to absorb, all mechanical: main narrowed visibility across these files, so the consolidated wrappers take each replaced function's current visibility; main switched ERR_OUT_OF_RANGE to node's Verified locally on the rebased branch: full debug build, clippy clean, the new source-lint suite (75 tests, including the byte-search deny list), One thing I cannot make green: the |
There was a problem hiding this comment.
I reviewed this PR at 805e601 and the bug-hunting pass found no issues; the earlier tmpdirSync nit is addressed. Because it's a ~340-line net refactor across node_fs.rs (fd-owning copy loop, utimes dispatch), path.rs (basename/extname/parse scan), and types.rs, a human sign-off is still worthwhile.
What was checked:
is_array_buffer_like()atsrc/jsc/JSType.rs:668is exactly the 14 variants the removed match arm listed.errno_sys_ptakesimpl AsRef<[u8]>, so the helper'sdest.as_bytes()is equivalent tocopy_file_inner's baredest: &ZStr.scan_last_component_tloop boundi_i64 >= lower_boundwithlower_bound = 0matches the removed posixi_i64 > -1;parse_posix_tstill seedsstart_part = 0while bounding atstart, andparse_windows_tseeds both atroot_end.EintrPolicy::RetryvsSurfacereproduces the only difference between the two removed loops; the scopeguard body (ftruncate/fchmod/close) andCell<u64>accounting are lifted verbatim.
Extended reasoning...
Overview
This PR consolidates duplicated code across three src/runtime/node/ files: node_fs.rs (uid/gid and atime/mtime argument readers, the two Linux copy_file_range loops with an EintrPolicy parameter, and a shared utimes_with for utimes/lutimes), path.rs (const-generic basename_t/extname_t/scan_last_component_t merging the posix/win32 twins), and types.rs (replacing a 14-arm JSType match with is_array_buffer_like()). It adds test/js/node/fs/fs-chown-utimes.test.ts pinning the shared readers and the utimes/lutimes symlink dispatch. Net -337 lines in src.
Security risks
None identified. No new input surface, no auth/crypto/permission code, no untrusted parsing added. The refactor moves existing validation into shared helpers without changing bounds or coercion order.
Level of scrutiny
High. This touches production-critical hot paths (fs.copyFile, fs.cp, path.basename/extname/parse) with fd-ownership transfer into an extracted helper, cfg-gated Windows/POSIX branches, and loop-bound arithmetic that must stay bit-identical to the Node reference. The PR's own contract is zero behavior change, so any subtle divergence is a regression. That warrants a maintainer's eyes rather than bot approval.
Other factors
- I spot-checked the equivalences most likely to drift:
is_array_buffer_like()covers exactly the 14 removedJSTypearms;errno_sys_p'simpl AsRef<[u8]>parameter makes thedestvsdest.as_bytes()change a no-op; the const-generic loop bounds (>= 0vs> -1,start_partseeding for posix vs win32 parse) match the removed bodies; the extracted scopeguard is armed at the same point relative to the ficlone fast path as both originals. - The
tmpdirSyncnit from my previous run was addressed in 805e601 (test now usesusing dir = tempDir(...)). - A maintainer (alii) is already engaged and asked to get the PR mergeable; robobun has done a detailed line-by-line equivalence audit in-thread and noted the
robobun/evidencecheck cannot go green for a zero-behavior-change refactor. Given the scope and the fd-ownership/scopeguard extraction, deferring to that human review rather than auto-approving.
|
Status on the final head (805e601): CI is green on every lane that has finished (45 checks, one Windows aarch64 test shard still running, no test failures), the review threads are closed, and the automated review pass at this head independently checked the same equivalences (the 14-variant match, the |
|
@robobun retry ci — darwin lanes expired (runner timeout) |
|
Retriggered: pushed an empty commit (1efebaa), new build is https://buildkite.com/bun/bun/builds/91724. The previous build (91669) ended with 190 passed and no test failures; the only incomplete jobs were the six darwin shards whose retries never got an agent, and the push canceled those. My Buildkite access is read-only, so a full rebuild is the only retry I can do; if the darwin pool is still starved, retrying just those shards from the Buildkite UI will be quicker than another push from me. |
There was a problem hiding this comment.
I reviewed the current head and found no bugs; the earlier tmpdirSync nit is addressed. Given the size (~660 lines removed across the Linux copy loop, cfg-gated utimes dispatch, and posix/win32 path scan merging) a human sign-off is still warranted — which this PR needs anyway to merge past robobun/evidence.
What was reviewed:
copy_file_range_with_fallbacks: scopeguard is armed at the same point both inline blocks armed it (after the ficlone fast path, before any fallible copy call); theEintrPolicy::Retry/Surfacesplit matches the old copyFile/cp EINTR arms; theerrno_sys_pdest.as_bytes()andfchmod(fd, m)normalizations are type-level only.scan_last_component_tseeds:(lower_bound, initial_start_part)=(start, 0)for posix parse,(root_end, root_end)for win32 parse,(start, start)for extname — each matches the removed loop's bound andstart_partinit; posix's oldi_i64 > -1is>= 0withstart = 0.utimes_with: each caller passes its ownsys::Tag(utime/lutime) and syscall fn, so #32964'serr.syscallnaming is preserved on both platforms.t.is_array_buffer_like()replaces the 14-armJSTypematch — checked against the helper's variant set.
Extended reasoning...
Overview
This PR consolidates duplicated code across three src/runtime/node/ files: node_fs.rs (uid/gid and atime/mtime argument readers shared across chown/fchown/lchown and utimes/futimes/lutimes; a single copy_file_range_with_fallbacks helper replacing two ~100-line inline Linux copy loops in fs.copyFile and fs.cp; a utimes_with helper parameterized by syscall tag and function pointer), path.rs (const-generic basename_t/extname_t/scan_last_component_t replacing separate posix/win32 copies), and types.rs (a 14-arm JSType match collapsed to is_array_buffer_like()). A new test file pins the shared argument-reader error naming and the utimes/lutimes symlink dispatch. Net -337 lines in src.
Security risks
None identified. The change is a mechanical dedupe of existing argument validation and syscall wrappers; no new input surface, no auth/crypto/permissions code, and the validation ranges ([-1, u32::MAX] for uid/gid, the timestamp coercion) are lifted verbatim.
Level of scrutiny
High. The consolidated regions include fd-ownership and scopeguard placement in the Linux copy path (where a misplaced guard is a leak or double-close), cfg-gated Windows/POSIX dispatch via function pointers, and index arithmetic in the path scanner where an off-by-one in lower_bound/initial_start_part would silently change path.parse/extname output. The PR's own contract is zero behavior change, so correctness rests entirely on line-by-line equivalence — exactly the kind of review a human should confirm rather than an automated approval.
Other factors
- The PR thread already contains a detailed per-hunk equivalence audit and two mechanical checks (main-added-lines ∩ branch-deleted-lines; byte-identity of untouched functions). CI is green across platforms on the rebased head; the one red check (
robobun/evidence) is structurally impossible to satisfy for a zero-behavior-change refactor and requires human override regardless. - A maintainer (alii) is actively engaged — asked for behavioral-change confirmation, requested the rebase, and is managing CI retries — so a human review is already in progress.
- My prior inline nit (
tmpdirSync→tempDir) was addressed in 805e601; the only commit since is a CI retrigger. This is my first review-body post on the PR.
1efebaa to
04908a0
Compare
|
The darwin expiry was not a transient runner timeout, so the retrigger could not have cleared it: the bare-metal mac fleet is offline for reimaging, and main rerouted darwin tests to a hosted queue (and stopped scheduling them on PR pushes) in #37354, #37364 and #37369, all of which landed after my last rebase. This branch was still generating pipeline steps that target the offline fleet, so every darwin shard expired no matter how many times it was retried. Rebased onto main again (04908a0, no conflicts, src delta byte-identical to before, the empty retrigger commit dropped). The new build is https://buildkite.com/bun/bun/builds/91839 and it schedules no darwin test shards, same as current main builds, so it should be able to finish. The one failure on the previous build was the mariadb Docker image failing to build on an alpine runner, which I reported separately; it is unrelated to this PR. |
There was a problem hiding this comment.
I reviewed this PR and the automated pass found no bugs. Given the scope — ~660 lines removed across the Linux copy_file_range fd/scopeguard path, the cfg-gated utimes_with dispatch, and the shared path.{basename,extname,parse} scan loops — a human look before merge is still worthwhile.
What was checked and held up:
is_array_buffer_like()is exactly the 14JSTypevariants the deleted match arm listed.scan_last_component_tloop bounds/seeds match each original: posix> -1≡>= 0withstart = 0;parse_posix_tpassesinitial_start_part = 0(notstart);parse_windows_t/extname_windows_tpassroot_end/startfor both.copy_file_range_with_fallbacks: scopeguard armed at the same point (post-ficlone) at both call sites;errno_sys_ptakesimpl AsRef<[u8]>sodestvsdest.as_bytes()is identical;Modeisu32so thefchmodcast is unchanged; theEintrPolicy::Retry/Surfacesplit matches the two original loops.utimes_withpreserves the per-callersys::Tag(utime/lutime) from #32964.
Extended reasoning...
Overview
This PR consolidates duplicated argument-parsing and syscall-loop code in src/runtime/node/node_fs.rs (uid/gid readers, atime/mtime readers, the two Linux copy_file_range loops for fs.copyFile and fs.cp, and the utimes/lutimes bodies), merges the posix/win32 twins of basename/extname and the parse scan loop in src/runtime/node/path.rs behind const-generic helpers, and replaces a 14-arm JSType match in src/runtime/node/types.rs with the existing is_array_buffer_like() predicate. Net -337 lines in src. A new test file pins the shared uid/gid and atime/mtime validation and the utimes/lutimes symlink dispatch.
Security risks
None identified. The change is a mechanical extraction of existing control flow into shared helpers; no new input-validation surface, no new unsafe blocks (the one unsafe in the copy helper is the same libc copy_file_range FFI call that was there before), and no changes to path-traversal or permission checks.
Level of scrutiny
High. This touches production-critical Node.js compat code: fs.copyFile/fs.cp on Linux involves fd ownership across a scopeguard (ftruncate/fchmod/close on every exit path), and node:path parsing is called on every module resolution. The refactor merges four near-identical loops whose only differences (separator predicate, drive-letter prologue, EINTR policy, initial start_part) are now parameters — each of those axes needs to be verified against the original per call site. The PR author documented a line-by-line equivalence audit and I independently spot-checked the specific points listed in the message above; all held. But the diff is large enough and the fd-ownership placement subtle enough that a maintainer eyeball is warranted, and alii is already engaged and asked for it to be made mergeable.
Other factors
The PR has been rebased three times over ~830 main commits with non-trivial conflict resolution (notably absorbing #32964's per-operation sys::Tag into utimes_with). CI is green across all platforms per the thread; the only red check is robobun/evidence, which is expected to fail for a zero-behavior-change refactor. My prior inline nit (tmpdirSync → tempDir) was addressed in 04908a0. The added test file follows harness conventions (using tempDir, describe.concurrent, fd closed in finally).
There was a problem hiding this comment.
I reviewed this at the current head and found no bugs. Given it's a -337-line consolidation across node_fs.rs (the Linux copy_file_range loop with fd-owning scopeguard, utimes_with) and path.rs (const-generic scan loops), a human sign-off is still worthwhile before merging past the evidence check.
What was checked: the EintrPolicy::Retry/Surface split matches the two original loops' EINTR arms; the scopeguard is armed at the same point (after ficlone, owning dest_fd) with the same ftruncate/fchmod/close body; utimes_with preserves the per-caller sys::Tag so err.syscall stays utime/lutime; scan_last_component_t loop bounds (i >= lower_bound with lower_bound = 0 ≡ the old posix i > -1) and start_part seeds match all four original call sites; is_array_buffer_like() covers exactly the 14 removed JSType arms.
Extended reasoning...
Overview
The PR consolidates duplicated code across three Node compat files: node_fs.rs (uid/gid and atime/mtime argument readers shared by chown/fchown/lchown and utimes/futimes/lutimes; the two near-identical Linux copy_file_range loops merged into copy_file_range_with_fallbacks parameterized by EintrPolicy; utimes/lutimes merged into utimes_with taking the syscall tag and a function pointer), path.rs (posix/win32 basename/extname/parse scan loops merged behind a const IS_WINDOWS: bool generic and a LastComponentScan helper), and types.rs (a 14-arm JSType match replaced with is_array_buffer_like()). Net 322+/659- in src plus a new test file pinning the shared readers' per-argument error naming and the utimes/lutimes symlink dispatch.
Security risks
None identified. No new user-input parsing surface; the argument validators are verbatim extractions with the same bounds ([-1, u32::MAX] for uid/gid). No auth, crypto, or path-traversal changes.
Level of scrutiny
High. node:fs and node:path are among the most-exercised Node compat modules, and the copy-loop consolidation touches fd ownership and scopeguard placement — the review guidelines flag this class ("Pair every acquisition with its release at the acquisition site", "Treat every refactor as guilty until proven behavior-preserving"). The path scan-loop merge changes loop-bound spellings (i > -1 → i >= 0) and threads start_part through a struct, which is exactly where off-by-ones hide. The PR author has done a thorough per-hunk equivalence audit and the bug hunter independently checked the same points without findings, but a refactor of this scope in these files should have a human's name on the approval.
Other factors
The maintainer has been actively shepherding this PR (three rebase rounds, explicitly asked whether there are behavioral changes, and asked for it to be made mergeable). The robobun/evidence check is red by construction for a zero-behavior-change refactor, so merging requires a human override regardless. My only prior comment (the tmpdirSync → tempDir nit) was addressed. The head has moved since the last recorded automated review pass (merge c15d0b5), so this run's zero-findings result is new information for the current head.
c15d0b5 to
fc027ff
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Given the scope — a ~1000-line refactor across the fs.copyFile/fs.cp Linux copy loops, the utimes/lutimes dispatch, and the path.basename/extname/parse scan loops — a human sign-off is still worthwhile before merging past the evidence check.
What was reviewed: is_array_buffer_like() matches exactly the 14 JSType variants the deleted arm listed; errno_sys_p takes impl AsRef<[u8]> so the dest vs dest.as_bytes() unification is a no-op; the merged scan_last_component_t loop bound >= lower_bound with lower_bound = 0 is the old posix > -1, and each caller's initial_start_part seed matches the value the removed loop initialized (0 for posix parse, root_end for win32 parse, start for extname); id_from_js/time_arg_from_js preserve the exact next → eat ordering and error text of the blocks they replace; the copy_file_range_with_fallbacks scopeguard is armed at the same point relative to dest_fd as both original inline blocks (after the ficlone fast path), and EintrPolicy::Retry/Surface reproduces the one behavioral difference between the two removed loops. The earlier tmpdirSync nit is resolved.
Extended reasoning...
Overview
This PR consolidates duplicated argument-parsing and syscall-dispatch code across three files in the Node.js compatibility layer: src/runtime/node/node_fs.rs (uid/gid and atime/mtime readers, the Linux copy_file_range loop shared by fs.copyFile and fs.cp, and the utimes/lutimes bodies), src/runtime/node/path.rs (posix/win32 basename, extname, and the reverse-scan loop inside parse merged behind a const IS_WINDOWS generic), and src/runtime/node/types.rs (a 14-arm JSType match replaced by is_array_buffer_like()). Net −337 lines in src/. A new test file pins the argument-validation surface and the utimes-vs-lutimes symlink dispatch. The PR has been rebased four times over two months with detailed equivalence audits at each step.
Security risks
None identified. The changes are pure extractions of existing control flow into shared helpers; no new input is accepted, no validation is loosened, and no security-relevant path (auth, TLS, path traversal) is touched. The one memory-safety-adjacent piece — the dest_fd scopeguard in the copy helper — is armed at the same point as both originals and the ftruncate/fchmod/close body is lifted verbatim.
Level of scrutiny
High. fs.copyFile, fs.cp, path.basename, path.extname, and path.parse are all high-traffic Node-compat APIs where an off-by-one in a loop bound or a dropped early-return would be a user-visible regression. The individual consolidations are each mechanically verifiable (and I did verify the loop bounds, start_part seeds, EINTR split, and is_array_buffer_like variant set against the removed code), but the aggregate diff is large enough and the fd-ownership/scopeguard reasoning subtle enough that it exceeds the "simple, mechanical, or obvious" bar for automated approval.
Other factors
The maintainer has been actively shepherding this PR ("get this mergeable") and robobun has stated it needs a human approval past the robobun/evidence check, which cannot pass by construction for a zero-behavior-change refactor. My earlier inline nit (tmpdirSync → tempDir) was addressed in 805e601 and the test file now uses disposable temp directories. The bug-hunting system raised and refuted one candidate (new test file vs. adding to fs.test.ts); the separate file is reasonable given it targets a specific consolidated surface. CI was green on the last completed build modulo darwin runner-pool issues unrelated to this change.
|
Rebased onto current main again and force-pushed (fc027ff). The branch is back to two commits on top of main: the refactor (still authored by @alii) and the test file. One conflict this round, in types.rs: #39164 replaced the Verified on the rebased tree: full debug build, |
fc027ff to
303001d
Compare
|
Rebased onto current main again (303001d). The only conflict was positional: #39448 deleted |
There was a problem hiding this comment.
I reviewed this PR at the current head and found no bugs. Given it's a ~340-line net refactor of the Linux copy_file_range loop (fd ownership + scopeguard placement), the node:path reverse-scan loops, and utimes/lutimes dispatch, a maintainer sign-off is still worthwhile.
What was checked:
is_array_buffer_like()is exactly the 14JSTypevariants the deleted match listed;errno_sys_ptakesimpl AsRef<[u8]>sodestvsdest.as_bytes()is equivalent.EintrPolicy::Surfaceat thefs.cpsite falls through to_ => return errfor EINTR, matching the old cp loop that had no EINTR arm; the scopeguard is armed at the same point (post-ficlone) at both callers.scan_last_component_tseeds (lower_bound,initial_start_part) match each of the four original loops:(0,0)/(start,start)/(start,0)/(root_end,root_end); posix'si_i64 > -1≡i_i64 >= 0.utimes_withthreadssys::Tag::utime/lutimeper caller, preserving #32964'serr.syscallsplit.
Extended reasoning...
Overview
This PR consolidates duplicated code in three node:* runtime files: node_fs.rs (uid/gid and atime/mtime argument readers, the two Linux copy_file_range loops merged behind an EintrPolicy enum, and a shared utimes_with body for utimes/lutimes), path.rs (const-generic basename_t/extname_t and a shared scan_last_component_t reverse-scan used by four call sites), and types.rs (14-arm JSType match → is_array_buffer_like()). It adds a test file pinning the consolidated argument-validation surface and the utimes/lutimes symlink dispatch. Net -337 lines in src/.
Security risks
None identified. The change is a behavior-preserving extraction; no new inputs are accepted, no validation is loosened, and no security-sensitive code (auth, crypto, path traversal checks) is touched. The path.rs changes are to basename/extname/parse, which are pure string operations already covered by upstream Node test tables.
Level of scrutiny
High. The Linux copy loop consolidation moves fd-ownership and a scopeguard (ftruncate/fchmod/close) into a helper — getting the arm point wrong would leak or double-close an fd. The path.rs merge collapses four hand-written reverse-scan loops with subtly different bounds (> -1 vs >= start) and start_part seeds into one parameterized function; an off-by-one here would silently change path.parse/extname output for edge cases. The utimes_with consolidation must preserve the per-operation err.syscall name that #32964 introduced. All of these were spot-checked against the deleted code and hold, but the density of load-bearing detail is exactly what the repo's REVIEW.md flags for "treat every refactor as guilty until proven behavior-preserving."
Other factors
The PR has been through five rebases with documented per-rebase equivalence checks, the maintainer is already engaged and driving toward merge, and the existing error.syscall suite plus 102 upstream Node path/fs tests exercise the consolidated code. My earlier tmpdirSync nit was addressed. No outstanding review threads remain. The one candidate issue raised this run (new test file vs adding to fs.test.ts) was ruled out — the file pins a specific consolidation surface and follows the same pattern as other sibling-validation test files in test/js/node/fs/. I'm deferring rather than approving because the scope and subtlety fall outside "simple, mechanical, or obvious."
303001d to
39c8135
Compare
|
Rebased onto current main again (39c8135). One conflict, in the fs args module: #40374 gave the argument structs borrowed lifetimes ( Verified on the rebased tree: debug build, rustfmt, |
There was a problem hiding this comment.
I reviewed this PR at the current head and didn't find any bugs. Because it's a ~1000-line refactor of the Linux copy_file_range loop (fd ownership + scopeguard), the merged posix/win32 path scanners, and the utimes/lutimes dispatch, a maintainer sign-off is still the right call — especially since it needs to land past the robobun/evidence check.
What was reviewed:
is_array_buffer_like()is exactly the 14JSTypevariants the removed match arm listed.EintrPolicy::Surfacereproduces the oldfs.cploop (no EINTR arm → falls through to the error return);Retryreproducesfs.copyFile'sE::EINTR => continue.- The scopeguard in
copy_file_range_with_fallbacksis armed at the same point relative to the ioctl_ficlone fast path as both original inline blocks. scan_last_component_tloop bounds andstart_partseeds match each of the four original call sites (posixi_i64 > -1≡i_i64 >= 0; win32>= start/>= root_endpreserved).
Extended reasoning...
Overview
This PR consolidates duplicated code in src/runtime/node/node_fs.rs, path.rs, and types.rs with zero intended behavior change: extracts id_from_js / times_from_js for the six chown/utimes argument readers, extracts copy_file_range_with_fallbacks with an EintrPolicy enum to unify the two Linux copy loops, extracts utimes_with parameterized by sys::Tag and a per-platform function pointer, merges posix/win32 basename/extname via const-generic is_sep_t, extracts scan_last_component_t for the four reverse-scan loops in extname/parse, and replaces a 14-arm JSType match with is_array_buffer_like(). Net −337 lines in src. A new test file pins the shared uid/gid and atime/mtime validation and the utimes/lutimes symlink dispatch.
Security risks
None identified. The change touches file-descriptor ownership (the dest_fd scopeguard) but does not alter it: the guard is armed at the same relative position and its body (ftruncate + fchmod + close) is lifted verbatim. No new user-controlled input paths, no changes to validation ranges or error-code mapping.
Level of scrutiny
High. This is a large refactor of production-critical filesystem code — the Linux copy_file_range loop with its EINTR/fallback/scopeguard handling is exactly the class of code REVIEW.md flags as most-blocked ("pair every acquisition with its release at the acquisition site"; "treat every refactor as guilty until proven behavior-preserving"). The path.rs consolidation touches the core of basename/extname/parse for both platforms. While the per-hunk equivalence checks all hold, this is not the kind of mechanical change an automated reviewer should approve unilaterally.
Other factors
The PR has been extensively self-audited across five rebases with detailed mechanical checks (revert-set intersection, byte-identity of untouched functions, per-target cargo check). A maintainer has been actively shepherding it and asked for it to be made mergeable; robobun reports it is ready and waiting for approval past the robobun/evidence check (which structurally cannot pass for a zero-behavior-change refactor). My prior inline nit (tmpdirSync → tempDir) was addressed in 805e601. The bug-hunting system found no issues at the current head. Given all that, this is ready for a human to merge — but the decision to land a refactor of this scope past a red required check should be a maintainer's, not mine.
39c8135 to
9fb19a3
Compare
|
Rebased onto current main again (9fb19a3). One conflict, in types.rs, same shape as the #39164 round: #40511 rewrote the body of the Verified on the rebased tree: debug build, rustfmt, |
9fb19a3 to
8c930a0
Compare
Pins the per-argument error naming shared by chown/fchown/lchown and by utimes/futimes/lutimes, and the follow/nofollow split between utimesSync and lutimesSync.
8c930a0 to
64e262a
Compare
.toThrow(err) compares only the message, so each validation case now also asserts the constructor and err.code via toThrowWithCode.
|
Rebased onto current main again; head is now c1e08db. This round had the first conflict that touches the consolidated code itself rather than its surroundings: #40860 replaced libuv Also in this push, per the review comment above: the test now asserts constructor and Verified on the rebased tree: debug build, rustfmt, the Windows cargo check, One correction for the record: the first force-push of this round (8c930a0) briefly carried only the test commit, because a local |
What this does
Dedupes repeated argument-conversion and error-path blocks in
node_fs.rsand removes dead helpers in nodepath/types. Net -330 lines in src (323 added, 653 removed) after the rebases below.Split from #31912 (whole-repo simplification pass; closing that PR in favor of module-scoped splits). This PR only moves and removes code — zero intended behavior change. Verified there by a per-file behavioral-equivalence audit and full CI; verified here by a standalone full-workspace compile check.
Tests
test/js/node/fs/fs-chown-utimes.test.tspins the consolidated surface: chown/fchown/lchown must validate uid and gid identically and name the failing argument itself, utimes/futimes/lutimes likewise for atime/mtime, and utimesSync must follow symlinks while lutimesSync must not (the dispatch the mergedutimes_withpreserves). Because this PR intends zero behavior change, these tests pass before and after the refactor by design; they guard the shared readers against future drift. A test that fails on main cannot exist for this PR: any such failure would mean the consolidation changed observable behavior, which is the one outcome it must not have. The correctness argument is the behavioral-equivalence audit plus the full existing suite passing on both sides, not a fail-before proof.The
error.syscallsuite added on main by #32964 runs through the consolidatedutimes_withand passes, so the utime/lutime naming is covered by an existing test that would fail if the consolidation got it wrong. The upstream node path-* and fs utimes/chown/copyfile/cp suites (102 files, including the ones #34505 added) also pass on this branch.Rebase notes
Rebased eight times as main moved; the consolidation itself is unchanged except where noted.
utimesandlutimesreport node's operation names (utime/lutime) inerr.syscall. The sharedutimes_withtakes thatsys::Tagas a parameter and each caller passes its own; keeping the old helper shape would have silently madelutimesreportutimeagain.fs.lchownon Windows, so the interim test gating for it was dropped.NodeFSFunctionEnum::heap_labelas dead code, so the hunk that derived the enum andheap_labelfrom one macro no longer has a purpose and was dropped; the enum and its impl are byte-identical to main.pubtopub(crate)or private); the consolidated wrappers adopt main's current visibility for each function they replace.&&form; the test tracks that.StringOrBufferbuffer arm (bool flags to enums); this PR's change to that arm is only its pattern, so the resolution keeps main's body under theis_array_buffer_like()guard.unwatch_file, the function adjacent to the newutimes_with; it stays deleted.TimeLike, need none.StringOrBufferbuffer arm body again (nowSelf::buffer_from_js); as with node/types: replace the StringOrBuffer parser bool flags with enums #39164, main's body is kept under theis_array_buffer_like()guard.ReturnCode::errno()plus a hand-builtsys::Errorwithrc.to_error(tag)in the Windows arms ofutimes/lutimes; the sharedutimes_withuses that same form (rc.to_error(syscall)thenwith_path), so its Windows error path matches main's two functions. Checked withcargo check --target x86_64-pc-windows-msvc, since that arm is cfg(windows).err.code(toThrowWithCode) for every validation case, since.toThrow(err)compares only the message.Every function main has modified in these files that this PR does not consolidate is byte-identical to main on this branch, and the only main-added lines this branch removes are the signature lines of the functions it rewrites. The Linux copy helper was re-verified against current main: everything before the removed loop block is identical at both call sites, the removed block at the
fs.copyFilesite contains the EINTR retry (EintrPolicy::Retry) and the one at thefs.cpsite does not (EintrPolicy::Surface), and the new FreeBSD copy paths main added are separate cfg blocks the helper does not touch.cargo checkpasses for the windows-msvc, apple-darwin, and freebsd targets in addition to the full Linux debug build and clippy.