Repository navigation
Conversation
…..]' bun <(echo '...') and bun /path/to/fifo opened the fd, readlink'd it via get_fd_path (which yields 'pipe:[inode]' on Linux and fails on macOS for a pipe), then tried to boot with that non-path. When fstat on the open fd says the entry point is not a regular file, read it into the eval_source right there and boot with the path as given, so process.argv[1], __filename and import.meta reflect what the user typed. The eval_source matching in the resolver/loader is widened to also accept an exact match on the stored source path, not only the /[eval] and /[stdin] suffixes, so the virtual source is served for whatever path the entry point was read from. Fixes #7102
WalkthroughChangesThe CLI now reads FIFO and process-substitution scripts directly from open file descriptors, boots them as synthetic stdin scripts, and preserves the original entry-point path in FIFO entry-point execution
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:23 PM PT - Jul 24th, 2026
✅ @robobun, your commit 76b92db1466dad47e32d23e7f6693b78a80759ba passed in 🧪 To try this PR locally: bunx bun-pr 35463That installs a local version of the PR into your bun-35463 --bun |
/dev/zero and other char devices would read forever; keep them on the existing error path. Process substitution and named FIFOs are the only shapes that need this.
The exact-match eval_source extension broke 'bun run -e foo file.js', where -e foo is only stored as execArgv and the file is the entry: the loader started serving the -e string in place of the file's source. Revert the resolver/loader/Bun.main changes and instead read the FIFO into eval.script and boot with cwd/[stdin] exactly like 'bun run -'. process.argv[1] still carries the path the user typed because it is prepended to passthrough. Tests move to test/cli/run/run_command.test.ts since this was never a regression.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/jsc_hooks.rs:3946— The widened|| specifier == eval_source.path.textmatch regressesbun run -e <script> -b <file>:boot()at run_command.rs:977-988 unconditionally installseval_sourcewithpath.text = <file's absolute path>andcontents = <-e text>whenever-eis set, so the entry file's specifier now matches and the loader serves the-estring instead of reading the file from disk. This is what's breakingtest/js/node/process/process.test.json all 9 CI platforms (including Windows, where the FIFO branch is unreachable) — its fixture at line 1275 runsrun -e bruh -b <print-process-execArgv.js> ...and now executesbruhinstead of the file. Fix: don't installeval_sourceinboot()when the entry is a real on-disk file (only for[eval]/[stdin]/FIFO), or gate the new exact-path match on a flag set only by the FIFO branch.Extended reasoning...
What the bug is
The PR widens the eval-source match in three places —
get_loader_and_virtual_sourceinsrc/runtime/jsc_hooks.rs:3946andsrc/bundler/options.rs, and_resolveinsrc/jsc/VirtualMachine.rsandsrc/runtime/jsc_hooks.rs— from only the/[eval]//[stdin]suffixes to also accept an exact match oneval_source.path.text. This breaksbun run -e <script> -b <file>, which previously ran<file>and only recorded-e <script>inprocess.execArgvfor Node compatibility.The code path that triggers it
- Argument parsing (
src/runtime/cli/Arguments.rs):-e bruhsetsctx.runtime_options.eval.script = "bruh", regardless of whether a file positional is also given. - Command dispatch (
src/runtime/cli/mod.rs): the-e→exec_evaldiversion only fires forAutoCommand; forbun run -e bruh -b <file>it proceeds to boot the file. - Boot (
src/runtime/cli/run_command.rs:972-988):entryis set to the file's absolute path and passed tovm.set_main(entry). Then, becausectx.runtime_options.eval.scriptis non-empty, it unconditionally installs:sovm.module_loader.eval_source = Some(Box::new(Source::init_path_string(entry, script)));
eval_source.path.text == "/abs/path/to/file.js"andeval_source.contents == "bruh". - Module load (
src/runtime/jsc_hooks.rs:3946, and the parallel site inoptions.rs):The entry file's specifier now equalsif is_eval || is_stdin || specifier == eval_source.path.text { virtual_source = Some(eval_source); loader = Some(Loader::Tsx); }
eval_source.path.text, so the loader serves"bruh"as the module source instead of readingfile.jsfrom disk.
Why existing code didn't prevent it
Before this PR, step 3 already ran and installed the same
eval_source, but it was inert: the loader/resolver only matched specifiers ending in/[eval]or/[stdin], so/abs/path/to/file.jsfell through to the normal filesystem read. The PR description says "-eandbun run -are unchanged", which is true for those invocations, but it misses thateval.scriptis also populated (andeval_sourceinstalled with the file's path) when-eis combined with a file argument underbun run.Impact
The user's file is silently replaced by the
-eargument. This is exactly the CI failure robobun reports:test/js/node/process/process.test.jsfails on all 9 platforms — Linux x64/aarch64/ASAN, Alpine x64/aarch64, Ubuntu x64/aarch64, and Windows x64/aarch64. The Windows failures are dispositive: the newISFIFObranch inmaybe_open_with_bun_jsis unreachable there, so the regression must be in the widened match, not the FIFO code.Step-by-step proof
Take the failing fixture at
test/js/node/process/process.test.js:1275:bun run -e bruh -b /path/to/print-process-execArgv.js foo -a -b -cExpected: the
.jsfile runs and prints{"execArgv":["-e","bruh","-b"],"argv":["foo","-a","-b","-c"]}.- Arguments.rs sets
eval.script = "bruh". RunCommand(notAutoCommand) → does not divert toexec_eval; boots/path/to/print-process-execArgv.js.- run_command.rs:972-975:
entry = "/path/to/print-process-execArgv.js",vm.set_main(entry). - run_command.rs:977-988:
eval.scriptis non-empty →eval_source = Source { path.text: "/path/to/print-process-execArgv.js", contents: "bruh" }. - Module loader resolves the entry specifier
"/path/to/print-process-execArgv.js". At jsc_hooks.rs:3946,is_eval = false,is_stdin = false, butspecifier == eval_source.path.text→ true.virtual_source = eval_source, so the module source is"bruh". - Bun evaluates
bruhas TSX →ReferenceError: Can't find variable: bruh(or similar), no JSON on stdout, test fails.
On main (before this PR), step 5's condition was
is_eval || is_stdin→ false, the loader read the file from disk, and the test passed.How to fix
Two options:
(a) Don't install
eval_sourceat run_command.rs:977-988 when the entry is a real on-disk file. That block should only run when the entry is one of the synthetic paths ([eval],[stdin]) or the new FIFO path — e.g. gate it onctx.runtime_options.eval.eval_and_print || <entry ends with [eval]/[stdin]> || <came from the FIFO branch>, or have the FIFO branch set a distinct flag thatboot()checks. This is the cleaner fix and also stops leaking the confusingeval_sourcein this mode.(b) Keep run_command.rs as-is but gate the new
specifier == eval_source.path.textclause (in all four sites: jsc_hooks.rs ×2, VirtualMachine.rs, options.rs) on a new bool set only by the FIFO branch, so the exact-path match only activates for pipe entry points.Either way, re-run
bun bd test test/js/node/process/process.test.jsto confirm theprocess.execArgvfixture passes again alongside the new07102.test.ts. - Argument parsing (
|
Addressed the review findings in 81d6190 / 6732699:
The net diff is now a single function in |
There was a problem hiding this comment.
Beyond the inline nits, I checked fd ownership in the new ISFIFO branch — bun_sys::File::from_fd(fd) takes ownership and closes on Drop (file.rs:17-22), so the fd is released on both the read-success and read-error paths even though there's no explicit bun_sys::close like the neighboring ISDIR/get_fd_path exits have. The regular-file path (get_fd_path → close) is unchanged.
Extended reasoning...
The ISFIFO branch is the only place in maybe_open_with_bun_js that doesn't call bun_sys::close(fd) explicitly — it instead constructs a temporary bun_sys::File::from_fd(fd) whose Drop impl closes the handle when the .read_to_end_into(...) expression completes. Verified against src/sys/file.rs:17-22 that Drop for File closes any non-stdio, non-INVALID fd, so no leak on either path. Not approving: this is a user-visible CLI entry-point behavior change that pivoted design mid-PR (81d6190 swapped the resolver-widening approach for [stdin] routing), and the inline nits (stale description, exec_stdin duplication, bun-as-node sibling) are worth a maintainer glance.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/runtime/cli/run_command.rs`:
- Around line 2888-2889: Update the caller of boot_stdin_script to propagate its
Result error instead of converting failures to false with unwrap_or(false).
Preserve false only for a successful non-match, so getcwd() and other
user-reachable syscall failures remain catchable rather than triggering fallback
module resolution.
In `@test/cli/run/run_command.test.ts`:
- Around line 46-49: Replace the expanded regression narrative above the `#7102`
test in run_command.test.ts with only the linked issue URL comment, preserving
the test itself unchanged.
- Around line 61-68: Update the FIFO writer promise in the async feed operation
by removing the catch that converts failures into resolved values, so errors
from open, writeFile, or close reject and propagate through await feed. Keep the
existing Promise.all and feed sequencing unchanged.
- Around line 97-100: Strengthen the stderr assertion in the run-command test by
requiring the expected /dev/fd/N descriptor path produced by the argv1
error-reporting behavior, rather than only excluding pipe:[inode]. Keep the
existing boom and exit-code assertions, and ensure the matcher validates the
intended descriptor-path format.
🪄 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: 8536631b-f20e-4dc4-bc8a-b653e2c0885f
📒 Files selected for processing (2)
src/runtime/cli/run_command.rstest/cli/run/run_command.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/cli/run_command.rs:2959-2962— The comment aboveSelf::boot(...)was carried over verbatim fromexec_stdinand is now stale for the FIFO caller: it says "the stdin path skipsconfigure_allocator(long_running=true)" (butmaybe_open_with_bun_jscalls it at ~2867 before invoking this helper) and annotatesbasename(target_name) (= "-")(but the param is nowargv1, which is the FIFO //dev/fd/Npath for the new caller, not"-"). Consider generalizing to e.g. "printsbasename(argv1)rather than[stdin]" and dropping the stdin-only rationale.Extended reasoning...
Summary
Commit e2443cd extracted the shared
boot_stdin_scripthelper (per the earlier dedup review comment) and moved the pre-existing comment block fromexec_stdininto it verbatim. That comment was written when the block had a single caller (bun run -) and two of its claims no longer hold now thatmaybe_open_with_bun_js's ISFIFO branch also calls it.The comment as it stands (run_command.rs:2955-2959)
// NOT routed through `_boot_and_handle_error` — the // stdin path skips the // `configure_allocator(long_running=true)` / `.md` checks and prints // `basename(target_name)` (= "-"), not `basename(entry_path)` // (= "[stdin]"), in the error message.
Step-by-step: why it's stale
- "the stdin path skips
configure_allocator(long_running=true)" — true only for theexec_stdincaller. The FIFO caller enters atmaybe_open_with_bun_js, which explicitly runsGlobal::configure_allocator(GlobalConfigureAllocator { long_running: true, .. })at lines 2867-2870 before theISFIFObranch at line 2880 callsSelf::boot_stdin_scriptat line 2888. So on that path the allocator is already configured and the comment's rationale ("skips") does not describe the shared helper's contract. basename(target_name) (= "-")— the parameter passed toboot_failed_exitis nowargv1(line 2962), nottarget_name, and its value is onlyb"-"for theexec_stdincaller. For the FIFO caller it'starget— the literal path the user typed, e.g./dev/fd/63or/tmp/xxx/script— so the(= "-")annotation is wrong for one of the two callers andtarget_nameno longer names anything in scope.- The core point the comment protects — "NOT routed through
_boot_and_handle_error… prints the argv1 basename rather than[stdin]in the error message" — is still correct and still worth keeping; only the caller-specific parentheticals drifted.
Why the existing code doesn't prevent it
This is purely a doc-comment; nothing in the build or lint checks that comments match parameter names or caller sets. The helper's own doc comment at lines 2926-2928 is accurate (it correctly says
argv1is"-"for stdin or the path as typed for a FIFO), so the drift is confined to this one inline block.Impact
Zero runtime impact — nit-level. The risk is REVIEW.md's "Comments carry only durable non-obvious content": a future reader debugging
_boot_and_handle_errorvsboot_failed_exitdivergence for a FIFO entry will read "skips configure_allocator" /(= "-")and be misled about what the FIFO path actually does.Fix
Generalize to describe the shared contract, e.g.:
// NOT routed through `_boot_and_handle_error` — this path skips its // `.md`-suffix check and prints `basename(argv1)` ("-" or the FIFO // path as typed) rather than `basename(entry_path)` (= "[stdin]") // in the error message.
or simply drop the
(= "-")and theconfigure_allocatorclause. - "the stdin path skips
There was a problem hiding this comment.
No issues found — the change is small, well-tested, and all prior review threads are resolved. Deferring only because routing FIFO entries through [stdin] semantics (sets process._eval, tsx loader, [stdin] in stack frames rather than /dev/fd/N) is a user-visible API decision worth a maintainer's sign-off.
What was reviewed:
- fd lifetime in the new
ISFIFObranch —File::from_fdowns the fd and closes on drop; no double-close with the regular-file path's explicitclose(the branch returns/diverges first). boot_failed_exitreturns!, so theErrarm cannot fall through toget_fd_path.exec_stdinrefactor is behavior-preserving (argv1 = b"-", same entry-path construction).- Tests: writer now races in
Promise.all; bash-dependent testsskipIf(!bash); assertions pinargv[1],import.meta.main, and the[stdin]:1frame.
Extended reasoning...
Overview
Two files touched. In src/runtime/cli/run_command.rs, maybe_open_with_bun_js gains an S::ISFIFO(mode) branch that reads the already-open fd to EOF and boots via a new boot_stdin_script helper. That helper is the body of the pre-existing exec_stdin extracted verbatim with argv1 parameterized (previously hardcoded b"-"), so bun run - and FIFO entry now share one implementation of the cwd/[stdin] synthetic entry point. The regular-file path (get_fd_path → _boot_and_handle_error) is untouched. Three new POSIX-only tests in test/cli/run/run_command.test.ts cover a named FIFO, bash <(), and the error stack frame.
Security risks
None identified. The branch is gated on S::ISFIFO on an fd the process already opened; contents are read into an owned Vec<u8> and handed to the same eval-script slot bun run - uses. No new path resolution, no privilege boundary crossed, no untrusted-length arithmetic.
Level of scrutiny
Medium. This is CLI entry-point dispatch — the code that decides how bun <path> boots — but the change is narrow (~30 net lines of Rust) and additive: it inserts one branch ahead of get_fd_path and refactors exec_stdin into a shared helper without changing its observable behavior. I verified fd ownership (the temporary File::from_fd(fd) closes on drop; the FIFO branch either returns or diverges via boot_failed_exit -> !, so the later explicit close(fd) at :2900/:2906 is never reached), and that the fstat refactor (is_dir → keep full st) preserves the existing ISDIR early-return.
Other factors
All six prior review threads (mine and CodeRabbit's) are resolved: the copy-pasted STDIN_TRIGGER/getcwd block was deduped into boot_stdin_script; the writer's swallowed .catch(e => e) was removed and feed moved into Promise.all; boot_stdin_script's Err now routes through boot_failed_exit; the error test pins [stdin]:1; the bun-as-node sibling is explicitly documented as node-parity in the PR body. The one declined nit (read_to_end_into → return false discards errno) matches the pre-existing pattern at exec_stdin:2922 and the neighboring open/fstat/get_fd_path arms — reasonable to defer.
The reason I'm not approving outright: the PR deliberately gives FIFO entries bun run - semantics (Node module globals exposed, process._eval set, tsx loader, [stdin] as the source name in stacks and import.meta.path) rather than file-entry semantics. The PR body argues this well, and the alternative was tried and reverted for breaking process.execArgv, but it is a product-level choice about what bun <(cmd) means that a maintainer should confirm.
|
CI on 76b92db is green for this change: |
What
On macOS the same invocation printed
error: script not found "/dev/fd/13". The expected output ishello.Cause
maybe_open_with_bun_jsopens the path (/dev/fd/N),fstats the fd, then callsget_fd_pathto derive the canonical absolute path before booting. For a pipe that readlink resolves topipe:[inode]on Linux and fails on macOS (noF_GETPATHfor a pipe), and the subsequent boot cannot load the module.Fix
When
fstaton the already-open fd says the entry point is a FIFO, read it right there and route through the existingcwd/[stdin]virtual-source path thatbun run -already uses. The path the user typed is prepended to passthrough soprocess.argv[1]is still/dev/fd/N(or the FIFO path). Regular-file entry points are untouched and still go throughget_fd_pathso symlinked scripts keep resolving to their real location.The check is deliberately only
S_ISFIFO: sockets cannot be re-opened via/dev/fd/Non Linux soopenalready fails beforefstat, and reading a char/block device there (e.g.bun /dev/zero) would hang.Why the
[stdin]path and not a new virtual-source mechanismAn earlier revision kept
/dev/fd/Nas the entry path and widened theeval_sourcematch in the resolver/loader to accept an exact match on the stored source path. That brokebun run -e foo file.js(where-e foois only recorded inprocess.execArgvandfile.jsis the entry), becauseboot()stores the-estring ineval_sourcewithpath.text = file.jsregardless, and the widened match then servedfooinstead of reading the file. Reverted in 81d6190.Routing through
[stdin]means a pipe entry behaves exactly likebun run -does today: the built-in Node module globals are exposed,process._evalis set, and the loader istsx. That is consistent (a pipe you read once is semantically stdin, not a file you can reopen) and keeps the change to a single function inrun_command.rs. A dedicated pre-read virtual-source slot that keeps file-entry semantics would have to re-gate five separateeval_source.is_some()consumers and is not obviously the right semantics anyway.Verification
New tests in
test/cli/run/run_command.test.ts(POSIX only) cover a named FIFO entry point, abash <()process-substitution entry point, and the error output. All three fail on the current release withModule not found 'pipe:[...]'and pass with this change.process.execArgvintest/js/node/process/process.test.jsand all oftest/cli/run/run-eval.test.tspass unchanged.Fixes #7102
The bun-as-
nodeshim (exec_as_if_node) is left unchanged: real Node's default also fails on<()on Linux because its CJS loader realpaths the entry, so the shim matching that is node-parity.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run_command.test.ts