shell(rm): accept --force as the long form of -f - #39224
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (2)
Comment |
|
Reproduced on bun 1.4.0 and on a debug build of main: Fix: CI on a89af9d: 178 of 179 jobs green, including every platform's shell tests. The one red job (debian x64-asan) fails only on |
There was a problem hiding this comment.
LGTM — adds the missing --force long-option arm mirroring the existing -f body.
What was reviewed:
- Verified the new arm is byte-identical to the
b'f'short-option branch (setsopts.force, resetsprompt_behaviourtoNever), matching the sibling--recursive/--verbose/--dirarms. - Test covers nonexistent operand, existing operand,
-i --forceordering, combination with--recursive/-r, and the negative case (--forcstill rejected). - Checked that
--forcedoesn't collide with theflag.len() > 2guard and that no other long option was missing its arm.
Extended reasoning...
Overview
Five-line addition to Rm::parse_flag in src/runtime/shell/builtin/rm.rs: a b"--force" match arm in the long-option block that sets opts.force = true and opts.prompt_behaviour = PromptBehaviour::Never, then returns ContinueParsing. This is byte-for-byte what the b'f' short-option arm does. The Opts::force field's doc comment already listed --force as a valid spelling, and every other implemented option (--recursive, --verbose, --dir, --interactive=..., --preserve-root) already had its long form — this was simply an omission.
A new test in test/js/bun/shell/commands/rm.test.ts exercises the flag against a nonexistent operand, an existing file, -i --force (verifying the last-wins semantics that -f already has), --force --recursive and -r --force on populated directory trees, and confirms --forc is still rejected with illegal option -- -. The test uses tempDir, asserts full {stdout, stderr, exitCode} objects, and lives alongside the existing force test in the same describe block.
Security risks
None. Option parsing only; no new filesystem behaviour, no change to what force does during removal, no untrusted-input parsing beyond exact byte-string matching.
Level of scrutiny
Low. This is a mechanical gap-fill in a fixed match expression, following the exact pattern of five neighbouring arms. The change cannot affect any input that doesn't literally start with --force as a whole argument. No memory-safety, threading, or GC surface is touched.
Other factors
The test is well-constructed per repo guidelines (hermetic tempdir, exact-value assertions, negative case, variant matrix including flag ordering). PR description confirms the test fails on main and passes with the fix, and that the whole rm.test.ts file passes. No CODEOWNERS coverage on this path. No outstanding reviewer comments.
|
Updated 4:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit 7ed9946 has some failures in 🧪 To try this PR locally: bunx bun-pr 39224That installs a local version of the PR into your bun-39224 --bun |
|
Updated 6:49 PM PT - Aug 15th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 39224That installs a local version of the PR into your bun-39224 --bun |
There was a problem hiding this comment.
LGTM — a five-line match arm mirroring the existing -f body, with a thorough test.
What was reviewed
- The new
--forcearm is byte-identical to the short-fhandling (setsopts.force, resetsprompt_behaviourtoNever), sorm -i --forceandrm --force -ibehave exactly like their-fequivalents. - Confirmed every other short option in
parse_flagalready has a long spelling; this was the only gap. - Test covers nonexistent/existing operands,
-i --forcecancellation,--force --recursiveand-r --forceon populated dirs, and the negative--forccase; usestempDir, asserts full{stdout, stderr, exitCode}.
Extended reasoning...
Overview
This PR adds a single b"--force" arm to the long-option match in Rm::parse_flag (src/runtime/shell/builtin/rm.rs), setting opts.force = true and opts.prompt_behaviour = PromptBehaviour::Never — exactly what the existing b'f' short-option arm does a few lines below. It also adds one test to test/js/bun/shell/commands/rm.test.ts. The Opts::force doc comment already documented --force as an accepted spelling; the parser just never matched it.
Security risks
None. This is option parsing for a shell builtin; the new arm sets the same two fields the already-accepted -f sets, and touches no removal, path-resolution, or memory-management code.
Level of scrutiny
Low. The change is a mechanical addition of one match arm whose body is copied verbatim from the sibling -f arm in the same function. No control flow, no new state, no allocation, no error paths. The parser structure (whole-word match on ---prefixed args) is unchanged, so existing long options and the IllegalOption fall-through are unaffected.
Other factors
- The test follows repo conventions:
tempDirfrom harness,await using, exact-object assertions on{stdout, stderr, exitCode}, and covers the variant matrix called out in REVIEW.md — including flag ordering, combination with--recursive/-r, the-i-cancellation semantics, and a negative case (--forcstill rejected, file kept). - The PR description explicitly verified the test fails on main (
illegal option -- -) and passes with the fix, and that the wholerm.test.tsfile passes. - The earlier CI failure on commit 7ed9946 was addressed by a89af9d (which reordered the test to sit after the
recursivetest); the second build is in progress but the change is trivially platform-independent. - No CODEOWNERS entry covers this path.
Problem
rmbuiltin rejects--force:rm --force missingexits 1 withrm: illegal option -- -, whilerm -f missingexits 0. Same for combinations such asrm --recursive --force dir.parse_flaginsrc/runtime/shell/builtin/rm.rs(line 494 onwards) matches the long spellings of every other option it implements (--recursive,--verbose,--dir,--interactive=...,--preserve-root) but has no--forcearm, so it falls through toIllegalOption. TheOpts::forcedoc comment already lists--forceas one of its spellings. After this change every short optionrmaccepts has its long spelling accepted too.Fix
--forcearm to the long-option match that does what thefshort-option arm does: setopts.forceand resetprompt_behaviourtoNever.rmis a builtin on every platform (there is no systemrmto fall back to), and GNUrmdefines-f, --forceas one option, including the "last of-f/-iwins" rule; sharing the-fbody keepsrm -i --forcebehaving likerm -i -fandrm --force -ilikerm -f -i. No other behaviour changes; whatforcedoes during removal is untouched.lsandmvhave their own option parsers with no long-option handling at all (ls --all,mv --force); that is a different gap in different files, not a missing arm in this match.test/js/bun/shell/commands/rm.test.ts(--force is the long form of -f): nonexistent operand, existing operand,-i --force,--force --recursiveand-r --forceon populated directories, and--forcstill rejected. Fails on main with theillegal optionoutput above, passes with the fix.rm.test.tsfile passes with the debug build.rm -fwith no operands) editsRm::nextand adds tests to the same file; the two apply cleanly in either order. Since--forcesets the sameopts.forcefield that shell(rm): exit 0 silently when -f is given without operands #39225 consults, whichever of the two lands second can add arm --forcewith-no-operands case to the other's tests.Background
Rm::parse_flag: a---prefixed argument is matched as a whole word against a fixed list, anything else starting with-is a cluster of single-letter options, and the first argument that matches neither ends option parsing.Opts.forcemakes the removal tasks treat ENOENT as success;Opts.prompt_behaviourrecords-i/-I/--interactive=..., and anything other thanNeveris currently rejected with"-i" is not supported yetonce parsing finishes, which is why-f(and now--force) resets it.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/rm.test.ts