add PTY support

[whoiskatrin]

This PR adds full PTY (pseudo-terminal) support to the Sandbox SDK, enabling interactive terminal sessions with proper terminal emulation.

Summary

• Add PTY support for interactive terminal sessions with xterm-256color emulation
• Support both WebSocket (real-time) and HTTP (polling) transports for PTY I/O
• Include dimension validation, structured exit codes, and graceful disconnect handling

Features
New sandbox.pty namespace:

• create(options) - Create a new PTY session with configurable terminal size, command, cwd, and env
• attach(sessionId) - Attach PTY to an existing session (inherits cwd/env)
• getById(id) - Reconnect to an existing PTY session
• list() - List all active PTY sessions
• getInfo(id) - Get PTY metadata
• resize(id, cols, rows) - Resize terminal dimensions
• write(id, data) - Send input to PTY
• kill(id, signal?) - Terminate PTY with optional signal

PTY Handle API:

• write(data) - Send input to the terminal
• resize(cols, rows) - Resize terminal window
• kill(signal?) - Terminate with optional signal
• onData(callback) - Subscribe to terminal output
• onExit(callback) - Handle process exit
• exited - Promise that resolves with exit code
• Async iteration support for scripting use cases

Container Runtime:

• PtyManager - Manages PTY lifecycle using Bun.Terminal API
• Dimension validation (1-1000 for both cols and rows)
• Structured exit codes with signal mapping (SIGINT, SIGTERM, SIGKILL, etc.)
• Configurable disconnect timeout for orphaned sessions
• Session attachment for cwd/env inheritance

Example Usage

// Create PTY with custom dimensions
const pty = await sandbox.pty.create({ cols: 120, rows: 40 });
// Subscribe to output
pty.onData((data) => terminal.write(data));
// Send commands
pty.write('cd /workspace && ls -la\n');
// Handle exit
pty.onExit((code) => console.log(`Exited with code ${code}`));
// Or use async iteration
for await (const data of pty) {
  process.stdout.write(data);
}

[changeset-bot]

🦋 Changeset detected

Latest commit: 5c7de26

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@cloudflare/sandbox	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@310

commit: 5c7de26

[github-actions]

🐳 Docker Images Published

Default:

FROM cloudflare/sandbox:0.0.0-pr-310-0ae4905

With Python:

FROM cloudflare/sandbox:0.0.0-pr-310-0ae4905-python

With OpenCode:

FROM cloudflare/sandbox:0.0.0-pr-310-0ae4905-opencode

Version: 0.0.0-pr-310-0ae4905

Use the -python variant if you need Python code execution, or -opencode for the variant with OpenCode AI coding agent pre-installed.

---

📦 Standalone Binary

For arbitrary Dockerfiles:

COPY --from=cloudflare/sandbox:0.0.0-pr-310-0ae4905 /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]

Download via GitHub CLI:

gh run download 20973719172 -n sandbox-binary

Extract from Docker:

docker run --rm cloudflare/sandbox:0.0.0-pr-310-0ae4905 cat /container-server/sandbox > sandbox && chmod +x sandbox

[claude]

Claude Code Review

This PR adds comprehensive PTY support with a well-designed architecture that addresses most critical issues from previous reviews. Error handling has improved significantly - terminal operations now wrap exceptions and return structured results. However, two critical issues remain that could cause production reliability problems.

Critical Issues

1. SSE stream listeners continue firing after enqueue failures

packages/sandbox-container/src/handlers/pty-handler.ts:361-390, 393-424 - When controller.enqueue() throws (client disconnected), the error is caught and logged but the listener keeps firing. Every subsequent PTY data event triggers another caught exception, filling logs and wasting CPU.

Fix: On enqueue error, call unsubData() and unsubExit() immediately inside the catch blocks to stop further callbacks.

2. WebSocket double-failure scenario loses user feedback

packages/sandbox-container/src/handlers/ws-adapter.ts:107-129, 133-161 - When PTY write/resize fails AND the error notification cannot be sent over WebSocket (dead connection), users receive no feedback. The operation promise never resolves/rejects, leaving users unable to distinguish between slow operations and failed operations.

This violates the core principle: users must always know when operations fail. Consider adding request/response pattern with explicit IDs for PTY operations, or document this limitation clearly in the fire-and-forget JSDoc.

Important Issues

3. Missing critical test coverage

The primary use case - WebSocket PTY integration - has zero E2E test coverage. The collaborative-terminal example shows this is the intended production pattern, yet tests only cover HTTP fallback. Also missing:

• Disconnect timeout behavior (resource leak risk)
• Bun.Terminal unavailability error handling
• Session attachment conflict unit test

See the pr-test-analyzer agent report for detailed severity ratings. At minimum, add WebSocket PTY E2E test before merge.

Positive Changes

• terminal.write() and resize() now wrapped in try-catch with structured error returns
• proc.exited has both .then() and .catch() handlers
• Listeners properly cleaned up in cancel() callback (SSE streams)
• No any types - proper BunTerminal interface defined
• Error messages are clear and actionable

Verdict

Fix SSE listener cleanup (critical #1) and either fix or clearly document the WebSocket double-failure scenario (critical #2). Add at minimum the WebSocket PTY E2E test. The architectural foundation is solid, but these reliability issues could create difficult-to-debug production problems.

[whoiskatrin]

this has been also tested by @sdan in his work, he has positive feedback

[claude]

Claude Code Review

All previous error handling issues are fixed. The write/resize error handling, disconnect scenarios, and callback wrapping are properly implemented.

However, the specialized review agents identified several issues in disconnect/cleanup paths where errors are logged but not acted upon:

Critical:

1. WebSocketAdapter error notification failure (ws-adapter.ts:117-127, 147-159): When PTY write/resize fails AND the error notification to client fails, the error is only logged. The client never learns the operation failed and continues interacting with a broken PTY. Should close the WebSocket to force reconnect.

2. PtyManager disconnect timer lacks context (pty-manager.ts:395-406): When disconnect timeout fires and kill() fails, error is logged but lacks critical context (PTY state, process PID, whether PTY is now in zombie state). The catch block doesn't check kill()'s return value - if kill() returns {success: false}, that's swallowed.

High Priority:
3. SSE stream listener leak (pty-handler.ts:361-390): When SSE enqueue fails (client disconnected), the error is caught and logged but listeners remain registered. PTY continues generating output that's discarded, wasting resources. Should unsubscribe immediately.

4. WebSocket disconnect cleanup (ws-transport.ts:604-635): Stream controller errors during disconnect are logged at debug level. Should be warning level to surface potential race conditions.

Test Coverage Gaps:
The test suite covers happy paths well but misses critical edge cases:

• Empty write operations (severity 9)
• Rapid resize operations (severity 8)
• Disconnect timeout behavior (severity 8)
• Control character handling (Ctrl+C, Ctrl+Z) (severity 8)
• WebSocket vs HTTP transport differences (severity 9)
• Async iterator edge cases (severity 8)

The agents have detailed analysis in their output. Recommend addressing the critical error handling issues before merge, and adding the high-severity test gaps soon after.

Verdict: Address critical error notification and disconnect handling issues, then merge. Test gaps can follow.

[claude]

Critical: When error notification fails, client will never know the operation failed. They'll continue interacting with a broken PTY. Close the WebSocket to force reconnect:

if (!errorSent) {
  this.logger.error(...);
  try {
    ws.close(1011, 'Failed to send error notification');
  } catch (closeError) {
    this.logger.error('Failed to close WebSocket', closeError, { connectionId: ws.data.connectionId });
  }
}

[claude]

High: When enqueue fails, listeners remain registered and PTY continues generating discarded output. Unsubscribe immediately to prevent resource waste:

if (isExpectedDisconnect) {
  logger.debug('SSE stream closed, cleaning up listeners', { ptyId });
} else {
  logger.error('SSE stream enqueue failed, cleaning up listeners', ...);
}
unsubData?.();
unsubExit?.();

[ghostwriternr]

The overall implementation is v solid and the example you've built is easily my favourite example we've shipped. The API - especially the choice to use the pty namespace under the sandbox class is neat. I wish we did that for all methods, maybe we for v1 💁

100% of my comments in this PR are mainly around the architecture of the system itself (specifically transports), cleaning up confusing semantics (attach to session), and eliminating some deadcode/redundancies that got accumulated over the course of what is clearly a non-trivial change. Aside from the transport recommendations, all the other recommendation will primarily reduce the amount of code we ship (which is one of my fav takeaways from this article Peter shared yesterday) and keep the implementation leaner/cleaner.

[ghostwriternr]

These methods probably don't belong here. The more I thought about it, it feels like a very natural fit for the PTY support is to use websockets because of the continuous bidirectional communication on every input (keystroke) and continuous output - and this is something you've already done (looking mainly at the changes here and to ws-transport.ts (and the unimplemented stubs on the http transport).

But the above is also exactly why the way to achieve this is to not add PTY-specific components to the transport layer itself, which serves a more foundational purpose that both the HTTP & WS transports implement. But instead, the PTY feature should unconditionally use the websocket transport and use the standard interface provided by the transport instead.

I'd imagine we could instead perhaps have (these are just illustrative, could think about whether the existing methods are useful too):

abstract sendMessage(message: object): void;
abstract onStreamEvent(
  streamId: string,
  event: string,
  callback: (data: string) => void
): () => void;

and have both transports implement this. Remove all pty specific code like listeners from the ws-transport implementation and use a more standardised streamEventListeners perhaps, and pty-client could possibly then do things like:

transport.sendMessage({ type: 'pty_input', ptyId, data });
transport.onStreamEvent(ptyId, 'pty_data', callback);

Right now, this sort of very tight unidirectional coupling is defeating the purpose of having a common transport.

[ghostwriternr]

Other files in this PR that are related to the same comment: ws-transport.ts, http-transport.ts, base-transport.ts, pty-client.ts

[ghostwriternr]

And rough pseudocode for how pty-client (?) changes as a result could be:

private ptyTransport: WebSocketTransport | null = null;

private async getPtyTransport(): Promise<WebSocketTransport> {
  if (!this.ptyTransport) {
    this.ptyTransport = new WebSocketTransport({...});
    await this.ptyTransport.connect();
  }
  return this.ptyTransport;
}

async create(options?: CreatePtyOptions): Promise<Pty> {
  const transport = await this.getPtyTransport();
  // ... create PTY, pass transport to PtyHandle
}

[ghostwriternr]

We seem to have a lot of code around the implementation for this attach feature, and while I initially got excited by the idea of it, the more I looked at this, the less clear I was on the exact purpose of this one. When I read "attach", I assumed it implies an ongoing relationship between PTY and session, but in reality the PTY gets its own bash process that diverges immediately after creation. And this part feels fair - because our session implementation uses FIFO-based I/O and primarily designed for a non-interactive use-case, wherein PTY uses TTY-based I/O and is for interactive use-cases. But what this means, is that attach is primarily just a way to avoid the user having to write

const pty = await sandbox.pty.create({
  cwd: '/workspace/project',
  env: { NODE_ENV: 'development' }
});

which is something we entirely support already. And furthermore, when you look at the fact that we currently use getInitialCwd and getInitialEnv, it's also not the latest state of the session but is only the initial state - making this entire attach feature even less enticing/useful.

And the existence of this feature has also lead to implementation of exclusive control, which doesn't make sense because the PTY and the session are divergent anyway so there is no reason to block one or the other.

So my personal recommendation is to remove the attach feature entirely unless there are strong reasons to keep it. We could get rid of the attach() method, handleAttach(), getSessionInfo, sessionToPty mapping, hasActivePty(), getBySessionId(), checkPtyExclusiveControl(), the attach route, AttachPtyOptions type, and PTY_EXCLUSIVE_CONTROL error code. And any other related code too.

[ghostwriternr]

This comment is maybe relevant to the other signals too. When I tested using the example app you have deployed to your account, it looks like Ctrl+C/Z send signals to the shell process only, but not to the foreground process group. So for instance, if you run sleep 100 and press Ctrl+C/Z, nothing happens because the SIGINT gets sent to bash but is ignored and thus the sleep never gets SIGINT.

Can you remind me why we need this special handling here? Naively, it feels like this method could just be:

write(id: string, data: string): { success: boolean; error?: string } {
  const session = this.sessions.get(id);
  if (!session) return { success: false, error: 'PTY not found' };
  if (session.state !== 'running') return { success: false, error: 'PTY has exited' };

  session.terminal.write(data);
  return { success: true };
}

But there's certainly a reason why this was added that I'm missing in my testing.

[whoiskatrin]

It's due to this bug in bun.terminal oven-sh/bun#25834

[ghostwriternr]

Looks like these methods are never used (start and cancel), but are only being used in tests. We could remove them? And by extension, I noticed the disconnectTimer/disconnectTimeout option defined elsewhere seems to be unused as well. And fwiw - following the general pattern in this SDK - I lean towards not having any automatic timeouts at all in the SDK. We could provide an optional option if users ask, but for simplicity, I'd just remove it because it's guaranteed to surprise users if we implement this properly and PTY just disconnects after 30 seconds of inactivity by default and users have to read source code/docs to understand how to not have that happen. Instead, we already expose kill to users so if they need timeouts, they can achieve it using very little code anyway and they have much more control over the process.

[ghostwriternr]

This method seems unused as well and only used by tests - and is not exposed to developers either. We can delete this and all related code if not needed?

[ghostwriternr]

+related to ws cleanup

[ghostwriternr]

Here, and maybe a few other places, I noticed we're re-defining some bun types ourselves, but possibly the much cleaner approach would be to use "@types/bun": "^1.3.5" which should have all these types and use them directly instead.

[ghostwriternr]

Any reason why this exists here, as well as in PtyHandle (where it does seem more appropriate)? We can probably delete these?

[ghostwriternr]

nit: this should probably be called from the cleanup function in server.ts?

[ghostwriternr]

Nit: This gets eliminated based on the comment re: attach feature (regardless of whether attach itself gets removed, this is bound to go I think). But perhaps the right set of error codes might be PTY_NOT_FOUND, PTY_WRITE_FAILED, PTY_ALREADY_EXITED that gets used appropriately. Right now, this PR does not use our overall error handling system and instead uses vanilla Error which doesn't surface up in useful manner.