[robobun]

Problem

• The ls builtin opens each operand with O_DIRECTORY and reads ENOTDIR as "the operand is a file". The kernel also returns ENOTDIR when a path component is a file, so ls f/x prints f/x and exits 0, and ls -l f/x prints a row of ?. coreutils: ls: cannot access 'f/x': Not a directory, exit 2.
• Every other open error is fatal, so a dangling or self-referencing symlink operand (ENOENT, ELOOP) is reported as an error. GNU and BSD ls list the link by name, exit 0.
• The operand went through the -a/-A dotfile filter, so ls .hidden and ls ./f print nothing, exit 0.
• Cause for all three: ShellLsTask::run_from_thread_pool, src/runtime/shell/builtin/ls.rs:459.

Fix

• When the open fails, apply the rule coreutils uses for a command-line operand: stat it, and on ENOENT or ELOOP fall back to lstat. A directory that would not open keeps the open error (EACCES, also through a symlink). A failed stat is the error to report (f/x, nx). Anything else that exists is a non-directory and is listed by name. lstat describes it, so -l shows a symlink as a link.
• Operands bypass the dotfile filter, which only applies to directory entries.
• shell_lstatat in interpreter.rs mirrors shell_statat so Windows operands get the same path rewrite as the open above.
• Verified: test/js/bun/shell/commands/ls.test.ts (5 new or updated tests fail on the release build, plus a symlink case in the permission test). The f/x test also carries the ls -d f/x and ls f f/x cases from shell(ls): report an operand whose parent is a file instead of listing it #39725. Also bunshell.test.ts.

Supersedes #39725, which only fixes the f/x case. Its stat check sits inside the ENOTDIR arm, so it cannot list a dangling link, and it keeps the dotfile filter on operands. Its four test cases pass on this branch. The two this branch did not already have are folded in.

Background

• The ls builtin runs one ShellLsTask per operand on the thread pool. A task either lists a directory's entries or appends the operand itself to its output. A task that sets err makes the builtin print ls: <path>: <strerror> and exit 1.
• lstat reports a symlink as itself, stat reports its target. A dangling link exists for lstat and does not exist for stat.

Notes

Cells from the conformance matrix, release 1.4.0 vs this branch vs bash -c, run as a non-root user. f is a regular file, dl -> nonexist, loop -> loop, sub is a directory, restricted is a mode 000 directory, symrestricted -> restricted.

command	1.4.0	this branch	coreutils 8.32
ls f/x	0 f/x	1 ls: f/x: Not a directory	2 cannot access 'f/x': Not a directory
ls -l f/x	0 ?????????? ? ? ? ? ? f/x	1 same error	2 same error
ls f/x/y/z	0 f/x/y/z	1 Not a directory	2 Not a directory
ls dl	1 No such file or directory	0 dl	0 dl
ls -l dl	1	0 lrwxrwxrwx ... dl	0 lrwxrwxrwx ... dl -> nonexist
ls loop	1 Too many levels of symbolic links	0 loop	0 loop
ls loop/x	1 ELOOP	1 ELOOP	2 ELOOP
ls .hidden	0 (empty)	0 .hidden	0 .hidden
ls ./f	0 (empty)	0 ./f	0 ./f
ls sub f/x	0 sub:\na\nf/x	1 sub:\na + error	2 same
ls restricted, ls symrestricted, ls restricted/x	1 Permission denied	1 Permission denied	2 Permission denied
ls f, ls symf, ls symdir, ls nx, ls -d f	unchanged	unchanged	match

The first version of this branch decided with lstat alone. Review caught that a symlink to an unreadable directory then passed as "exists, not a directory" and swallowed the EACCES. The stat first, lstat on ENOENT/ELOOP order is what coreutils does (gobble_file, DEREF_COMMAND_LINE_SYMLINK_TO_DIR), and the permission test now covers the symlink.

Not changed here:

• The exit code stays 1. The builtin uses 1 for every error, coreutils uses 2 for a failed operand and 1 for a failed entry.
• -l does not print -> target for symlinks.
• ls -l symdir and ls -d symdir still follow a command-line symlink to a directory. coreutils does not with -l or -d.

The two existing broken symlink tests expected the error. They came in with #20531 (a memory refactor) and pinned the behavior of the time. They now expect the coreutils output.

The chmod based permission denied tests fail in any environment that runs as root, with or without this change (#39232). They pass here as nobody.

cargo check -p bun_runtime --target x86_64-pc-windows-msvc passes.

---

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/ls.test.ts

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e318f282-eafe-473b-870b-1f85c36fcca7

📥 Commits

Reviewing files that changed from the base of the PR and between abe2ad4 and 6517b29.

📒 Files selected for processing (3)

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

Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review.

---

Walkthrough

Changes

ls now handles non-directory operands through shell_statat and shell_lstatat. It lists broken symlinks and preserves symlink metadata, while reporting invalid traversals and explicit dot-prefixed operands correctly. Tests cover these cases across short, long, and directory-only output.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly and concisely describes the main ls operand-error handling change.
Description check	✅ Passed	The description explains the problem, fix, scope, and verification results, although it uses different headings than the template.

---

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

[robobun]

^{Updated 11:59 AM PT - Aug 22nd, 2026}

✅ @robobun, your commit 6517b29370e6b23f8af7fd6f91642ea3c98371df passed in Build #103666! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 40091

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

bun-40091 --bun

[claude]

LGTM — the earlier symlink-to-unreadable-directory regression is fixed and now covered by ls link-to-restricted in the permission test.

What was reviewed:

• Traced list_non_directory_operand through each row of the conformance matrix (f/x, dl, loop, loop/x, .hidden, restricted, symrestricted, plain file) — the stat→lstat order routes each to the coreutils outcome.
• Confirmed the Ok(_) fall-through to lstat is needed so -l on a symlink-to-file shows the link, and that add_entry_short/add_entry_long_from_stat are pure extractions of the previous inline code.
• shell_lstatat mirrors shell_statat byte-for-byte with lstat/lstatat; bun_sys::lstat exists for the Windows arm.
• The two flipped broken symlink tests were pinning accidental behavior from #20531; new expectations match GNU/BSD ls.

Extended reasoning...

Overview

The PR fixes three bugs in the shell ls builtin's handling of command-line operands, all rooted in ShellLsTask::run_from_thread_pool interpreting the open(O_DIRECTORY) errno as the sole classifier: (1) ENOTDIR from a non-directory path component was treated the same as ENOTDIR from a file operand, so ls f/x succeeded; (2) any non-ENOTDIR error was fatal, so dangling/looping symlinks errored instead of being listed; (3) file operands went through the -a/-A filter, so ls .hidden and ls ./f printed nothing. The fix replaces the errno switch with the coreutils operand rule (stat, then lstat on ENOENT/ELOOP), factored into list_non_directory_operand. Supporting refactors extract add_entry_short and add_entry_long_from_stat so the operand path can bypass should_skip_entry and reuse an already-obtained Stat. shell_lstatat is added alongside shell_statat in interpreter.rs for Windows path rewriting parity.

Security risks

None. The change only affects how ls classifies and reports its own operands; no new inputs are trusted, no allocations are sized from untrusted data, and the added syscalls (stat/lstat) are read-only metadata queries on paths the user already passed. The one direction that could have weakened error reporting — a symlink to an unreadable directory silently succeeding — was caught in the first review round and is now fixed (stat sees S_IFDIR through the link, so the open's EACCES is reported) and pinned by the link-to-restricted test case.

Level of scrutiny

Medium. This is a self-contained behavioral fix in one shell builtin with no memory-safety surface change (no new ownership, the ZBox::from_bytes clone matches the pre-PR pattern, and the &Stat refactor is a trivial by-ref conversion). The user-visible behavior change — dangling symlinks now succeed, f/x now fails — is a correctness fix bringing the builtin in line with GNU and BSD ls, backed by a detailed conformance table in the PR body. The two existing tests whose expectations flip are documented as having pinned accidental behavior from a memory refactor (#20531), which is the right call under REVIEW.md's "never mutate an existing test" rule since the original assertions encoded a bug rather than a contract.

Other factors

An earlier revision decided with lstat alone; my prior review flagged that this swallowed EACCES on a symlink to a mode-000 directory. The author fixed it in e6b6dcf by adding the stat-first step and a regression case in the permission denied directory test. All comment-cop feedback (long comments) has been trimmed. The bug hunting system found no issues on the current revision. Test coverage is thorough across the variant matrix (short and -l listings, f/x at multiple depths, mixed operands, loop vs loop/x, .hidden and ./f), and the Windows arm was compile-checked. The remaining coreutils divergences (exit code 1 vs 2, no -> target in -l, -l/-d still following symlink-to-dir) are explicitly scoped out and unchanged from before this PR.

[robobun]

6517b29 does two things:

• cargo clippy denied needless_pass_by_value on the open_err argument of list_non_directory_operand (ls.rs:514). It now takes &bun_sys::Error.
• shell(ls): report an operand whose parent is a file instead of listing it #39725 fixed the f/x case of the same bug and is now closed in favor of this PR. Its four test cases pass on this branch. The two this branch did not already have, ls -d f/x and ls f f/x, are added to the f/x test.

[claude]

LGTM — the earlier finding (symlink to an unreadable directory swallowing EACCES) is fixed: list_non_directory_operand now stats first and only falls back to lstat on ENOENT/ELOOP, and the permission denied directory test covers link-to-restricted.

What was reviewed:

• Traced list_non_directory_operand for each row of the conformance matrix (f/x, dangling link, loop, loop/x, .hidden, ./f, symrestricted, plain file, nonexistent) — each reaches the intended arm.
• add_entry_short / add_entry_long_from_stat extractions are behavior-preserving; operand path now bypasses should_skip_entry while directory entries still go through it.
• shell_lstatat mirrors shell_statat exactly (same Windows path rewrite); the non-isPosix-gated ENOTDIR test only asserts the message prefix on Windows.
• The two flipped broken symlink tests now expect coreutils behavior; they were pinning the pre-fix error.

Extended reasoning...

Overview

The PR reworks how the shell ls builtin handles a command-line operand when openat(O_DIRECTORY) fails. Previously the code branched on the open errno alone: ENOTDIR → print the operand as a file (wrong for f/x), anything else → error (wrong for dangling/looping symlinks). It also routed the operand through add_entry, which applies the -a/-A dotfile filter meant for directory entries, so ls .hidden and ls ./f printed nothing.

The new list_non_directory_operand implements the coreutils rule: stat the operand; if the target is a directory, keep the open error (covers EACCES through a symlink); if stat itself fails with anything other than ENOENT/ELOOP, report that (covers f/x → ENOTDIR); otherwise lstat and list the operand by name (covers dangling links, self-loops, regular files, symlinks-to-files). The listing goes straight to add_entry_short/add_entry_long_from_stat, so the dotfile filter no longer applies to operands. shell_lstatat is added alongside shell_statat so Windows gets the same POSIX-path rewrite as the open call above it.

The rest of the diff is mechanical extraction of add_entry_short and add_entry_long_from_stat from their inlined bodies, plus tests.

Prior review round

I flagged one regression on the first revision: a symlink to a mode-000 directory would lstat as S_IFLNK, pass !ISDIR, and be listed with exit 0, discarding the EACCES. The author fixed this exactly as suggested — stat first, lstat only on ENOENT/ELOOP — and added ls link-to-restricted to the permission test. I re-traced the fixed function against every case in the PR's conformance table plus the regression case and each hits the intended arm. All comment-cop threads about long comments were also addressed; every added comment is now one line.

Security risks

None. This is output-formatting logic in a shell builtin; no untrusted parsing, no privilege boundaries, no allocation driven by external size fields. The change moves error handling toward failing correctly (f/x now errors instead of silently succeeding) rather than away from it.

Level of scrutiny

Medium. The stat/lstat decision tree is subtle — it took one round to get right — but the function is small, self-contained, matches the documented coreutils rule (gobble_file, DEREF_COMMAND_LINE_SYMLINK_TO_DIR), and every branch in the matrix has a test. The two existing tests whose expectations flipped were pinning the incorrect pre-fix behavior (introduced in a memory refactor, not as a deliberate spec), and the PR body calls this out explicitly.

Other factors

Test coverage is thorough: f/x, f/x/y/z, -l f/x, -d f/x, mixed good/bad operands, dangling link (short and -l), self-loop and loop/x, .hidden, -d .hidden, ./f, -l ./f, and the symlink-to-restricted regression. The Windows-visible test relaxes only the strerror text, not the exit code or path echo. The refactored helpers are byte-for-byte moves; stat_mtime(&stat) → stat_mtime(stat) is the only signature-driven edit. Known gaps (exit code 1 vs 2, no -> target in -l, -l/-d still following symlink-to-dir) are called out in the PR body as out of scope and are pre-existing.