Conversation
cp's parse_short returned Some(ContinueParsing) for -R/-v/-n, which parse_one_flag treats as "done with this argument" and advances to the next argv entry. In a combined cluster like -Rv that meant only the first flag was applied: -Rv dropped verbose, -vR dropped recursive. Return None (keep iterating the cluster) for accepted flags, matching the mkdir builtin. On macOS the recursive path also shortcuts through clonefile(), which copies the whole tree without a per-file callback; skip that fast path when the shell asked for verbose output so each copy is reported.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangescp combined flags
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced with Review addressed: accept Fail-before/pass-after verified locally on Linux. CI status: three runs (80890, 81017, 81123) with zero test failures touching this diff. Rust compile finished successfully in every build-bun log; the red is all unrelated infra:
94 test shards passed on 81017 including the shell cp suite. Diff is ready for a maintainer to merge past the infra noise. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:33 AM PT - Jul 25th, 2026
⏳ @robobun, your commit ef8fb21 is still building in
Add |
||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The parse_short fix is straightforward and matches mkdir's pattern exactly, and tests look solid. Deferring for a human glance at the node_fs.rs piece: skipping the macOS clonefile() fast path for shell -v is a reasonable trade-off, but it's a deliberate perf change in shared fs.cp code and comment-cop flagged the block (mostly re-indent noise, but worth a look).
What was reviewed
parse_one_flagat interpreter.rs:2352 confirmsSome(_)exits the cluster loop; returningNoneis the documented contract and matches mkdir.rs:444-451.skip_clonefileguard:IS_SHELLis const-generic,ParentRef<ShellCpTask>isCopy+Deref,optsis set once pre-schedule — cross-thread read is the same patternon_copyalready uses;fs.cp(IS_SHELL=false) short-circuits and is unaffected.- Tests drain pipes concurrently, cover both
-Rv/-vRorderings plus-Rvn, and force the builtin on POSIX via env var.
Extended reasoning...
Overview
Three files: (1) src/runtime/shell/builtin/cp.rs — change three parse_short arms from Some(ContinueParsing) to None so combined -Rv/-vR clusters are fully consumed; (2) src/runtime/node/node_fs.rs — on macOS, skip the clonefile() whole-tree fast path when the shell caller requested verbose output, so the per-file directory walk fires on_copy for each entry; (3) new tests in test/js/bun/shell/commands/cp.test.ts.
Security risks
None. No untrusted-input parsing beyond single-byte flag matching that already existed; no new syscalls or privilege boundaries. The ParentRef deref reads an immutable bool set before the task was scheduled, following the same pattern on_copy already uses from work-pool threads.
Level of scrutiny
The cp.rs change is mechanical — I verified parse_one_flag (interpreter.rs:2338-2356) returns immediately on any Some(_) from parse_short, so the old code demonstrably dropped trailing cluster bytes, and the fix mirrors mkdir's parse_short byte-for-byte. Low scrutiny needed there.
The node_fs.rs change is where I'd like a human: it wraps the existing macOS clonefile block in if !skip_clonefile, where skip_clonefile = IS_SHELL && this_ref.shelltask.is_some_and(|s| s.opts.verbose). It's narrowly gated (const-generic IS_SHELL short-circuits for node fs.cp; only shell + -v skips), and the fallback directory-walk path is what already runs on Linux/Windows. But it is a deliberate perf-for-correctness trade in a shared file, and a maintainer may prefer an alternative (e.g. emitting a single verbose line after clonefile succeeds, or walking the cloned tree post-hoc). That's a design call, not a bug.
Other factors
- comment-cop flagged four lines in
node_fs.rs. Three (1995/2004/2009) are pre-existing comments that were merely re-indented inside the newif— pure noise. The fourth (1989-1991) is the new 3-line explanation of why clonefile is skipped; it reads as a legitimate why-comment to me, not a workaround justification, but the author should decide whether to trim or dismiss. - My prior inline note on the pre-existing
b'p' → "-P"typo is explicitly non-blocking and unrelated to this diff's changes. - Tests follow harness conventions:
tempDir,{...bunEnv, ...}, concurrent pipe drain, exitCode asserted last,test.concurrentfor spawned subprocesses, andBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1so the parser is exercised on POSIX where the builtin is otherwise disabled.
Accept -f as a no-op (cp already hardcodes force=true) so the common
-Rf spelling keeps working instead of newly erroring. Change the -Rvn
test to -nRv/-fRv so the leading-flag arms are load-bearing. Use
.expect("IS_SHELL => shelltask") to match the two sibling IS_SHELL
sites in this file. Trim the clonefile comment to one line and fix the
pre-existing -p -> "-P" typo in the unsupported-flag message.
There was a problem hiding this comment.
LGTM — the -p typo from my earlier note is fixed, and the rest looks correct.
Checked parse_one_flag in interpreter.rs: returning None from parse_short is the documented way to keep iterating a -abc cluster, and this now matches mkdir's impl exactly.
Verified the macOS skip_clonefile guard: IS_SHELL && ... short-circuits so node fs.cp never touches .shelltask, and the .expect("IS_SHELL ⇒ shelltask") idiom is the same one already used in on_copy (same field, same thread context).
The -f → no-op change is safe given CpFlags { force: true } is hardcoded; the -Rp/-Ri/etc. now-erroring behavior change is intentional and called out.
Extended reasoning...
Overview
Three files: cp.rs changes the parse_short return values for -R/-v/-n/-f from Some(ContinueParsing)/Unsupported to None and fixes the -p error-message typo; node_fs.rs wraps the macOS clonefile() fast path in a skip_clonefile guard so shell cp -v falls through to the per-file walk that fires on_copy; cp.test.ts adds cluster-flag coverage both in the existing Windows-only TestBuilder block and in a cross-platform spawned block gated on BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1.
Correctness
I read parse_one_flag at interpreter.rs:2338-2357: it iterates each byte of the -abc cluster, and any Some(_) from parse_short immediately returns — so the old Some(ContinueParsing) broke out after the first letter. Returning None lets the loop finish and the function returns ContinueParsing itself. This is exactly what mkdir's parse_short (mkdir.rs:441-451) does for -p/-v. The -f no-op is justified by CpFlags { force: true, .. } hardcoded in run_from_thread_pool_impl. The intentional side effect (-Rp now errors instead of silently dropping p) is the honest behavior and is documented in the PR description.
For node_fs.rs, this_ref.shelltask.expect("IS_SHELL ⇒ shelltask").opts.verbose mirrors the existing pattern at line 1543-1545. Reading opts.verbose from the workpool thread is already done by on_copy → cp_on_copy, so there's no new thread-affinity concern. The IS_SHELL && short-circuit means the non-shell (fs.cp) path never dereferences shelltask and keeps the clonefile() fast path unchanged.
Security risks
None. This is CLI flag parsing for an experimental shell builtin (POSIX defaults to system /bin/cp); no auth, crypto, or untrusted-data parsing beyond argv bytes that were already being matched.
Level of scrutiny
Low-to-moderate. The core change is a handful of Some(ContinueParsing) → None swaps whose semantics are directly verifiable against the shared parser, plus a mechanical if !skip { ... } wrapper around an existing macOS block. The only user-visible behavior changes (-f accepted, -Rp etc. now erroring) are small, deliberate, and reasoned through in the description.
Other factors
All prior feedback is addressed: my earlier inline note about the b'p' → b"-P" typo is fixed in the diff, and the comment-cop long-comment complaints were resolved (one shortened, three were pre-existing re-indented lines). Tests follow harness conventions — tempDir, bunEnv spread, test.concurrent, concurrent pipe draining, output asserted before exit code — and assert both the -R effect (files copied) and the -v effect (per-file lines printed) so neither flag can silently regress. Fail-before/pass-after was verified by the author on Linux; macOS/Windows deferred to CI.
There was a problem hiding this comment.
LGTM — the -p/-P typo from my earlier pass is fixed, and the two inline notes this round are non-blocking nits (one-word comment direction, and pre-existing -n semantics that #34913 is already tracking).
Extended reasoning...
Overview
Three files: cp.rs changes four match-arm return values in Opts::parse_short (Some(ContinueParsing) → None for R/v/n, Unsupported → None for f) plus the -p message fix; node_fs.rs wraps the existing macOS clonefile() block in an if !skip_clonefile guard so shell cp -v takes the per-file walk path; cp.test.ts adds -Rv/-vR/-nRv/-fRv coverage both via TestBuilder and via spawned subprocesses with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 so the parser runs on every platform.
Security risks
None. This is CLI flag parsing for a shell builtin behind an experimental env var on POSIX (default on Windows). No untrusted-input bounds arithmetic, no auth/crypto, no new syscall paths — the only fs behavior change is skipping an optimization on macOS when verbose output is requested.
Level of scrutiny
Low-to-moderate. The core fix is a pattern-match to mkdir's parse_short (verified: mkdir returns None for accepted short flags at mkdir.rs:446,450). The macOS guard reuses the file's own .expect("IS_SHELL ⇒ shelltask") idiom (node_fs.rs:1544); ParentRef is Copy + Deref, and IS_SHELL is a const generic so the && short-circuits at compile time when false — no panic risk for node fs.cp. The intentional behavior change (-Rp/-Ri/etc now error instead of silently dropping the second letter) is called out in the description and is the more honest failure mode.
Other factors
My previous inline comment (-p reporting -P) was addressed in 83a706f. The comment-cop bot flags on node_fs.rs were resolved (pre-existing comments, only re-indented). Tests follow harness conventions: tempDir, {...bunEnv, ...}, test.concurrent, Promise.all pipe drain, exit-code asserted last. The two remaining findings are a one-word comment direction ("below" → "above") and pre-existing -n no-op semantics — neither affects correctness and neither is introduced by this PR.
There was a problem hiding this comment.
LGTM — both nits from my earlier pass are addressed (-p message in 83a706f, comment direction in ef8fb21). The open -n note is pre-existing behavior and explicitly non-blocking; fine to leave for #34913 or a follow-up.
Extended reasoning...
Overview
Fixes short-flag cluster parsing in the shell cp builtin: parse_short now returns None for accepted flags (R/v/n/f) so parse_one_flag keeps iterating the -abc cluster instead of bailing after the first letter. Also fixes the pre-existing -p→-P error-message typo, and gates the macOS clonefile() fast path off when the shell requested -v so per-file verbose output actually prints. Tests spawn with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 so the flag parser is exercised on every platform, plus TestBuilder cases in the existing Windows-only block.
Security risks
None. No untrusted-input parsing changes beyond which recognised letters set which boolean; no new syscalls, no privilege or path-handling changes.
Level of scrutiny
Low-to-moderate. The parse_short change is mechanical — verified against the documented FlagParser contract ("return None to keep iterating", interpreter.rs:2310) and against mkdir's parse_short which already does exactly this. The node_fs.rs change is a narrowly-scoped IS_SHELL && verbose guard around the existing clonefile block; it uses the same .expect("IS_SHELL ⇒ shelltask") convention as the two existing sites in the file and short-circuits on IS_SHELL so fs.cp (IS_SHELL=false) never evaluates shelltask and never loses the fast path. The intentional behavior change (-Rp/-Ri/etc. now error instead of silently dropping the unimplemented flag) is called out in the description and only affects the experimental builtin.
Other factors
Since my last review the author pushed ef8fb21 fixing the "below → above" comment nit; the -p typo was already fixed in 83a706f. The remaining open inline about -n silently accepting no-clobber is pre-existing (the old Some(ContinueParsing) was equally a silent accept for standalone -n), was flagged as non-blocking, and CodeRabbit noted #34913 touches -n semantics — reasonable to defer. New tests cover both cluster orderings (-Rv/-vR) and leading-flag arms (-nRv/-fRv), assert both effects (tree copied AND verbose lines printed), drain pipes concurrently, and use test.concurrent. The comment-cop bot flags on node_fs.rs were resolved as pre-existing comments merely re-indented by the new if wrapper.
This branch has not been deployed
1 participant
What does this PR do?
The shell
cpbuiltin only applied the first flag of a combined short-flag cluster and silently discarded the rest:On POSIX this is hidden because the builtin is disabled by default (falls through to /bin/cp); on Windows and with
BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1it's the only implementation.Cause
FlagParser::parse_shortis documented as "returnNoneto keep iterating" the-abccluster, andparse_one_flagreturns immediately on anySome(_).cp's impl returnedSome(ContinueParsing)for-R/-v/-n, soparse_one_flagadvanced to the next argv entry after the first recognised letter instead of the next letter in the cluster.Fix
Nonefor accepted short flags (R/v/n), matching themkdirbuiltin.-fas a no-op: the builtin already hardcodesforce: true, socp -Rf(very common) keeps working instead of newly erroring on the now-reachablef.-punsupported-flag message (was reporting-P).clonefile(), which copies the whole tree in one syscall with no per-fileon_copycallback. When the shell asked for verbose output, skip that fast path so the directory walk reports each copy. nodefs.cp(IS_SHELL=false) is unaffected.Intentional behavior change
cp -Rp/-Ri/-RH/-RL/-RPpreviously set-Rand silently dropped the second letter (so e.g.-Rpcopied without preserving attributes). They now error with "unsupported option" the same way standalone-p/-i/-H/-L/-Palready did. That's the honest answer: those flags aren't implemented.Verification
New tests spawn with
BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1so the builtin's flag parser runs on every platform, plusTestBuildercases inside the existing Windows-only block:Related to #35616 (which adds
-r/--verbosealiases; sameparse_shortsite).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