From 0245de5c78eb03e5dfbbd26a8c80ccf61a2df574 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Wed, 3 Dec 2025 11:10:10 +0000 Subject: [PATCH] fix: address CodeRabbit review comments for PTY support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix docs example consistency: use array form consistently - Add spawnSync warning to TypeScript PTY type definitions - Fix Windows compile error in js_bun_spawn_bindings.zig - Fix extra_fds PTY to use dup() for consistency - Add isNumber() type check for PTY width/height options - Fix error message consistency in stdio.zig - Fix switch case overlap in shell/subproc.zig (remove .pipe/.readable_stream that were already handled) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- docs/runtime/child-process.mdx | 35 ++++++++------------ packages/bun-types/bun.d.ts | 10 +++--- src/bun.js/api/bun/js_bun_spawn_bindings.zig | 14 +++++--- src/bun.js/api/bun/process.zig | 8 ++++- src/bun.js/api/bun/spawn/stdio.zig | 8 ++++- src/shell/subproc.zig | 4 +-- 6 files changed, 44 insertions(+), 35 deletions(-) diff --git a/docs/runtime/child-process.mdx b/docs/runtime/child-process.mdx index 629bb79719..97257710c7 100644 --- a/docs/runtime/child-process.mdx +++ b/docs/runtime/child-process.mdx @@ -138,8 +138,7 @@ console.log(output); // includes ANSI color codes ### Checking isTTY in child process ```ts -const proc = Bun.spawn({ - cmd: ["bun", "-e", "console.log('isTTY:', process.stdout.isTTY)"], +const proc = Bun.spawn(["bun", "-e", "console.log('isTTY:', process.stdout.isTTY)"], { stdout: "pty", }); @@ -152,16 +151,13 @@ console.log(output); // "isTTY: true" You can use PTY for stdin, stdout, and/or stderr. When multiple streams use PTY, they share the same underlying pseudo-terminal: ```ts -const proc = Bun.spawn({ - cmd: [ - "bun", - "-e", - ` - console.log("stdout.isTTY:", process.stdout.isTTY); - console.log("stdin.isTTY:", process.stdin.isTTY); - console.log("stderr.isTTY:", process.stderr.isTTY); - `, - ], +const code = ` + console.log("stdout.isTTY:", process.stdout.isTTY); + console.log("stdin.isTTY:", process.stdin.isTTY); + console.log("stderr.isTTY:", process.stderr.isTTY); +`; + +const proc = Bun.spawn(["bun", "-e", code], { stdin: "pty", stdout: "pty", stderr: "pty", @@ -178,15 +174,12 @@ const output = await proc.stdout.text(); Specify terminal width and height using the object syntax: ```ts -const proc = Bun.spawn({ - cmd: [ - "bun", - "-e", - ` - console.log("columns:", process.stdout.columns); - console.log("rows:", process.stdout.rows); - `, - ], +const code = ` + console.log("columns:", process.stdout.columns); + console.log("rows:", process.stdout.rows); +`; + +const proc = Bun.spawn(["bun", "-e", code], { stdout: { type: "pty", width: 120, // columns diff --git a/packages/bun-types/bun.d.ts b/packages/bun-types/bun.d.ts index 1340b57071..7bfad2efd1 100644 --- a/packages/bun-types/bun.d.ts +++ b/packages/bun-types/bun.d.ts @@ -5407,7 +5407,7 @@ declare module "bun" { * - `"ignore"`, `null`, `undefined`: The process will have no standard input (default) * - `"pipe"`: The process will have a new {@link FileSink} for standard input * - `"inherit"`: The process will inherit the standard input of the current process - * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdin.isTTY === true`. Falls back to `"pipe"` on Windows. + * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdin.isTTY === true`. Falls back to `"pipe"` on Windows. Not supported with `spawnSync`. * - `ArrayBufferView`, `Blob`, `Bun.file()`, `Response`, `Request`: The process will read from buffer/stream. * - `number`: The process will read from the file descriptor * @@ -5416,7 +5416,7 @@ declare module "bun" { * - `"pipe"`, `undefined`: The process will have a {@link ReadableStream} for standard output/error * - `"ignore"`, `null`: The process will have no standard output/error * - `"inherit"`: The process will inherit the standard output/error of the current process - * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdout.isTTY === true` / `process.stderr.isTTY === true`. Falls back to `"pipe"` on Windows. + * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdout.isTTY === true` / `process.stderr.isTTY === true`. Falls back to `"pipe"` on Windows. Not supported with `spawnSync`. * - `ArrayBufferView`: The process write to the preallocated buffer. Not implemented. * - `number`: The process will write to the file descriptor * @@ -5431,7 +5431,7 @@ declare module "bun" { * - `"ignore"`, `null`, `undefined`: The process will have no standard input * - `"pipe"`: The process will have a new {@link FileSink} for standard input * - `"inherit"`: The process will inherit the standard input of the current process - * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdin.isTTY === true`. Falls back to `"pipe"` on Windows. + * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdin.isTTY === true`. Falls back to `"pipe"` on Windows. Not supported with `spawnSync`. * - `ArrayBufferView`, `Blob`: The process will read from the buffer * - `number`: The process will read from the file descriptor * @@ -5444,7 +5444,7 @@ declare module "bun" { * - `"pipe"`, `undefined`: The process will have a {@link ReadableStream} for standard output/error * - `"ignore"`, `null`: The process will have no standard output/error * - `"inherit"`: The process will inherit the standard output/error of the current process - * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdout.isTTY === true`. Falls back to `"pipe"` on Windows. + * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdout.isTTY === true`. Falls back to `"pipe"` on Windows. Not supported with `spawnSync`. * - `ArrayBufferView`: The process write to the preallocated buffer. Not implemented. * - `number`: The process will write to the file descriptor * @@ -5457,7 +5457,7 @@ declare module "bun" { * - `"pipe"`, `undefined`: The process will have a {@link ReadableStream} for standard output/error * - `"ignore"`, `null`: The process will have no standard output/error * - `"inherit"`: The process will inherit the standard output/error of the current process - * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stderr.isTTY === true`. Falls back to `"pipe"` on Windows. + * - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stderr.isTTY === true`. Falls back to `"pipe"` on Windows. Not supported with `spawnSync`. * - `ArrayBufferView`: The process write to the preallocated buffer. Not implemented. * - `number`: The process will write to the file descriptor * diff --git a/src/bun.js/api/bun/js_bun_spawn_bindings.zig b/src/bun.js/api/bun/js_bun_spawn_bindings.zig index d914a9070a..8ec181719a 100644 --- a/src/bun.js/api/bun/js_bun_spawn_bindings.zig +++ b/src/bun.js/api/bun/js_bun_spawn_bindings.zig @@ -635,11 +635,15 @@ pub fn spawnMaybeSync( .stdout_maxbuf = subprocess.stdout_maxbuf, }; - log("After subprocess init: stdout state={s}, stdin FD={?d}, stdout FD={?d}", .{ - @tagName(subprocess.stdout), - if (spawned.stdin) |fd| fd.native() else null, - if (spawned.stdout) |fd| fd.native() else null, - }); + if (comptime Environment.isPosix) { + log("After subprocess init: stdout state={s}, stdin FD={?d}, stdout FD={?d}", .{ + @tagName(subprocess.stdout), + if (spawned.stdin) |fd| fd.native() else null, + if (spawned.stdout) |fd| fd.native() else null, + }); + } else { + log("After subprocess init: stdout state={s}", .{@tagName(subprocess.stdout)}); + } subprocess.process.setExitHandler(subprocess); diff --git a/src/bun.js/api/bun/process.zig b/src/bun.js/api/bun/process.zig index e0fcbe09e5..c010c575ba 100644 --- a/src/bun.js/api/bun/process.zig +++ b/src/bun.js/api/bun/process.zig @@ -1531,7 +1531,13 @@ pub fn spawnProcessPosix( // Use existing PTY slave (should have been created from primary stdio) if (pty_slave) |slave| { try actions.dup2(slave, fileno); - try extra_fds.append(pty_master.?); + // dup() the master FD so each extra_fd has its own FD for epoll + const duped = try bun.sys.dup(pty_master.?).unwrap(); + if (!options.sync) { + try bun.sys.setNonblocking(duped).unwrap(); + } + try to_close_on_error.append(duped); + try extra_fds.append(duped); } }, } diff --git a/src/bun.js/api/bun/spawn/stdio.zig b/src/bun.js/api/bun/spawn/stdio.zig index 0e8ffbb77c..589558c2c4 100644 --- a/src/bun.js/api/bun/spawn/stdio.zig +++ b/src/bun.js/api/bun/spawn/stdio.zig @@ -476,6 +476,9 @@ pub const Stdio = union(enum) { if (try value.get(globalThis, "width")) |width_val| { if (!width_val.isUndefinedOrNull()) { + if (!width_val.isNumber()) { + return globalThis.throwInvalidArguments("PTY width must be a number", .{}); + } const width = width_val.toInt32(); if (width <= 0 or width > std.math.maxInt(u16)) { return globalThis.throwInvalidArguments("PTY width must be a positive integer <= 65535", .{}); @@ -486,6 +489,9 @@ pub const Stdio = union(enum) { if (try value.get(globalThis, "height")) |height_val| { if (!height_val.isUndefinedOrNull()) { + if (!height_val.isNumber()) { + return globalThis.throwInvalidArguments("PTY height must be a number", .{}); + } const height = height_val.toInt32(); if (height <= 0 or height > std.math.maxInt(u16)) { return globalThis.throwInvalidArguments("PTY height must be a positive integer <= 65535", .{}); @@ -501,7 +507,7 @@ pub const Stdio = union(enum) { } } - return globalThis.throwInvalidArguments("stdio must be an array of 'inherit', 'ignore', 'pty', or null", .{}); + return globalThis.throwInvalidArguments("stdio must be an array of 'inherit', 'pipe', 'ignore', 'pty', Bun.file(pathOrFd), number, or null", .{}); } pub fn extractBlob(stdio: *Stdio, globalThis: *jsc.JSGlobalObject, blob: jsc.WebCore.Blob.Any, i: i32) bun.JSError!void { diff --git a/src/shell/subproc.zig b/src/shell/subproc.zig index c48b6d7e09..7eff3c1f83 100644 --- a/src/shell/subproc.zig +++ b/src/shell/subproc.zig @@ -178,8 +178,8 @@ pub const ShellSubprocess = struct { .ipc, .capture => { return Writable{ .ignore = {} }; }, - .pty, .readable_stream, .pipe => { - // The shell never uses these for stdin in sync mode + .pty => { + // The shell never uses PTY directly for stdin return Writable{ .ignore = {} }; }, }