Skip to content

Commit 0245de5

Browse files
Claude Botclaude
andcommitted
fix: address CodeRabbit review comments for PTY support
- 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 <noreply@anthropic.com>
1 parent 6d4965c commit 0245de5

File tree

6 files changed

+44
-35
lines changed

6 files changed

+44
-35
lines changed

docs/runtime/child-process.mdx

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,7 @@ console.log(output); // includes ANSI color codes
138138
### Checking isTTY in child process
139139

140140
```ts
141-
const proc = Bun.spawn({
142-
cmd: ["bun", "-e", "console.log('isTTY:', process.stdout.isTTY)"],
141+
const proc = Bun.spawn(["bun", "-e", "console.log('isTTY:', process.stdout.isTTY)"], {
143142
stdout: "pty",
144143
});
145144

@@ -152,16 +151,13 @@ console.log(output); // "isTTY: true"
152151
You can use PTY for stdin, stdout, and/or stderr. When multiple streams use PTY, they share the same underlying pseudo-terminal:
153152

154153
```ts
155-
const proc = Bun.spawn({
156-
cmd: [
157-
"bun",
158-
"-e",
159-
`
160-
console.log("stdout.isTTY:", process.stdout.isTTY);
161-
console.log("stdin.isTTY:", process.stdin.isTTY);
162-
console.log("stderr.isTTY:", process.stderr.isTTY);
163-
`,
164-
],
154+
const code = `
155+
console.log("stdout.isTTY:", process.stdout.isTTY);
156+
console.log("stdin.isTTY:", process.stdin.isTTY);
157+
console.log("stderr.isTTY:", process.stderr.isTTY);
158+
`;
159+
160+
const proc = Bun.spawn(["bun", "-e", code], {
165161
stdin: "pty",
166162
stdout: "pty",
167163
stderr: "pty",
@@ -178,15 +174,12 @@ const output = await proc.stdout.text();
178174
Specify terminal width and height using the object syntax:
179175

180176
```ts
181-
const proc = Bun.spawn({
182-
cmd: [
183-
"bun",
184-
"-e",
185-
`
186-
console.log("columns:", process.stdout.columns);
187-
console.log("rows:", process.stdout.rows);
188-
`,
189-
],
177+
const code = `
178+
console.log("columns:", process.stdout.columns);
179+
console.log("rows:", process.stdout.rows);
180+
`;
181+
182+
const proc = Bun.spawn(["bun", "-e", code], {
190183
stdout: {
191184
type: "pty",
192185
width: 120, // columns

packages/bun-types/bun.d.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5407,7 +5407,7 @@ declare module "bun" {
54075407
* - `"ignore"`, `null`, `undefined`: The process will have no standard input (default)
54085408
* - `"pipe"`: The process will have a new {@link FileSink} for standard input
54095409
* - `"inherit"`: The process will inherit the standard input of the current process
5410-
* - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdin.isTTY === true`. Falls back to `"pipe"` on Windows.
5410+
* - `"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`.
54115411
* - `ArrayBufferView`, `Blob`, `Bun.file()`, `Response`, `Request`: The process will read from buffer/stream.
54125412
* - `number`: The process will read from the file descriptor
54135413
*
@@ -5416,7 +5416,7 @@ declare module "bun" {
54165416
* - `"pipe"`, `undefined`: The process will have a {@link ReadableStream} for standard output/error
54175417
* - `"ignore"`, `null`: The process will have no standard output/error
54185418
* - `"inherit"`: The process will inherit the standard output/error of the current process
5419-
* - `"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.
5419+
* - `"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`.
54205420
* - `ArrayBufferView`: The process write to the preallocated buffer. Not implemented.
54215421
* - `number`: The process will write to the file descriptor
54225422
*
@@ -5431,7 +5431,7 @@ declare module "bun" {
54315431
* - `"ignore"`, `null`, `undefined`: The process will have no standard input
54325432
* - `"pipe"`: The process will have a new {@link FileSink} for standard input
54335433
* - `"inherit"`: The process will inherit the standard input of the current process
5434-
* - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdin.isTTY === true`. Falls back to `"pipe"` on Windows.
5434+
* - `"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`.
54355435
* - `ArrayBufferView`, `Blob`: The process will read from the buffer
54365436
* - `number`: The process will read from the file descriptor
54375437
*
@@ -5444,7 +5444,7 @@ declare module "bun" {
54445444
* - `"pipe"`, `undefined`: The process will have a {@link ReadableStream} for standard output/error
54455445
* - `"ignore"`, `null`: The process will have no standard output/error
54465446
* - `"inherit"`: The process will inherit the standard output/error of the current process
5447-
* - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stdout.isTTY === true`. Falls back to `"pipe"` on Windows.
5447+
* - `"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`.
54485448
* - `ArrayBufferView`: The process write to the preallocated buffer. Not implemented.
54495449
* - `number`: The process will write to the file descriptor
54505450
*
@@ -5457,7 +5457,7 @@ declare module "bun" {
54575457
* - `"pipe"`, `undefined`: The process will have a {@link ReadableStream} for standard output/error
54585458
* - `"ignore"`, `null`: The process will have no standard output/error
54595459
* - `"inherit"`: The process will inherit the standard output/error of the current process
5460-
* - `"pty"`: The process will use a pseudo-terminal (PTY). The child will see `process.stderr.isTTY === true`. Falls back to `"pipe"` on Windows.
5460+
* - `"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`.
54615461
* - `ArrayBufferView`: The process write to the preallocated buffer. Not implemented.
54625462
* - `number`: The process will write to the file descriptor
54635463
*

src/bun.js/api/bun/js_bun_spawn_bindings.zig

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -635,11 +635,15 @@ pub fn spawnMaybeSync(
635635
.stdout_maxbuf = subprocess.stdout_maxbuf,
636636
};
637637

638-
log("After subprocess init: stdout state={s}, stdin FD={?d}, stdout FD={?d}", .{
639-
@tagName(subprocess.stdout),
640-
if (spawned.stdin) |fd| fd.native() else null,
641-
if (spawned.stdout) |fd| fd.native() else null,
642-
});
638+
if (comptime Environment.isPosix) {
639+
log("After subprocess init: stdout state={s}, stdin FD={?d}, stdout FD={?d}", .{
640+
@tagName(subprocess.stdout),
641+
if (spawned.stdin) |fd| fd.native() else null,
642+
if (spawned.stdout) |fd| fd.native() else null,
643+
});
644+
} else {
645+
log("After subprocess init: stdout state={s}", .{@tagName(subprocess.stdout)});
646+
}
643647

644648
subprocess.process.setExitHandler(subprocess);
645649

src/bun.js/api/bun/process.zig

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1531,7 +1531,13 @@ pub fn spawnProcessPosix(
15311531
// Use existing PTY slave (should have been created from primary stdio)
15321532
if (pty_slave) |slave| {
15331533
try actions.dup2(slave, fileno);
1534-
try extra_fds.append(pty_master.?);
1534+
// dup() the master FD so each extra_fd has its own FD for epoll
1535+
const duped = try bun.sys.dup(pty_master.?).unwrap();
1536+
if (!options.sync) {
1537+
try bun.sys.setNonblocking(duped).unwrap();
1538+
}
1539+
try to_close_on_error.append(duped);
1540+
try extra_fds.append(duped);
15351541
}
15361542
},
15371543
}

src/bun.js/api/bun/spawn/stdio.zig

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,9 @@ pub const Stdio = union(enum) {
476476

477477
if (try value.get(globalThis, "width")) |width_val| {
478478
if (!width_val.isUndefinedOrNull()) {
479+
if (!width_val.isNumber()) {
480+
return globalThis.throwInvalidArguments("PTY width must be a number", .{});
481+
}
479482
const width = width_val.toInt32();
480483
if (width <= 0 or width > std.math.maxInt(u16)) {
481484
return globalThis.throwInvalidArguments("PTY width must be a positive integer <= 65535", .{});
@@ -486,6 +489,9 @@ pub const Stdio = union(enum) {
486489

487490
if (try value.get(globalThis, "height")) |height_val| {
488491
if (!height_val.isUndefinedOrNull()) {
492+
if (!height_val.isNumber()) {
493+
return globalThis.throwInvalidArguments("PTY height must be a number", .{});
494+
}
489495
const height = height_val.toInt32();
490496
if (height <= 0 or height > std.math.maxInt(u16)) {
491497
return globalThis.throwInvalidArguments("PTY height must be a positive integer <= 65535", .{});
@@ -501,7 +507,7 @@ pub const Stdio = union(enum) {
501507
}
502508
}
503509

504-
return globalThis.throwInvalidArguments("stdio must be an array of 'inherit', 'ignore', 'pty', or null", .{});
510+
return globalThis.throwInvalidArguments("stdio must be an array of 'inherit', 'pipe', 'ignore', 'pty', Bun.file(pathOrFd), number, or null", .{});
505511
}
506512

507513
pub fn extractBlob(stdio: *Stdio, globalThis: *jsc.JSGlobalObject, blob: jsc.WebCore.Blob.Any, i: i32) bun.JSError!void {

src/shell/subproc.zig

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,8 @@ pub const ShellSubprocess = struct {
178178
.ipc, .capture => {
179179
return Writable{ .ignore = {} };
180180
},
181-
.pty, .readable_stream, .pipe => {
182-
// The shell never uses these for stdin in sync mode
181+
.pty => {
182+
// The shell never uses PTY directly for stdin
183183
return Writable{ .ignore = {} };
184184
},
185185
}

0 commit comments

Comments
 (0)