Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesThe shell path builtins now use shared cwd-relative joining. POSIX operands preserve kernel handling of ChangesShell path semantics
Merge Risk: ⚪ Minimal · up to The shell builtins now preserve intended path-resolution semantics, with descriptor-relative recursive removal and protected rm operands covered by the supplied tests. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:07 AM PT - Sep 8th, 2026
✅ @robobun, your commit 213d4ca8b7ea37d6205ebc469b027e25055bc198 passed in 🧪 To try this PR locally: bunx bun-pr 41903That installs a local version of the PR into your bun-41903 --bun |
|
Rescoped. This PR now covers Reproduced on 1.4.3 (Linux), in a temp dir with With this branch: |
The mkdir, touch and cp builtins joined each operand onto the shell cwd with a normalizing join, so `.` and `..` components were folded out of the string before any syscall saw it. `mkdir -p d/link/../nd` created `d/nd` instead of the directory next to the link target, and `touch d/missing/../t` succeeded instead of failing with ENOENT. The new shell_join_cwd prefixes a relative operand with the shell cwd and changes nothing else (Windows keeps the normalizing join, since Win32 resolves `..` textually itself). cp's refusal to copy a file onto itself compared the two joined strings, which no longer match for two spellings of one path, so it now compares device and inode after the target has its final form.
d936b02 to
b1fb0da
Compare
…le check; normalize absolute operands on Windows again
There was a problem hiding this comment.
LGTM — 213d4ca addresses both points from the last pass: shell_join_cwd now routes absolute operands through the normalizing join on Windows so the byte-equality same_file check holds, and classify() follows a symlink operand via stat (falling back to the link's own id when dangling) so cp file link-to-file and the reverse are refused; the new ontoSymlink/fromSymlink test cases cover it.
What was reviewed:
shell_join_cwd: empty-operand early return, POSIX plain-separator join vs Windows normalizingjoin_z_spill, absolute-path handling per platform.cpsame-file:(st_dev, st_ino)comparison after target-dir join, re-classifyon the joineddir/basenamepath, error message shows the user's operands (not the cwd-prefixed form).- Confirmed the dropped
rm.rs/rm.test.tschanges are no longer in the diff, so the earliersetDefaultTimeoutand./..diagnostic threads are moot for this PR.
Extended reasoning...
Overview
This PR replaces the textual resolve_path::join_z* normalization of mkdir/touch/cp shell-builtin operands with a shared shell_join_cwd helper that, on POSIX, only prefixes the shell cwd and hands the path to the kernel byte-for-byte — so link/../x resolves through the filesystem as every other program sees it. Windows keeps the normalizing join (Win32 resolves .. textually and NT calls reject dot components). cp also gains a real same-file check: classify() returns (st_dev, st_ino) (following a symlink operand to its target) and same_file() compares those on POSIX, with normalized-path byte equality on Windows. Since the previous review round, commit 213d4ca addressed both open inline comments (Windows absolute-operand normalization; symlink-aware identity), added test coverage for the symlink-as-operand cases, and the rm portion was dropped from the PR entirely in favor of #41842.
Security risks
Path resolution through symlinks is security-adjacent, but this change moves the builtins toward kernel semantics (the same view ls, cat, redirects, and coreutils have) rather than away from them, eliminating a class of "operates on a different file than the user typed" surprises. The shell runs with the invoking user's privileges and the cp builtin is behind BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS on POSIX. No new attack surface is introduced; the same-file refusal is a safety guard that now covers strictly more cases (hard links, symlinks, alternate spellings) than the base branch's string equality.
Level of scrutiny
Moderate. The change is focused (one shared helper, three call sites, one identity check), the reference behavior is well-specified (GNU/BSD cp, coreutils mkdir/touch), and the test matrix is thorough — symlink .. traversal for all three builtins, an eight-way same-file refusal matrix including the >128 KB macOS unlink-first path, and updated ENAMETOOLONG expectations for the ./-padded operands. The cross-platform #[cfg] split is the main risk area; bun run rust:check-all is warranted but the Windows branch is a small, mechanical variation on the base-branch code (same join_z_spill, now also applied to absolute operands, restoring base-branch behavior there).
Other factors
This is the sixth review pass. All prior inline threads from this reviewer are either addressed by 213d4ca (the two on cp.rs) or moot because the rm changes were dropped from the diff. No human CHANGES_REQUESTED reviews are outstanding, no CODEOWNERS cover the touched paths, and the bug hunt exited on dry_streak with no findings and no ruled-out candidates. The PR description explicitly defers Windows verification to CI (the new POSIX-only tests are test.skipIf(isWindows) with stated reasons); the Windows code path is unchanged in behavior from base except for re-normalizing absolute operands, which restores what base already did.
Problem
mkdir,touchandcpshell builtins folded.and..out of each operand,path.joinstyle, before any syscall.mkdir -p d/link/../ndcreatedd/ndwhere coreutils (andls,cat,mv, redirects in the same shell) seeother/nd, andtouch d/missing/../tcreatedd/tinstead of failing with ENOENT.mkdir.rs,touch.rsandcp.rsjoined the operand onto the shell cwd withresolve_path::join_z*, which normalizes like Node'spath.join.Fix
shell_join_cwd(interpreter.rs): a relative operand gets the shell cwd prefixed and nothing else changes. Windows keeps the normalizing join, because Win32 resolves..textually itself.cprefused to copy a file onto itself by comparing the two joined strings. Two spellings of one path no longer compare equal, so the check now compares device and inode once the target has its finaldir/basenameform, as BSD and GNU cp do.test/js/bun/shell/commands/{mkdir,touch,cp}.test.ts, new cases fail on 1.4.3 and pass here. Also thels,mv,rmandbunshellsuites.Background
mkdir,touchandcpsit on path-only APIs (node:fs mkdir and cp,utimens), so they spell the operand from the cwd string.a/link/..is the parent of the link target andmissing/..does not exist. Only a textual resolver (Node'spath, Win32) reads them asaand..cpis a builtin by default only on Windows; on POSIX the new cp tests enable it withBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1.Notes
rm:rm -rlisted a directory through the kernel path but unlinked each entry under the lexically joined parent, sorm -rf d/link/..deleted same-named files ind/. An earlier revision of this PR fixed that too (file entries unlinked relative to the listed directory's fd, plus a./..operand refusal). That part is dropped here because shell(rm): resolve every entry relative to the directory fd the walk holds #41842 reworks the wholermwalk to resolve every entry, directories included, relative to a held parent fd, which fixes the same bug and the directory-level re-resolution race, and the two conflicted throughoutrm.rs. Thermregression cases from that revision are posted on shell(rm): resolve every entry relative to the directory fd the walk holds #41842. The POSIX./..operand refusal (rm -rf .empties the cwd today) is shell(rm): refuse '.' and '..' operands instead of emptying the directory #34906's subject and is not included here either.mkdir/touchoperand longer thanPATH_MAXnow fails withENAMETOOLONGinstead of being normalized short, as coreutils does (the existing long-operand tests change accordingly);mkdir ''andtouch ''fail with ENOENT instead of acting on the cwd (shell: fail an empty operand with ENOENT instead of acting on the cwd #38002 covers empty operands more broadly, including the Windows fd-relative builtins).mkdir -pv d/missing/../xcreatesd/missingand thend/x, printing both, exactly as GNU does;mkdir d/file/../xisENOTDIR.shell_join_cwdkeeps normalizing both relative and absolute operands there. The macOS copy path unlinks the destination beforeclonefilefor files over 128 KB, so a same-file copy that slipped past the builtin's check would not be harmless there; the new cp test includes a 200 KB file copied onto./itselfand through an absolute spelling.rmpart) also passed its tests on Windows x64;cargo checkforx86_64-pc-windows-msvcandaarch64-apple-darwinpasses on this one.mkdir -p d/link/../ndcreatesother/nd;touch d/nodir/../tisENOENT;cp a ./ais refused ("are the same file");touch ''andmkdir ''fail.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/touch.test.ts, test/js/bun/shell/commands/mkdir.test.ts, test/js/bun/shell/commands/cp.test.ts