From c4d0f5219a68d1b1743d0bd64d295c7c01f2f0b7 Mon Sep 17 00:00:00 2001 From: Gabriel Brown Date: Fri, 10 Jul 2026 17:19:38 -0400 Subject: [PATCH] fix(terminal): use real TTY exec and resize --- apps/agent-worker/src/runtime/docker.ts | 8 + apps/agent-worker/src/terminal.ts | 140 +++++++++--------- .../tests/unit/terminal-resize.test.ts | 46 ++++++ 3 files changed, 124 insertions(+), 70 deletions(-) create mode 100644 apps/agent-worker/tests/unit/terminal-resize.test.ts diff --git a/apps/agent-worker/src/runtime/docker.ts b/apps/agent-worker/src/runtime/docker.ts index f169928..f3a23fc 100644 --- a/apps/agent-worker/src/runtime/docker.ts +++ b/apps/agent-worker/src/runtime/docker.ts @@ -1,6 +1,7 @@ import { mkdir } from 'node:fs/promises'; import path from 'node:path'; import type { Readable } from 'node:stream'; +import Docker from 'dockerode'; import { execa } from 'execa'; import { env } from '../env'; @@ -10,6 +11,13 @@ type CommandResult = { output: string; }; +let dockerClient: Docker | undefined; + +export const getDockerClient = () => { + dockerClient ??= new Docker(); + return dockerClient; +}; + const environmentArgs = (environment: Record) => Object.entries(environment).flatMap(([name, value]) => [ '-e', diff --git a/apps/agent-worker/src/terminal.ts b/apps/agent-worker/src/terminal.ts index c99a5a3..b9d4c62 100644 --- a/apps/agent-worker/src/terminal.ts +++ b/apps/agent-worker/src/terminal.ts @@ -1,11 +1,11 @@ -import { spawn } from 'node:child_process'; -import type { ChildProcessWithoutNullStreams } from 'node:child_process'; import type { Server } from 'node:http'; +import type { Duplex } from 'node:stream'; import type { WebSocket } from 'ws'; import { WebSocketServer } from 'ws'; import type { BoxHandle } from './user-container'; import { env } from './env'; +import { getDockerClient } from './runtime/docker'; import { verifyTerminalToken } from './terminal-token'; import { acquireUserBox } from './user-container'; import { getTerminalWorkspace } from './worker'; @@ -16,8 +16,26 @@ const clampDimension = (value: unknown) => { return Math.min(Math.max(n, 1), 1000); }; -// Single-quote a string for a POSIX shell. -const shellQuote = (value: string) => `'${value.replaceAll("'", `'\\''`)}'`; +export const parseResizeMessage = ( + data: Buffer, + isBinary: boolean, +): { cols: number; rows: number } | null => { + if (isBinary) return null; + try { + const message = JSON.parse(data.toString('utf8')) as { + type?: string; + cols?: number; + rows?: number; + }; + if (message.type !== 'resize') return null; + const cols = clampDimension(message.cols); + const rows = clampDimension(message.rows); + if (!cols || !rows) return null; + return { cols, rows }; + } catch { + return null; + } +}; const bridge = async (ws: WebSocket, jobId: string) => { const workspace = getTerminalWorkspace(jobId); @@ -26,41 +44,27 @@ const bridge = async (ws: WebSocket, jobId: string) => { return; } - // bun can't load node-pty (native ABI mismatch) and dockerode can't attach to - // podman, so we drive the runtime CLI (` exec -i`) and allocate the PTY - // *inside* the container with `script`, bridging the plain pipes to the socket. - // // Register the message handler immediately and buffer input/size until the exec // is ready (acquiring the box can take seconds on first connect), so the initial // resize and early keystrokes aren't dropped. - const procHolder: { current?: ChildProcessWithoutNullStreams } = {}; + const execHolder: { + current?: { exec: import('dockerode').Exec; stream: Duplex }; + } = {}; const pendingInput: Buffer[] = []; let cols = 80; let rows = 24; ws.on('message', (data: Buffer, isBinary: boolean) => { - if (!isBinary) { - // Text frames are control messages (resize); anything else is raw input. - try { - const message = JSON.parse(data.toString('utf8')) as { - type?: string; - cols?: number; - rows?: number; - }; - if (message.type === 'resize') { - const c = clampDimension(message.cols); - const r = clampDimension(message.rows); - if (c && r) { - cols = c; - rows = r; - } - return; - } - } catch { - // fall through: treat as raw input - } + const resize = parseResizeMessage(data, isBinary); + if (resize) { + cols = resize.cols; + rows = resize.rows; + void execHolder.current?.exec + .resize({ h: rows, w: cols }) + .catch(() => undefined); + return; } - if (procHolder.current) procHolder.current.stdin.write(data); + if (execHolder.current) execHolder.current.stream.write(data); else pendingInput.push(data); }); @@ -72,7 +76,8 @@ const bridge = async (ws: WebSocket, jobId: string) => { const cleanup = () => { if (released) return; released = true; - procHolder.current?.kill(); + execHolder.current?.stream.end(); + execHolder.current?.stream.destroy(); handleHolder.current?.release(); }; ws.on('close', cleanup); @@ -103,54 +108,47 @@ const bridge = async (ws: WebSocket, jobId: string) => { return; } - // Reattach a persistent tmux session across reconnects when available, else a - // plain login shell. `stty` sizes the PTY to the client's viewport up front. - const launcher = - `stty rows ${rows} cols ${cols} 2>/dev/null; ` + - // Reattach a persistent tmux session when tmux is present; otherwise fall back - // to an interactive login shell (`-i` so it prints a prompt and line-edits). - // Check with `command -v` rather than `exec tmux || …`: a failed `exec` makes a - // non-interactive shell exit before the `||`, so the fallback never runs. - 'if command -v tmux >/dev/null 2>&1; then exec tmux new-session -A -s spoon; ' + - 'else exec bash -il; fi'; - const envFlags = [ - '-e', - 'TERM=xterm-256color', - '-e', - `HOME=${workspace.containerHome}`, - ...workspace.secrets.flatMap((s) => ['-e', `${s.name}=${s.value}`]), - ]; - - const proc = spawn( - env.containerRuntime, - [ - 'exec', - '-i', - ...envFlags, - '-w', - workspace.containerRepo, - boxName, + const docker = getDockerClient(); + const container = docker.getContainer(boxName); + const exec = await container.exec({ + AttachStdin: true, + AttachStdout: true, + AttachStderr: true, + Tty: true, + Cmd: [ '/bin/bash', '-lc', - `exec script -qfc ${shellQuote(launcher)} /dev/null`, + 'if command -v tmux >/dev/null 2>&1; then exec tmux new-session -A -s spoon; else exec bash -il; fi', ], - { stdio: ['pipe', 'pipe', 'pipe'] }, - ); - procHolder.current = proc; + Env: [ + 'TERM=xterm-256color', + `HOME=${workspace.containerHome}`, + ...workspace.secrets.map((secret) => `${secret.name}=${secret.value}`), + ], + WorkingDir: workspace.containerRepo, + }); + const stream = await exec.start({ hijack: true, stdin: true, Tty: true }); + execHolder.current = { exec, stream }; + + if (isReleased()) { + stream.end(); + stream.destroy(); + return; + } + + await exec.resize({ h: rows, w: cols }).catch(() => undefined); // Replay any keystrokes the client sent before the process was ready. - for (const buffered of pendingInput) proc.stdin.write(buffered); + for (const buffered of pendingInput) stream.write(buffered); pendingInput.length = 0; - const forward = (chunk: Buffer) => { + stream.on('data', (chunk: Buffer) => { if (ws.readyState === ws.OPEN) ws.send(chunk, { binary: true }); - }; - proc.stdout.on('data', forward); - proc.stderr.on('data', forward); - proc.on('exit', () => { + }); + stream.on('end', () => { if (ws.readyState === ws.OPEN) ws.close(); }); - proc.on('error', () => { + stream.on('error', () => { if (ws.readyState === ws.OPEN) ws.close(); }); }; @@ -179,7 +177,9 @@ export const attachTerminalServer = (server: Server) => { return; } wss.handleUpgrade(request, socket, head, (ws) => { - void bridge(ws, jobId); + void bridge(ws, jobId).catch(() => { + if (ws.readyState === ws.OPEN) ws.close(); + }); }); }); diff --git a/apps/agent-worker/tests/unit/terminal-resize.test.ts b/apps/agent-worker/tests/unit/terminal-resize.test.ts new file mode 100644 index 0000000..4a7d1fe --- /dev/null +++ b/apps/agent-worker/tests/unit/terminal-resize.test.ts @@ -0,0 +1,46 @@ +import { afterEach, describe, expect, test, vi } from 'vitest'; + +const load = async () => { + vi.resetModules(); + process.env.SPOON_WORKER_TOKEN = 'test-worker-token'; + process.env.GITHUB_APP_ID = '123'; + process.env.GITHUB_APP_PRIVATE_KEY = + '-----BEGIN PRIVATE KEY-----\ntest\n-----END PRIVATE KEY-----'; + vi.doMock('../../src/worker', () => ({ getTerminalWorkspace: () => null })); + vi.doMock('../../src/user-container', () => ({ + acquireUserBox: vi.fn(), + })); + return await import('../../src/terminal'); +}; + +describe('parseResizeMessage', () => { + afterEach(() => vi.resetModules()); + + test('parses and clamps a resize control frame', async () => { + const { parseResizeMessage } = await load(); + const msg = Buffer.from( + JSON.stringify({ type: 'resize', cols: 120, rows: 40 }), + ); + expect(parseResizeMessage(msg, false)).toEqual({ cols: 120, rows: 40 }); + }); + + test('returns null for binary input (raw keystrokes)', async () => { + const { parseResizeMessage } = await load(); + expect(parseResizeMessage(Buffer.from([1, 2, 3]), true)).toBeNull(); + }); + + test('returns null for non-resize JSON', async () => { + const { parseResizeMessage } = await load(); + expect( + parseResizeMessage(Buffer.from('{"type":"other"}'), false), + ).toBeNull(); + }); + + test('clamps out-of-range dimensions into [1,1000]', async () => { + const { parseResizeMessage } = await load(); + const msg = Buffer.from( + JSON.stringify({ type: 'resize', cols: 99999, rows: 0 }), + ); + expect(parseResizeMessage(msg, false)).toEqual({ cols: 1000, rows: 1 }); + }); +});