shell: fix inverted file-blob check for builtin `> ${Response(...)}` redirect

Closed
RO
17d ago
+330-175

What does this PR do?

Builtin::init_redirections guards a > ${jsvalue} stdout/stderr redirect against in-memory Blob bodies with:

let is_file_blob = matches!(body, Value::Blob(b) if !b.needs_to_read_file());
if (redirect.stdout() || redirect.stderr()) && !is_file_blob { /* throw */ }

needs_to_read_file() is true for file-backed blobs, so is_file_blob was true for in-memory bodies and false for file-backed ones, the opposite of the sibling direct-Blob branch. Since echo always runs as a builtin:

import { $ } from "bun";
// throws "Cannot redirect stdout/stderr to an immutable blob", should write
await $`echo hi > ${new Response(Bun.file("/tmp/out"))}`;
// succeeds with exit 0 and output discarded, should throw
await $`echo hi > ${new Response(new Blob(["x"]))}`;

Fixing the predicate alone isn't sufficient: a file-backed body that passes the guard was stored as BuiltinIO::Blob(..), and write_no_io_to treated that variant as Ok(buf.len()) (a no-op), so output would still be silently dropped. The same silent drop also affected echo hi > ${Bun.file(fd)} (fd-backed blobs reach the JsBuf arm because only path-backed Bun.file is inlined as a path string by handle_template_value).

Fix

The Request/Response and bare-Blob JsBuf branches now share one setup_blob_redirect helper:

  • a file-backed blob has its store.data.File.pathlike extracted and is opened through the same IOReader/IOWriter path that an Atom (> filename) redirect already uses (hoisted into open_file_redirect); an fd-backed blob is shell_dup'd so the writer owns a private fd, and the dup'd fd is probed with is_pollable() so a pipe/socket/tty takes the async writer path
  • an in-memory blob as stdout/stderr throws "Cannot redirect stdout/stderr to an immutable blob"; as stdin it stays BuiltinInput::Blob

This mirrors what Stdio::extract_blob already does for the spawned-subprocess path in Cmd::init_subproc_redirections.

The BuiltinIO::Blob output variant is no longer constructed anywhere and has been removed so the silent-drop arm cannot reappear; BuiltinInput::Blob stays for in-memory stdin.

Related

  • #35314 adds a Locked/Used/Error body check just before this branch; that PR does not touch the polarity and its added check_body_for_redirect call sits cleanly above the new body.use_() line.
  • #33996 fixes the interpreter keepalive left pinned after a Yield::failed() throw; the new tests spawn a subprocess per case so they do not depend on that fix.
  • setup_blob_redirect also routes < ${Response(Bun.file(p))} stdin through IOReader (previously it delivered empty bytes via shared_view() on a file store). This is only observable via builtin cat (Windows, or POSIX with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1), where builtin cat < file has a separate pre-existing issue on POSIX; left out of this PR's test coverage.

How did you verify your code works?

Added to test/js/bun/shell/bunshell.test.ts under redirect builtin stdout to Blob / Response body:

  • echo > ${new Response(Bun.file(path))} writes hello\n to the file
  • echo > ${new Response(new Blob([..]))}, ${new Response("..")}, ${new Blob([..])} each reject with the exact "immutable blob" message
  • echo > ${Bun.file(fd)} writes to the fd (and the user's fd stays open)
USE_SYSTEM_BUN=1 bun test bunshell.test.ts -t "redirect builtin stdout"   # 3 fail, 2 pass
bun bd test bunshell.test.ts -t "redirect builtin stdout"                 # 5 pass
bun bd test bunshell.test.ts                                              # 419 pass, 0 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/bunshell.test.ts