shell: don't abort when a glob's directory prefix doesn't exist#31367
Conversation
A glob whose literal directory prefix is missing (e.g. `echo /nonexistent/*`) makes the walker fail with ENOENT. Expansion::on_glob_walk_done raised a JS exception from the glob task callback and kept running the interpreter, so the exception stayed pending with no JS frame to observe it and the process aborted; .nothrow()/try-catch never got a chance. Treat a failed walk like an empty match set instead: command position fails with "no matches found" (exit 1, catchable ShellError), assignment position expands to the literal pattern — the same behavior relative patterns already have when their directory is missing. Also skip the cd-EACCES test when running as root, since root bypasses the permission check it relies on.
|
Updated 10:07 PM PT - May 24th, 2026
❌ @robobun, your commit 2677fda has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31367That installs a local version of the PR into your bun-31367 --bun |
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 6 minutes and 22 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR changes glob expansion error handling in the shell runtime to log failures and continue to standard processing logic instead of immediately throwing. Regression tests validate the new behavior across execution contexts and update an existing test's platform gating. ChangesGlob expansion error handling without throwing
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…tches Keep the ENOENT/ENOTDIR fallthrough (missing directory prefix behaves like a no-match glob), but route other walk failures (EACCES, EMFILE, internal errors) through ExpansionState::Err — the same path walker-init failures use — so the real error reaches stderr with exit code 1.
Assigns never prints expansion errors, so routing a non-ENOENT walk failure through ExpansionState::Err in assignment position produced exit 1 with empty stderr and left the variable unset. Fold the walk-error handling into the no-match branch instead: assignments always fall back to the literal pattern (scalar assignments don't glob), command position keeps "no matches found" for ENOENT/ENOTDIR and the real error for anything else.
There was a problem hiding this comment.
All my earlier feedback has been addressed and I have nothing further to flag — deferring to a maintainer mainly to sign off on the deliberate fail-loudly-on-partial-results choice (vs. bash/zsh's skip-and-continue), since that's a user-visible semantics call rather than a correctness question.
Extended reasoning...
Overview
This PR fixes a process abort in Bun.$ when a glob's literal directory prefix doesn't exist (e.g. echo /nonexistent/*). The change is confined to Expansion::on_glob_walk_done in src/runtime/shell/states/Expansion.rs (~30 lines net): instead of calling interp.throw() from the glob task callback (which left a pending JS exception with no observing frame and aborted), walk errors are now routed through the existing ExpansionState::Err / no-match machinery. ENOENT/ENOTDIR fall through to "no matches found"; other syscall errors and ShellGlobErr::Unknown set ExpansionState::Err so the real error reaches stderr in command position; assignment position always falls back to the literal pattern. Two new tests in bunshell.test.ts cover the crash repro (subprocess), the throwing/.nothrow()/assignment paths, and a root-gated EACCES case in both positions; an existing EACCES cd test also gained !isRoot gating.
Security risks
None. This is error-path handling for filesystem glob walks; no auth, crypto, parsing of untrusted input, or privilege boundaries are touched. The change strictly narrows behavior from "abort the process" to "exit 1 with a diagnostic", and the assignment-position literal fallback matches bash/zsh.
Level of scrutiny
Moderate. The shell interpreter state machine is intricate (NodeId trampoline, multiple parent kinds for Expansion), and getting the error-propagation contract right took three revisions here — the second of which fixed a real silent-exit-1 bug in assignment position. The final shape is sound and well-tested, but it encodes a design decision: when the walker yields some matches and then errors (e.g. one unreadable subdir among readable siblings), the partial results are discarded and the command fails, whereas bash/zsh silently skip the unreadable subdir and return the partial set. The author's rationale (deterministic, avoids readdir-order-dependent results, real parity belongs in GlobWalker) is reasonable, and I flagged it as non-blocking — but it's the kind of user-visible semantics call a maintainer should bless rather than a bot.
Other factors
- Three prior review rounds from me; the one blocking finding (Assigns dropping the boxed error) was fixed in 720085a, and the two remaining non-blocking observations (partial-results discard; pre-existing
CondExprTODO) were acknowledged with sound rationale and resolved. - The two commits since my last review (
6cdab3b5ci retrigger,2677fdaeremove comments) are cosmetic. - Test coverage is good: subprocess-isolated crash repro plus EACCES coverage in both command and assignment position.
- No CODEOWNERS entry for
src/runtime/shell/. - Strict improvement over the pre-PR behavior (process abort) on every path.
|
CI status summary for reviewers: the changed code is green everywhere — The red builds were each caused by unrelated, known-flaky areas:
None of these touch the shell or glob expansion. One retrigger was already spent; leaving further retries/merge to a maintainer. |
* oven/main (20 new commits): webcore: free Blob's owned content type on drop (oven-sh#31358) Support cross-compiling macOS binaries from Linux (oven-sh#31303) test: forward keep-alive requests in proxy.test.ts's mock proxy (oven-sh#31352) Port Bun.stringWidth to C++ with explicit SIMD (oven-sh#31351) Fix quadratic hang reporting duplicate-binding parse errors in the transpiler (oven-sh#31341) shell: don't abort when a glob's directory prefix doesn't exist (oven-sh#31367) Error instead of crashing on deeply nested statements in the transpiler (oven-sh#31333) Fix JSX transform panic when a bare `key` prop precedes `key` with a value (oven-sh#31350) Cap ANSI markdown indentation so deeply nested lists render in linear time (oven-sh#31366) css: bound selector-list expansion when compiling nesting for older targets (oven-sh#31277) node:http2: reassemble HEADERS+CONTINUATION before HPACK decoding (oven-sh#31323) Fix `await using` expression printing `using` as `await` (oven-sh#31324) Parenthesize `async` when it starts a for-of loop initializer (oven-sh#31326) Print Infinity and negative numeric property keys as computed properties (oven-sh#31328) css: keep required grouping parens in @container conditions when minifying (oven-sh#31330) Fix panic on anonymous export default class with an auto-accessor field (oven-sh#31331) node:http2: send GOAWAY frames on stream 0 (oven-sh#31353) parser: fix Scope mismatch while visiting panic from decorators on dropped class members (oven-sh#31340) webcrypto: reject oversized BufferSource inputs instead of aborting (oven-sh#31356) Error instead of crashing on deeply nested TypeScript types in the transpiler (oven-sh#31361) Resolved conflicts: - scripts/build.ts: kept both OHOS and macOS-cross argv entries - scripts/build/config.ts: kept both OHOS and macOS-cross config fields - scripts/build/deps/webkit.ts: kept OHOS fno-pic exclusion, adopted upstream -flto=thin
2 participants
What does this PR do?
Fixes a process abort (SIGTRAP / exit 133 in release,
assertNoExceptionassertion in debug) when aBun.$glob pattern points into a directory that doesn't exist:.nothrow()/try-catchcannot prevent it.Cause
For
echo /nonexistent/*the pattern's literal prefix (/nonexistent) becomes the glob walk root, and opening it fails with ENOENT.Expansion::on_glob_walk_donehandled any walk error by callinginterp.throw(...)— raising a JS exception from the glob task callback — and then kept running the interpreter with the expansion markedDone. The exception stayed pending on the VM with no JS frame above the task to observe it, so the next exception check aborted the process. (It also silently dropped the argument and let the command "succeed" with exit 0.)Relative patterns never hit this path:
echo ./nonexistent/*already reportsbun: no matches found: ./nonexistent/*with exit code 1.Fix
Stop throwing from the task callback, and handle walk errors through the shell's normal expansion-error machinery:
bun: no matches found: <pattern>on stderr, exit code 1, catchableShellError,.nothrow()respected. Matches what relative patterns already do and zsh.ExpansionState::Err, the same path walker-init failures use, so the real error reaches stderr with exit code 1 (e.g.bun: Permission denied: /root/) instead of masquerading as "no matches found".FOO=/nonexistent/*,FOO=/unreadable/*): every kind of walk failure falls back to the literal pattern, matching the existing no-match behavior and bash/zsh (scalar assignments don't glob). Assigns never prints expansion errors, so erroring there would mean exit 1 with empty stderr.(The last two refinements came out of review on earlier revisions of this PR.)
How did you verify your code works?
bun -e 'await Bun.$echo /nonexistent-path-xyz/*.nothrow().text(); console.log("survived")'aborts (exit 133/134). After: printssurvived, the command fails with exit 1 andbun: no matches found: /nonexistent-path-xyz/*.test/js/bun/shell/bunshell.test.ts(glob on a nonexistent absolute directory does not crash the process) runs the repro in a subprocess and covers.nothrow(), the default-throws ShellError path, and assignment position. It fails (the child aborts) without the fix and passes with it.glob over an unreadable directory reports the real errorcovers the EACCES path in command and assignment position (skipped when running as root); the behavior was also verified manually by running the debug build as an unprivileged user against a mode-000 directory.test/js/bun/shell/bunshell.test.tspasses with a debug (ASAN) build.