shell(rm): refuse '.' and '..' operands instead of emptying the directory

[robobun]

Reproduction

import { $ } from "bun";
import { mkdtempSync, readdirSync } from "node:fs";
import { tmpdir } from "node:os"; import { join } from "node:path";
$.nothrow();
const box = mkdtempSync(join(tmpdir(), "rmdot-"));
await $`mkdir -p k/s; echo A > k/a; echo C > k/s/c`.cwd(box).quiet();
const r = await $`${{ raw: "cd k; rm -rf ." }}`.cwd(box).quiet();
console.log(r.exitCode, r.stderr.toString().trim(), readdirSync(join(box, "k")));
// Bun : 1 "rm: .: Invalid argument" []                               <- k was EMPTIED, then rmdir(".") failed
// bash: 1 "rm: refusing to remove '.' or '..' directory: skipping '.'" [ "a", "s" ]

Same for rm -rf ./, rm -rf d/., rm -rf d/.., and cd k/s; rm -rf .. (which deleted the parent's contents).

Cause

POSIX requires rm to write a diagnostic and do nothing more with an operand whose last path component is . or ... The builtin had no such check, so the operand was passed to the recursive ShellRmTask which opened it as a directory, deleted every entry, and only then hit EINVAL from unlinkat(cwd, ".", AT_REMOVEDIR).

The preserve-root guard ("<path>" may not be removed) also resolved relative operands against the host process cwd (bun_sys::getcwd_z) rather than the shell instance's cwd. With a process launched from /, $\rm -rf sub`.cwd(deepSandbox)` was spuriously refused; launched from elsewhere, the same guard could be side-stepped.

Fix

The per-operand validation now runs once at the start of the Exec state:

• Operands whose basename is . or .. get the GNU-style refusing to remove '.' or '..' directory: skipping '<op>' diagnostic and are skipped.
• The preserve-root check resolves against Builtin::shell().cwd() (the shell instance's cwd, same source cp/mkdir/touch already use).
• Refused operands are skipped while the remaining operands are still processed, matching GNU coreutils; the exit code is 1 if any operand was refused.

Dotfiles and ... are unaffected, and rm -rf / still prints "/" may not be removed.

Verification

bun bd test test/js/bun/shell/commands/rm.test.ts
  17 pass  0 fail

The new rm refuses '.' and '..' operands cases and rm preserve-root guard resolves against the shell cwd fail on the released binary and pass with this change.

---

no test proof · iteration 1 · 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

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 21 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: 8c2af917-5cda-4dbe-a69f-7a319623d18f

📥 Commits

Reviewing files that changed from the base of the PR and between 4902e1c and a9438ff.

📒 Files selected for processing (1)

• test/js/bun/shell/commands/rm.test.ts

Walkthrough

Changes

The rm builtin now tracks up-front refusals, filters unsafe operands during execution, centralizes exit-code calculation, and adds tests for refusal behavior and shell working-directory resolution.

rm refusal handling

Layer / File(s)	Summary
Execution state and dispatch src/runtime/shell/builtin/rm.rs	ExecState records refusals and computes exit codes from refusals or worker errors; the state machine removes the prior write-error wait path.
Operand refusal and task scheduling src/runtime/shell/builtin/rm.rs	Execution resolves the shell cwd, refuses unsafe operands, emits diagnostics, schedules accepted operands, and completes when no tasks remain.
Completion status and regression coverage src/runtime/shell/builtin/rm.rs, test/js/bun/shell/commands/rm.test.ts	Completion paths use centralized exit-code computation, with tests covering refused operands, mixed deletions, platform-specific cases, and cwd resolution.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: refusing '.' and '..' operands in shell rm.
Description check	✅ Passed	The description covers the change and verification, though it uses custom headings instead of the template's exact sections.
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.

---

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

[robobun]

^{Updated 7:05 AM PT - Jul 29th, 2026}

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

---

🧪   To try this PR locally:

bunx bun-pr 34906

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

bun-34906 --bun

[claude]

No new issues found; my earlier inline notes are all resolved (thanks for splitting the ... test out for Windows). Deferring to a human because this relocates the preserve-root guard into the Exec state and rewires the completion accounting (had_refusal, the extra output_count bump for the refusal write, the total_tasks == 0 early-out) around concurrently-scheduled ShellRmTasks — I traced the needs_io vs write_no_io paths and they look consistent, but this is destructive-path state-machine surgery that's worth a maintainer's eyes.

What was reviewed:

• ./.. basename refusal against the operand matrix in the tests, plus .../dotfiles negative cases
• preserve-root now resolving via Builtin::shell().cwd() instead of bun_sys::getcwd_z
• removal of RmState::WaitingWriteErr/write_failing_error — no remaining references, each builtin has its own copy
• completion paths when all operands are refused, with and without stderr.needs_io()

Extended reasoning...

Overview

Two-file change: src/runtime/shell/builtin/rm.rs (~200 lines net) and test/js/bun/shell/commands/rm.test.ts (+~100 lines). The runtime change adds the POSIX-mandated ./.. operand refusal, moves the preserve-root guard from ParseOpts into the start of Exec, switches its resolution base from the process cwd to the shell instance's cwd, and reworks how refused operands feed into the exit-code / output-drain accounting (new had_refusal on ExecState, new exit_code() helper used at all three completion sites, an output_count bump for the batched refusal message when stderr needs async I/O, and a total_tasks == 0 short-circuit to Done). The now-unreachable WaitingWriteErr state and write_failing_error helper are deleted.

Security risks

The change is on a destructive filesystem operation, but strictly narrows what rm will act on (refuses ./.. up front) and fixes a case where the guard could be side-stepped by process-vs-shell cwd mismatch. No new inputs are trusted; refused operands are skipped rather than processed. I don't see a way this widens the delete surface.

Level of scrutiny

High. rm -rf is as destructive as it gets, and the diff touches the async completion protocol between worker-pool ShellRmTasks and the main-thread on_io_writer_chunk/on_shell_rm_task_done callbacks. I walked the four cases (refusal + tasks × needs_io/no_io, refusal + no tasks × needs_io/no_io) and the output_count/output_done/tasks_done/total_tasks invariants hold in each — the comment about task completions bouncing to the main thread is what makes the post-schedule output_count.fetch_add non-racy. It reads correct, but a maintainer who owns the shell I/O writer contract should confirm.

Other factors

Test coverage is solid: a test.each over seven operand spellings asserting exact stderr/exit/remaining-files, a mixed-operand test proving non-refused operands still run, negative cases for dotfiles and ..., the / guard, and a spawned-subprocess test for the shell-cwd fix (process launched from /). All three of my earlier inline notes are resolved — two were declined with reasonable justification (cross-platform \\ handling is a shell-wide convention; --no-preserve-root is pre-existing and out of scope), and the Windows ... fixture was split out with skipIf(isWindows) in 165c878.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/shell/commands/rm.test.ts`:
- Around line 291-302: The rm command fixture must assert that the spawned child
launches with the configured root working directory. Update the fixture JSON
produced by the child to include process.cwd(), then extend the expected object
in the test around Bun.spawn to require that value to be "/"; preserve the
existing stderr, exitCode, and subExists assertions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8d83902c-3f97-49d9-8633-2052bfa0db3c

📥 Commits

Reviewing files that changed from the base of the PR and between 59242d6 and 4902e1c.

📒 Files selected for processing (2)

• src/runtime/shell/builtin/rm.rs
• test/js/bun/shell/commands/rm.test.ts

[claude]

I didn't find any issues, but since this rewrites the rm builtin's per-operand validation and folds it into the Exec state (changing how refusal diagnostics interact with the output_count/output_done accounting and concurrent ShellRmTask scheduling), it's worth a maintainer glance at the state-machine flow.

What was reviewed:

• The ./.. basename refusal and shell-cwd resolution — matches GNU/POSIX; Builtin::shell().cwd() is the same source cp/mkdir/touch use.
• The needs_io() refused-msg enqueue path when total_tasks == 0 — on_io_writer_chunk correctly reaches Done via tasks_done (0) >= total_tasks (0) && output_drained().
• output_count.fetch_add for the refusal write happens after tasks are scheduled, but task completions post back to the main thread so no ordering hazard.
• Removed WaitingWriteErr/write_failing_error — dead after the guard moved out of ParseOpts; remaining wait_write_err uses still route through the catch-all arm of on_io_writer_chunk.

Extended reasoning...

Overview

Two files: src/runtime/shell/builtin/rm.rs and test/js/bun/shell/commands/rm.test.ts. The runtime change moves the per-operand preserve-root check from the ParseOpts state into the Exec state, adds a POSIX-mandated refusal for operands whose basename is . or .., switches the guard's cwd source from bun_sys::getcwd_z (host process) to Builtin::shell().cwd() (shell instance), and lets refused operands be skipped while remaining operands still run. Supporting refactors: a new had_refusal flag and exit_code() helper on ExecState, and removal of the now-dead RmState::WaitingWriteErr variant and write_failing_error helper.

Security risks

The change is fail-safe in direction — it adds refusals, never removes them. The one prior nit (backslash-as-separator on POSIX via resolve_path::basename) fails toward a spurious refusal, not a wrong deletion, and the author declined it for consistency with other shell builtins. The pre-existing --no-preserve-root no-op was already flagged and deferred as out of scope.

Level of scrutiny

High. rm -rf is a destructive operation and the previous behavior was silent data loss (rm -rf . emptied the cwd). The fix itself is straightforward, but it's woven into a hand-rolled state machine that coordinates worker-thread ShellRmTasks with main-thread IO-writer callbacks via atomic output_count/output_done counters. I traced the four combinations (total_tasks == 0 × needs_io() Some/None) and they all reach Done with the right exit code, but this is exactly the kind of accounting a maintainer who owns the shell builtin dispatch should confirm.

Other factors

All prior review threads (mine and CodeRabbit's) are resolved: the ... test is now Windows-gated, long comments were trimmed per comment-cop, and the preserve-root subprocess test now asserts process.cwd() === "/". Test coverage is thorough — seven ./.. operand spellings, mixed refused+accepted operands, dotfiles and ... as negative cases, / still refused, and a spawned-from-/ subprocess for the cwd fix. The bug hunter found nothing this run.

[robobun]

CI status for a9438ff (build #85018): test/js/bun/shell/commands/rm.test.ts passed on every lane. All 15 failing tests are classified [flaky] by ci:errors (also failing on main), none in this diff's area:

node-dns.test.js, mock-module.test.ts, napi.test.ts, 14029.test.ts,
08965.test.ts, spawn-streaming-stdout.test.ts, jsonc.test.ts,
20875.test.ts, 2_function_arguments/do.test.ts,
node-http-pinned-write.test.ts, 20144.test.ts, socket-retention.test.ts,
shell/leak.test.ts (#11816 external; passed alone),
streams-leak.test.ts, test-fastutf8stream-reopen.js

No [new] failures. The diff is green; ready for review.

[robobun]

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.