shell(cp): name the operand as written in the builtin's stat errors - #39238
shell(cp): name the operand as written in the builtin's stat errors#39238robobun wants to merge 3 commits into
Conversation
The cp builtin resolves each operand against the shell's cwd before
classifying it and reported that resolved path when the stat failed
("cp: No such file or directory: /tmp/x/missing.txt"). ls, rm and mv
and the system cp name the operand as the user wrote it. is_dir now
takes the operand alongside the resolved path and tags the error with
it, on both the POSIX (lstat) and Windows (GetFileAttributes) branches.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe Changescp operand error reporting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:55 PM PT - Aug 15th, 2026
✅ @robobun, your commit fd7c816381605bce7681a078f5a79e23e9456442 passed in 🧪 To try this PR locally: bunx bun-pr 39238That installs a local version of the PR into your bun-39238 --bun |
|
Status: fix pushed (two lines in
|
There was a problem hiding this comment.
LGTM — small, focused error-message fix with thorough test coverage.
What was reviewed:
with_pathpreserves errno/syscall (src/sys/Error.rs:220), so the target-side ENOENT branch at cp.rs:655 still fires correctly.- Windows EBUSY path-matching is unaffected:
src_absolute/tgt_absoluteare only set after bothis_dircalls succeed, so stat errors were never candidates before or after. - Test follows harness conventions (subprocess with flag set so it runs on POSIX,
tempDir, spreadbunEnv, concurrent pipe drain,sortedShellOutputfor the racy multi-source case, exitCode last).
Extended reasoning...
Overview
This PR changes the shell cp builtin so that stat errors name the operand as the user typed it rather than the cwd-resolved absolute path. The Rust change is three lines: is_dir gains an operand: &[u8] parameter and both platform branches tag the error with it via with_path(operand); the two call sites in run_from_thread_pool_impl pass &self.src / &self.tgt. A new test in test/js/bun/shell/commands/cp.test.ts pins the message shape across seven cases.
Security risks
None. This only affects which path string appears in a user-facing error message; no new inputs are parsed, no allocations change shape, and the errno/syscall carried on the error are preserved unchanged.
Level of scrutiny
Low-to-medium. The behavioral surface is a diagnostic string in an experimental builtin (POSIX-gated behind BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1, on by default only on Windows). I verified the two places downstream that inspect the error object rather than just printing it: (1) the target-side e.get_errno() == ENOENT branch — with_path copies errno and syscall verbatim (src/sys/Error.rs:220-227), so this is unchanged; (2) the Windows EBUSY dedup, which compares sys.path against src_absolute/tgt_absolute — those fields are assigned only after both is_dir calls return Ok, so a stat-error path was never eligible for that comparison and still isn't. The pattern also mirrors the existing shell_statat precedent in interpreter.rs:2231.
Other factors
The test is well-constructed: it spawns a child with the builtin flag set (so it exercises the Rust path on POSIX where the enclosing describe is skipped), covers ENOENT on the source in three spellings including ./, the -R variant, a mixed-success multi-source run (asserting the good file is still copied and each bad operand is named on its own line, order-normalized via sortedShellOutput), and the ENOTDIR case for both source and target on POSIX. The Windows carve-out is justified — get_file_attributes returning None is always mapped to ENOENT, so the target side cannot fail there. Harness conventions are followed (tempDir with using, {...bunEnv, ...}, concurrent Promise.all on stdout/stderr/exited, exitCode asserted last). The PR description documents USE_SYSTEM_BUN=1 failing and bun bd test passing, plus a 22-case behavioral diff against 1.4.0. No prior review comments to address.
Keeps is_dir as it is on main and puts the operand on the error where it is reported, the way ls and rm tag theirs; the stat calls themselves are being restructured by other changes and these two lines are not.
There was a problem hiding this comment.
LGTM — two-line error-message fix that re-tags stat failures with the operand as written, matching ls/rm/mv and system cp.
What was reviewed:
- Verified
with_pathpreserves errno/syscall, so the target ENOENT arm (matched first) and exit code are unaffected. - Confirmed the Windows EBUSY pass compares against
src_absolute/tgt_absolute, which are set only after both stat checks succeed — re-tagged stat errors never reach it. - Test covers source/target, relative/unnormalized/subdir spellings,
-R, mixed good+bad sources (good one still copied), and POSIX-only ENOTDIR at both sites; child-process spawn forces the builtin on POSIX.
Extended reasoning...
Overview
This PR changes two lines in src/runtime/shell/builtin/cp.rs: the two Err arms in run_from_thread_pool_impl that report a stat failure now re-tag the bun_sys::Error with self.src / self.tgt (the operand as the user typed it) via .with_path(...) before wrapping in ShellErr::new_sys. Previously the error carried the cwd-resolved absolute path. A new test in test/js/bun/shell/commands/cp.test.ts pins the behavior across seven cases.
Security risks
None. This only changes which path string appears in a user-facing error message for a shell builtin. No auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
Low. The production change is two one-line edits on error-reporting paths of an experimental builtin (opt-in on POSIX, default on Windows). I verified bun_sys::Error::with_path copies errno and syscall and only replaces path, so the ENOENT guard on the target arm (matched before the catch-all) is unaffected and the exit code stays 1. The pattern mirrors rm.rs's error_with_path. The Windows EBUSY dedup compares error paths against src_absolute/tgt_absolute, which are populated only after both stat checks pass — so a stat-failure error was never a candidate for that comparison and still isn't.
Other factors
The test is well-constructed: it spawns a child with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 so it exercises the builtin on POSIX (the in-process describe block is gated off there), asserts exact stderr per case, sorts the multi-source stderr to handle work-pool ordering, verifies the good source among failing ones is still copied, and gates the ENOTDIR cases on POSIX since Windows's is_dir maps every failure to ENOENT (making the target arm unreachable there). The PR description documents that the test fails under USE_SYSTEM_BUN=1 and passes with the debug build. The one prior review comment (comment-cop about a long comment) was addressed in fd7c816. No outstanding reviewer concerns.
Problem
cpbuiltin (on by default on Windows,BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1on POSIX) names the path it resolved instead of the operand the user typed when an operand cannot be stat-ed. With cwd/tmp/x, on bun 1.4.0:cp: cannot stat 'missing.txt': No such file or directory, BSD cp printscp: missing.txt: No such file or directory, and bun'sls,rmandmvbuiltins printmissing.txt(shell: report the operand as written in cat, touch and mkdir errors #39189 does the same for cat, touch and mkdir). cp's own messages for the other operand problems (X is a directory (not copied),directory X does not exist, ...) already use the operand as written, so one command mixes both forms.ShellCpTask::run_from_thread_pool_impl(src/runtime/shell/builtin/cp.rs) joinsself.src/self.tgtonto the cwd and stats the joined paths throughis_dir, whose error carries the path it stat-ed (lstattags it on POSIX, the Windows branch tags it explicitly). The twoErrarms pass that error toShellErr::new_sysunchanged, andBuiltin::shell_err_to_stringprints whatever path it carries.Fix
Errarms that report a stat failure (source check and target check) re-tag the error with the operand,e.with_path(&self.src)/e.with_path(&self.tgt), before it becomes aShellErr. Two lines;is_diris unchanged.with_pathcopies errno and syscall, so the target arm that treats ENOENT as "target does not exist yet" (matched before this arm) is unaffected, and the exit code is still 1.is_dirfor two reasons: that is wherelsandrmtag theirs (error_with_path), and several open PRs restructure the stat calls themselves (shell(cp): report ENAMETOOLONG instead of panicking on operands longer than the path buffers #38162 resolves the operands through a new helper, shell(cp): copy the file a symlink operand points at unless -R is given #37954 stats the source directly, shell(cp): classify the target operand through symlinks #37956 classifies the target through symlinks) while leaving these twoErrarms as they are, so the change merges with each of them either way. The test pins the message for whichever lands later.src_absolute/tgt_absolute, but those are set only after both checks pass, so a stat error was never a candidate for it and is not now. Errors from the copy itself (cp_on_finish) still carry the absolute paths that pass relies on.cp: <message>: <path>, where the other builtins print<path>: <message>) and the absolute paths in errors raised during the copy. Both would change messages that the open cp PRs above pin in their tests, and the second needs the copied tree's paths mapped back onto the operands; they are separate changes if wanted.test/js/bun/shell/commands/cp.test.ts, "stat errors name the operand as written". Runs the builtin in a child process with the flag set (the in-process suite in that file is skipped on POSIX) and covers a missing source spelledmissing.txt,sub/missing.txtand./missing.txt(as written, not normalized), a missing source with-R, two missing sources around a good one (each line names its own operand, the good one is still copied, exit 1), and on POSIX the non-ENOENT case at both sites:f.txt/xas source andg.txt/xas target, bothNot a directory. On Windows the builtin maps every stat failure to ENOENT, so the target arm cannot be reached there and those two cases are POSIX-only.USE_SYSTEM_BUN=1 bun test test/js/bun/shell/commands/cp.test.ts: the new test fails; every case differs only by the path in the message.bun bd test test/js/bun/shell/commands/cp.test.ts: passes.-v, trailing slashes, identical files, the Custom messages, an empty operand, resulting file trees) gives identical output on 1.4.0 and this build except for the six stat-error messages, which now name the operand; list below.cargo clippy -p bun_runtime --no-deps,cargo fmt --checkandcargo check -p bun_runtime --target x86_64-pc-windows-msvcpass.Background
src/runtime/shell/builtin/and run in-process.cpis dispatched by default on Windows only; on POSIX it needsBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1, otherwise the system binary is spawned.ShellCpTaskis created per source operand and runs on a work-pool thread. It resolves both operands to absolute paths (the node:fs copy it hands off to does not know the shell's cwd), stats them to pick the POSIX cp synopsis, then callsfs.cp. Those two stat checks are the only place the builtin itself reports an error about an operand; everything after that is reported by node:fs with the paths it was given.bun_sys::Errorcarries an errno, a syscall tag and a path;with_pathreturns a copy with a different path.ShellErr::Sysis how builtins report such errors; for cp it is rendered byshell_err_to_stringascp: <coreutils message>: <path>and turned into exit code 1.Probe: output identical on bun 1.4.0 and this build except for these six lines
Run with
BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1on Linux in a directory with filesa,b, directoriesdirandsub/inner(containingc); exit code, stdout, stderr and the resulting tree compared for:cp -v a new,cp -v a b,cp -v a dir,cp -v a dir/,cp -v a b dir,cp a b new,cp a ./a,cp -R sub dir,cp -R sub newdir,cp -R sub/ newdir2,cp -R a dir2,cp -R a b nodir,cp sub x,cp missing x,cp ./missing x,cp sub/missing x,cp /no-such-dir/missing x,cp a/x y,cp a b/x,cp -R a b/x,cp a b dir/nope,cp "" x.Earlier shape of this PR
The first revision gave
is_diranoperandparameter and tagged the error inside it, on both platform branches. Self-review pointed out that the open PRs listed above replace or remove theis_dircalls while keeping the twoErrarms, so the re-tag moved to the arms andis_dirwent back to what it is on main. The behavior and the test are the same in both revisions.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