node:fs: apply cpSync errorOnExist per entry, not to the destination directory

[robobun]

Repro

const fs = require("fs");
fs.mkdirSync("out");                        // any prior run, or mkdir -p
fs.cpSync("assets", "out", { recursive: true, force: false, errorOnExist: true });

node: copies everything; ERR_FS_CP_EEXIST only if an individual dest FILE exists
bun:  SystemError: Target already exists: cp returned EEXIST (out already exists) out

force: false + errorOnExist: true is the "add my files, never clobber the user's" mode, and a destination directory essentially always exists, so every such call fails.

Cause

onDir in src/js/internal/fs/cp-sync.ts refused the destination directory itself:

function onDir(srcStat, destStat, src, dest, opts) {
  if (!destStat) return mkDirAndCopy(srcStat.mode, src, dest, opts);
  if (opts.errorOnExist && !opts.force) {
    throw fsCpEExistError({ message: `${dest} already exists`, ... });
  }
  return copyDir(src, dest, opts);
}

node's lib/internal/fs/cp/cp-sync.js has no such check: onDir merges, and ERR_FS_CP_EEXIST comes only from mayCopyFile, per colliding file. The branch above is the body of node's async lib/internal/fs/cp/cp.js onDir, which does refuse the directory. It was copied into the sync path in #31830, where only the async half belonged.

Fix

Drop the check from the sync onDir. src/js/internal/fs/cp.ts (async) keeps it, because node's async cp really is stricter than its cpSync. Added a comment on both halves so the asymmetry doesn't read as an oversight.

Verification

Differential run against node v26.3.0, { recursive: true, force: false, errorOnExist: true }:

scenario	node cpSync	bun cpSync before	bun cpSync after
dest missing	copies	copies	copies
dest exists, empty	copies	EEXIST	copies
dest exists, non-colliding file	merges	EEXIST	merges
dest exists, colliding file	EEXIST	EEXIST (on dest dir)	EEXIST (on the file)
dest exists, colliding subdir	merges	EEXIST	merges
dest subdir w/ colliding file	EEXIST	EEXIST (on dest dir)	EEXIST (on the file)
dest subdir w/ other file	merges	EEXIST	merges

fs.promises.cp / fs.cp match node on every row both before and after (all seven refuse the existing directory), and a test now pins that.

Bun's thrown error is a SystemError carrying path = the colliding file. node sets path only when a filter is passed (without one its copyDir runs in C++ and drops the field); the code and message text are identical either way.

• bun bd test test/js/node/fs/cp.test.ts — 50 pass, 0 fail (3 new)
• all 77 test/js/node/test/parallel/test-fs-cp-* pass, including test-fs-cp-async-dir-exists-error-on-exist.mjs and test-fs-cp-sync-error-on-exist.mjs

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 14 minutes

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: f0685813-34a7-4e72-935d-0d19cad8f397

📥 Commits

Reviewing files that changed from the base of the PR and between 4ca9ebc and aeb489e.

📒 Files selected for processing (3)

• src/js/internal/fs/cp-sync.ts
• src/js/internal/fs/cp.ts
• test/js/node/fs/cp.test.ts

---

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

[robobun]

^{Updated 3:30 AM PT - Jul 6th, 2026}

❌ @robobun, your commit aeb489e has some failures in Build #68788 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33416

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

bun-33416 --bun

[claude]

LGTM — clean removal of the over-strict directory check to match node's cpSync, well-covered by the three new tests; the inline nit about the missing mirror comment on cp.ts is non-blocking.

Extended reasoning...

Overview

This PR removes an 8-line errorOnExist && !force check from onDir in src/js/internal/fs/cp-sync.ts and adds a 3-line explanatory comment in its place. The removed check caused fs.cpSync to throw ERR_FS_CP_EEXIST on the destination directory itself, whereas node's cpSync merges into an existing directory and only errors per colliding file (via mayCopyFile, which is unchanged). The async cp.ts intentionally keeps its directory-level check because node's async cp really is stricter — the PR description documents this asymmetry with a differential table against node v26.3.0. Three new tests in test/js/node/fs/cp.test.ts pin all three behaviors: sync merges, sync still errors on a colliding file, and async refuses the directory.

Security risks

None. This loosens an over-strict check to match node, but the per-file collision guard in mayCopyFile remains — errorOnExist still prevents overwriting existing files. No path traversal, permissions, or untrusted-input handling is affected.

Level of scrutiny

Low-to-moderate. This is a small, targeted Node.js compatibility fix in a JS built-in module — a pure deletion bringing the ported code back in line with upstream lib/internal/fs/cp/cp-sync.js. The PR description traces the bug's origin (#31830 copied the async onDir body into the sync path), verifies against node's own behavior across 7 scenarios, and confirms all 77 test-fs-cp-* node parallel tests pass. The change is mechanical and easily audited against node's source.

Other factors

The one finding is a nit: the PR description says a comment was added "on both halves" but only cp-sync.ts was touched — cp.ts's onDir has no mirror comment. This is a documentation gap, not a correctness issue: the sync-side comment already explains the asymmetry ("node's async cp refuses the directory itself; its cpSync does not"), and the new promises.cp refuses the destination directory test pins the async behavior against future regression. The nit is worth addressing but doesn't block approval.

[claude]

LGTM — the mirror comment on the async onDir addresses my earlier nit; small, well-tested Node compat fix.

Extended reasoning...

Overview

Removes the directory-level errorOnExist && !force throw from cpSync's onDir in src/js/internal/fs/cp-sync.ts so that an existing destination directory is merged into (with ERR_FS_CP_EEXIST raised only per colliding file), matching Node's lib/internal/fs/cp/cp-sync.js. The async onDir in src/js/internal/fs/cp.ts keeps the check (Node's async cp really is stricter), and both functions now carry a comment explaining the intentional asymmetry. Three new tests in test/js/node/fs/cp.test.ts pin: cpSync merges into an existing dir, cpSync still throws on a colliding file (with the file's path on the error), and promises.cp refuses the existing directory outright.

Security risks

None. This is a Node.js compatibility fix in the JS-side fs.cp walker. It relaxes one over-strict check (refusing an existing destination directory) to match Node's documented and observed behavior; the per-file collision check in mayCopyFile is unchanged, so errorOnExist still prevents overwriting existing files. No auth, crypto, path-traversal, or untrusted-input parsing is touched.

Level of scrutiny

Low-to-moderate. The functional change is a 9-line deletion whose correctness is directly verifiable against Node's reference implementation (the PR description includes a 7-row differential table against Node v26.3.0), plus two 3-line comments. The rest is test coverage. The blast radius is limited to fs.cpSync(..., { recursive: true, force: false, errorOnExist: true }) on an existing destination directory — a case that previously always failed in Bun, so this can only make previously-failing calls succeed the way Node does.

Other factors

• My earlier inline nit (only cp-sync.ts had the explanatory comment) has been addressed in commit 30c75b0, which adds the mirror comment to cp.ts's onDir.
• No bugs were found by the bug-hunting system on the current revision.
• The PR reports 50/50 tests passing in cp.test.ts and all 77 test-fs-cp-* Node parallel tests passing, including the two that specifically cover this sync/async asymmetry.
• Tests are placed in the existing test/js/node/fs/cp.test.ts alongside related coverage, use tempDirWithFiles, and assert exact error codes and paths.

[claude]

LGTM — the mirror comment on the async side is in now, and the fix + tests match node's cpSync/cp split cleanly.

Extended reasoning...

Overview

The runtime change is a 9-line deletion in src/js/internal/fs/cp-sync.ts: the errorOnExist && !force branch in onDir() is removed so cpSync merges into an existing destination directory and only raises ERR_FS_CP_EEXIST per colliding file (via the existing mayCopyFile). A 3-line comment is added there, and — following my earlier nit — a mirrored 3-line comment is added above the async onDir() in src/js/internal/fs/cp.ts, which intentionally keeps the directory-level check. Three tests are added to test/js/node/fs/cp.test.ts pinning (a) sync merge into an existing dir, (b) sync per-file EEXIST on a nested collision, and (c) async promises.cp still refusing the directory.

Security risks

None. This is a Node.js-compat semantics fix in JS-side ported code; it changes when an error is thrown, not what is written or where. No path handling, permissions, or trust boundaries change.

Level of scrutiny

Low. The touched code is a near-verbatim port of node's lib/internal/fs/cp/cp-sync.js, and the deleted branch is demonstrably absent from that upstream file — it was accidentally copied over from node's async cp.js in a prior PR. The description includes a differential table against node v26.3.0 across seven scenarios and confirms the 77 ported test-fs-cp-* tests still pass. The async side is unchanged in behavior (comment only).

Other factors

My previous review flagged only the missing async-side comment; that was addressed in 30c75b0 and the thread is resolved. The new tests are well-placed, use the harness helpers correctly, assert exact directory contents and error .code/.path, and lock in the intentional sync/async divergence so it can't be "fixed" by mistake later. No CODEOWNERS cover these paths.

[robobun]

Status: diff is ready, CI is red on a darwin-aarch64 infra failure

The change itself is green. The only hard CI failure is a Buildkite artifact download timing out on the macOS aarch64 agent, which kills the job before a single test runs:

2026-07-06 03:29:17 INFO   Searching for artifacts: "**" within step: "darwin-aarch64-build-bun"
2026-07-06 03:29:17 INFO   Found 2 artifacts. Starting to download to: .../bun/release

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).
    at getExecPathFromBuildKite (scripts/runner.node.mjs:2182:13)

Zero test failures in that job, because no tests executed.

This is not specific to this PR

The same artifact download timed out after 120s for step 'darwin-aarch64-build-bun' is failing darwin aarch64 - test-bun on at least six other unrelated branches at the same time, each with zero test failures in the job. For example #33413's build (farm/4d1aec15/zstd-decompress-truncated-frame) hits it identically. The agent appears unable to pull the build artifact inside the 120s budget.

Across the two runs of this branch, nothing red ever touched fs.cp:

run	hard failures	cause
build 68747	darwin 26 aarch64	artifact download timeout, no tests ran
	darwin 14 x64	terminal.test.ts PTY spawn timed out at 90s (88/90 of that file passed; it has no fs.cp usage). Did not recur.
build 68788	darwin 26 aarch64	artifact download timeout, no tests ran

Everything else was already inside Buildkite's own flaky annotation (spawn-pipe-leak, sql-onconnect-onclose-throw, napi, node-http-uaf, bun-security-scanner-workspaces, serve, filesink, sql round-trip). Build 68788 has now finished its test lanes: 283 jobs passed, 1 failed, and it produced no error-style annotation at all. The single red is the artifact-download timeout above.

Why the diff cannot be responsible

This PR changes three files, all fs.cp: a 9-line deletion in src/js/internal/fs/cp-sync.ts, a comment in src/js/internal/fs/cp.ts, and tests. It cannot reach a PTY spawn or a Buildkite artifact download.

It also cannot affect macOS's clonefile() fast path, which is the one darwin-specific thing in cpSync. That path is gated in src/js/node/fs.ts:

if (!filter && !dereference && !preserveTimestamps && !verbatimSymlinks && !mode && !errorOnExist && force) {
  const { ok, checked } = tryNativeFastPathSync(src, dest, options);

With errorOnExist: true the guard is false, so tryNativeFastPathSync is never called. And for the calls that do reach it (errorOnExist: false), the branch I deleted was opts.errorOnExist && !opts.force, already dead because its first conjunct is false. Those calls are byte-for-byte unchanged.

Verification

• bun bd test test/js/node/fs/cp.test.ts: 50 pass, 0 fail (3 new tests; 2 of the 3 fail without the src change)
• all 77 test/js/node/test/parallel/test-fs-cp-* pass, including test-fs-cp-async-dir-exists-error-on-exist.mjs and test-fs-cp-sync-error-on-exist.mjs
• 7-scenario differential against node v26.3.0 matches row for row, for both cpSync and promises.cp

I have used my one re-roll and am not going to keep pushing empty commits. This needs a maintainer to retry the darwin lane or merge through it.