shell(cp): print the -v line only for a single-file copy that succeeded

[robobun]

Problem

• The Bun Shell cp -v builtin prints src -> dest on stdout for a single-file copy that did not happen. With d/b.txt being a directory, cp -v b.txt d exits 1 and prints cp: Is a directory: .../d/b.txt on stderr, but stdout still says .../b.txt -> .../d/b.txt. Same for any other failing copy (a read-only destination as a non-root user, for example), and on Linux/FreeBSD also for a symlink source whose destination already exists, where the copy is skipped but the line was printed anyway.
• Cause: the two single-file arms of NewAsyncCpTask::cp_async (src/runtime/node/node_fs.rs, one #[cfg(windows)], one POSIX) called this.on_copy(src, dest) regardless of what copy_single_file_sync returned. on_copy is what appends the verbose line (ShellCpTask::cp_on_copy in src/runtime/shell/builtin/cp.rs).
• The directory walk had its own copy of the decision (CpSingleTask::run_owned) and got it right; the two single-file arms had also drifted from each other (only the POSIX one printed the line for a tolerated EEXIST).

Fix

• One NewAsyncCpTask::record_copy_result(src, dest, result) now handles a file's result for all three sites (both single-file arms and the walk): on_copy on Ok, nothing for an EEXIST the flags tolerate, finish_concurrently(Err) otherwise. Success is not recorded explicitly; on_subtask_done already resolves with Ok when no error was recorded, which is how the walk (and the macOS clonefile path) completed before.
• Why this is right: coreutils and BSD cp -v print a -> b only for copies they made, cp_on_copy is documented as being called per successfully copied file, and the walk already behaved this way. The error on stderr and the exit code were already correct; this only removes the line. Net effect for the single-file arms is the same as before except that on_copy is no longer called on Err.
• fs.cp / fs.promises.cp are unaffected: on_copy is a no-op for them, and each result still resolves or rejects the same way (Ok and a tolerated EEXIST resolve, anything else rejects with that error).
• The tolerated-EEXIST case is reachable from the shell today only through a symlink source onto an existing destination (cp_symlink cannot replace the destination and the shell passes force: true, so the EEXIST is swallowed). This PR stops printing the line there; that the copy is skipped silently with exit 0 instead of replacing the destination like cp(1) is a separate, pre-existing bug and is tracked separately.
• Verified with test/js/bun/shell/commands/cp.test.ts. The new block runs cp -v through the builtin in a child bun (BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1, so it is exercised on every platform): a failing copy, file file dir where one file fails (only the copied one is listed, and it was copied), a read-only destination (skipped as root), cp -R -v with one file in the tree failing (the directories and the other file are listed, the failed file is not), and a symlink source onto an existing file (listed only if the destination actually became the link).
  • USE_SYSTEM_BUN=1 bun test test/js/bun/shell/commands/cp.test.ts: 3 fail (failing copy, mixed, symlink); the read-only one also fails when run as an unprivileged user; the -R one passes before and after, it pins the walk's behaviour now that it shares the helper.
  • bun bd test test/js/bun/shell/commands/cp.test.ts: all pass (the read-only one verified as an unprivileged user as well).
  • bun bd test test/js/node/fs/cp.test.ts, cp-symlink-target.test.ts, and the upstream test-fs-cp-async-* file-to-file / overwrite / nested-tree cases: pass.
  • cargo check -p bun_runtime --target x86_64-pc-windows-msvc is clean (one of the arms is Windows-only); the Windows lanes of the first CI run were green as well.

Background

• The shell cp builtin (always used on Windows, opt-in via BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 elsewhere) does not copy files itself. For each source operand it creates a ShellCpTask, which resolves the operands and hands the copy to the native implementation behind fs.cp, NewAsyncCpTask, instantiated with IS_SHELL = true.
• NewAsyncCpTask::cp_async runs on a work-pool thread. A non-directory source is copied right there with copy_single_file_sync; a directory is walked, and each file inside becomes a CpSingleTask on the pool. The task holds a subtask count; when the last holder calls on_subtask_done, the result is delivered to the JS thread, as Ok unless some holder recorded an error with finish_concurrently (first error wins, later files still get copied).
• on_copy(src, dest) feeds -v: for IS_SHELL it appends src -> dest to a buffer the shell writes to stdout when the task completes; for node:fs it does nothing. The walk also calls it once per directory after creating it.
• Tolerated EEXIST: fs.cp with force: false copies with COPYFILE_EXCL and treats an existing destination as "skip", unless errorOnExist was requested. The shell never asks for that (force: true), so for it the case only comes up via cp_symlink, as described above.

Earlier version of this PR

The first push changed only the two single-file arms (a finish_single_file helper that still called finish_concurrently itself) and left the walk's own copy of the logic in place. Review pointed out that the walk could share the helper, that nothing exercised -R -v, and that the tolerated-EEXIST arm is in fact reachable from the shell (symlink source onto an existing destination), so the helper was moved to cover all three sites and the -R and symlink tests were added.

---

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/bun/shell/commands/cp.test.ts

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 859c6743-303a-48ab-8f90-814a8c10e70e

📥 Commits

Reviewing files that changed from the base of the PR and between 74c2457 and cc178c8.

📒 Files selected for processing (2)

• src/runtime/node/node_fs.rs
• test/js/bun/shell/commands/cp.test.ts

---

Walkthrough

Async cp completion handling now uses one shared result path. Tests cover verbose output and destination state for failed, recursive, read-only, and symlink copies.

Changes

Async cp reporting

Layer / File(s)	Summary
Shared copy-result handling src/runtime/node/node_fs.rs	Single-file async copy paths use record_copy_result for successful copies, tolerated EEXIST errors, and other failures.
Verbose copy behavior tests test/js/bun/shell/commands/cp.test.ts	Concurrent child-process tests verify output and destination state for failed, recursive, read-only, and symlink copy operations.

Possibly related PRs

• oven-sh/bun#37693: Modifies related async cp completion paths in src/runtime/node/node_fs.rs.

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

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly and concisely describes the main change: verbose output appears only after a successful single-file copy.
Description check	✅ Passed	The description explains the problem, cause, fix, scope, and verification results, including platform-specific test limitations.

---

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

[robobun]

^{Updated 3:05 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 2d7bfb6 has some failures in Build #94310 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37967

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

bun-37967 --bun

[robobun]

Status: ready for review (head 2d7bfb6).

Reproduced on bun 1.4.0 with the builtin enabled (BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1): with d/b.txt a directory, cp -v b.txt d exits 1 and prints cp: Is a directory: .../d/b.txt on stderr, but stdout also contains .../b.txt -> .../d/b.txt. The read-only destination case from the report reproduces the same way as an unprivileged user, and so does a symlink source copied onto an existing file (line printed, nothing copied).

The new tests in test/js/bun/shell/commands/cp.test.ts fail on the unfixed binary (failing copy, mixed multi-source, symlink; the read-only one when run as a non-root user, it is skipped as root) and pass with this branch; the -R -v one passes both ways and pins the directory walk, which now shares the same code.

CI: every Linux (glibc, musl, ASAN) and Windows (x64, arm64) test job passed on both runs (builds 93747 and 94310); the only test failures were unrelated tests that passed on retry. The four macOS jobs did not run in either build (canceled before starting in the first, expired waiting for an agent in the second), so macOS has only been checked by reading the code: the helper is platform-independent, and the symlink test is written to hold whichever way copyfile(3) treats an existing destination there. Re-running the macOS jobs on the Buildkite build is all that is left.

[claude]

LGTM — clean fix that gates the -v line on copy success and unifies the two divergent single-file arms into one helper.

What was reviewed:

• finish_single_file vs the removed inline blocks: Ok/Err/tolerated-EEXIST paths deliver the same result to finish_concurrently as before; only the spurious on_copy on failure is dropped.
• The POSIX-arm change (no on_copy on tolerated EEXIST) is unobservable: shell passes force: true, error_on_exist: false so EXCL is never set, and on_copy is a no-op for node:fs.
• e.get_errno() == E::EEXIST is equivalent to the old e.errno == E::EEXIST as _ on both platforms (Error::get_errno round-trips the stored discriminant).
• New tests: hermetic tempdirs, pipes drained concurrently, child bun with the builtin env var so POSIX is covered; the mixed-outcome test's stdout is order-independent since only the succeeding copy emits a line.

Extended reasoning...

Overview

The PR fixes a cosmetic-but-wrong behaviour in Bun Shell's cp -v builtin: when a single-file copy fails (e.g. destination path is a directory, or read-only target), the src -> dest line was still printed to stdout even though stderr and the exit code correctly signalled failure. The root cause was two near-duplicate arms in NewAsyncCpTask::cp_async (one #[cfg(windows)], one POSIX) that called on_copy(src, dest) unconditionally after copy_single_file_sync returned, before checking whether the result was Ok.

The fix extracts a small finish_single_file(&self, src, dest, result) helper that both arms now call. It records on_copy only in the Ok arm, converts a tolerated EEXIST (!error_on_exist) to Ok(()) without recording a line, and passes any other error through. This mirrors what CpSingleTask::run_owned (the directory-walk per-file task) already does, and what cp_on_copy's doc comment says ("for every successfully-copied file"). Three new tests in cp.test.ts exercise the failing-copy, mixed-outcome, and read-only cases through a child bun with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 so the builtin runs on all platforms.

Security risks

None. This is output-formatting logic in a shell builtin; no parsing of untrusted input, no auth/crypto/permissions surface. The change narrows behaviour (prints less), and does not alter what gets copied, what errors are surfaced, or the exit code.

Level of scrutiny

Low-to-medium. The diff is small (net −7 in the Rust file), mechanical, and the helper is a straight refactor of the two removed blocks with the one intended semantic change. I traced each of the three result cases (Ok, tolerated EEXIST, other Err) against both old arms and confirmed finish_concurrently receives the same value in every case; the only observable difference is the removed on_copy call on the error path, which is exactly the bug being fixed. The switch from e.errno == E::EEXIST as _ to e.get_errno() == E::EEXIST is equivalent — Error::get_errno round-trips the stored discriminant on both POSIX and Windows. self.args is the same struct the old code accessed via the local args = &this.args binding.

Other factors

• The one intentional divergence between old arms (POSIX called on_copy on tolerated EEXIST, Windows did not) is resolved to the Windows/CpSingleTask behaviour. I verified this is unobservable: the shell hard-codes force: true, error_on_exist: false (src/runtime/shell/builtin/cp.rs:712-716) so COPYFILE_EXCL is never set on the shell path and EEXIST isn't returned there; for node:fs, on_copy early-returns when !IS_SHELL.
• Tests follow repo conventions: tempDir+using, bunEnv spread, concurrent stdout/stderr/exited drain, await using on the spawn, skipIf(isRoot) for the permissions case with a try/finally chmod restore. The mixed-outcome test's stdout assertion is order-independent because only one source succeeds. join() produces the platform separator so the Windows path shape matches.
• PR description reports the tests fail under USE_SYSTEM_BUN=1 and pass under bun bd, and that cargo check on the Windows target is clean.

[claude]

I reviewed this PR and didn't find any bugs. The record_copy_result refactor looks correct — the scopeguard at cp_async's entry guarantees on_subtask_done runs on every return, and that path already filled in Ok(()) when no result was recorded, so dropping the explicit finish_concurrently(Ok(())) from the single-file arms is safe and matches how CpSingleTask already worked. Because the helper is shared with fs.cp/fs.promises.cp and the comment-cop flag on the new doc comment is still outstanding, a human look would still be worthwhile.

What was reviewed:

• Traced all three call sites of record_copy_result and confirmed on_subtask_done is reached on every path (via the scopeguard in cp_async and the explicit call in CpSingleTask::run_owned).
• Confirmed on_copy is a no-op when IS_SHELL = false, so fs.cp behavior is unchanged (same result delivered on Ok / tolerated EEXIST / Err).
• Checked get_errno() vs the old errno == E::EEXIST as _ — the helper canonicalizes libuv codes on Windows, so the switch is correct.
• New tests follow harness conventions (subprocess isolation for the once-per-process env flag, concurrent pipe drain, tempDir, sorted comparison for the concurrent -R walk).

Extended reasoning...

Overview

Two files: src/runtime/node/node_fs.rs extracts a record_copy_result helper that all three per-file result sites (the Windows single-file arm, the POSIX single-file arm, and CpSingleTask::run_owned in the directory walk) now call; test/js/bun/shell/commands/cp.test.ts adds five subprocess-isolated tests exercising cp -v failure modes through the builtin.

The functional change is that on_copy (which appends the -v line for the shell) now fires only on Ok(()). The two single-file arms previously called it unconditionally (and disagreed with each other on the tolerated-EEXIST path); the walk already gated it on Ok. The helper also stops calling finish_concurrently(Ok(())) explicitly on success, relying instead on on_subtask_done (line 1670) to fill in Ok(()) when has_result is unset — exactly the mechanism CpSingleTask already depended on.

Security risks

None. This is cosmetic -v stdout output for the shell cp builtin. No parsing of untrusted input, no path handling changes, no permission logic.

Level of scrutiny

Medium. The logic change itself is small and well-justified against coreutils/BSD behavior, and the deduplication is exactly what REVIEW.md asks for ("the second time a multi-line block appears in your diff, extract a named helper"). But node_fs.rs backs both shell cp and fs.cp/fs.promises.cp, and the refactor removes an explicit finish_concurrently(Ok(())) in favor of an implicit fill-in — that's the kind of "refactor is guilty until proven behavior-preserving" change a human should sign off on. I traced it and it holds (the scopeguard::guard(this, Self::on_subtask_done) at line 1790 covers every return from cp_async, and run_owned calls on_subtask_done explicitly at line 1488), but it's subtle enough to warrant a second pair of eyes.

Other factors

• The comment-cop bot flagged the 6-line doc comment on record_copy_result (line 1528) after cc178c8, and it hasn't been addressed. It reads to me as a false positive — it's a function doc comment stating a contract, not a workaround justification, and neighbouring helpers (finish_concurrently, on_subtask_done) carry similar-length docs — but it's an outstanding automated review item.
• The .errno == E::EEXIST as _ → .get_errno() == E::EEXIST switch is a quiet improvement: get_errno canonicalizes libuv codes on Windows.
• Tests look solid: subprocess isolation (the enable flag is read once per process), describe.concurrent, await using + concurrent pipe drain, sortedShellOutput for the -R case where files copy in any order, skipIf(isRoot) on the read-only test with a chmod restore in finally. The symlink test relies on symlinkSync succeeding, which on Windows CI needs Developer Mode — worth a glance but likely fine given other bun tests do the same.

[claude]

LGTM — focused fix that consolidates three duplicated result-handling arms into one record_copy_result helper and stops cp -v from printing lines for copies that failed or were skipped.

What was reviewed:

• Verified the completion path: cp_async has a scopeguard calling on_subtask_done, which sets Ok(()) when no result was recorded — so dropping the explicit finish_concurrently(Ok(())) on success is behavior-preserving for both fs.cp and the shell.
• Checked that on_copy is a no-op when !IS_SHELL, so fs.cp/fs.promises.cp resolve/reject exactly as before.
• Confirmed e.get_errno() == E::EEXIST is the checked accessor and equivalent to (safer than) the previous raw errno == E::EEXIST as _ comparison.
• Tests follow harness conventions (subprocess with bunEnv spread, await using, drained pipes, skipIf(isRoot), chmod restored in finally, sortedShellOutput for concurrent walk output).

Extended reasoning...

Overview

The PR fixes a bug in the Bun shell cp -v builtin where the src -> dest verbose line was printed even when the copy failed (e.g., destination is a directory, read-only, or a symlink onto an existing file where the EEXIST is tolerated). The root cause was that the two single-file arms of NewAsyncCpTask::cp_async (one #[cfg(windows)], one POSIX) called on_copy unconditionally after copy_single_file_sync, and had drifted from each other and from the directory-walk path (CpSingleTask::run_owned), which already got it right.

The fix introduces NewAsyncCpTask::record_copy_result(src, dest, result) that all three sites now call: on_copy on Ok, no-op for a tolerated EEXIST, finish_concurrently(Err) otherwise. Net native diff is ~-20 lines. The single-file arms no longer explicitly record Ok(()) — I traced that cp_async installs a scopeguard (_done) invoking on_subtask_done, which writes Ok(()) when has_result is still false, so the success path resolves identically. The macOS clonefile fast path already relied on this same mechanism.

Security risks

None. This touches only how the verbose-output callback and result are dispatched after a copy completes; no path parsing, no permission checks, no untrusted-input handling changed.

Level of scrutiny

Low-medium. This is a targeted bugfix in an experimental shell builtin (opt-in via BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 on POSIX, default only on Windows). The change is a deduplication that makes three call sites share the walk's already-correct logic. The only subtle bit — relying on on_subtask_done to record success — is exactly what the walk and clonefile paths already did, and I verified it by reading finish_concurrently and on_subtask_done in node_fs.rs. fs.cp is unaffected because on_copy early-returns when !IS_SHELL.

Other factors

Five new tests cover the failing single-file case, mixed success/failure with multiple sources, read-only destination (skipped as root), -R -v with one file failing (pins the walk now that it shares the helper), and the symlink-onto-existing-file case (asserts the -v line agrees with what actually happened to the destination). The tests spawn a child bun with the builtin enabled so they exercise the code on every platform, use test.concurrent, drain both pipes with Promise.all, and restore chmod in finally. The comment-cop bot's feedback about the long doc comment was addressed in 2d7bfb6 (comment removed). No CODEOWNERS for these paths.