From 1824f53c89f709ce73c35d9388ca39e4fe1a5460 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Thu, 30 Jul 2026 05:02:10 +0000 Subject: [PATCH] let the pty sidecar decide what shell it runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit officer built the whole PtyInitConfig: it read the owner's SHELL (defaulting to /bin/zsh), added `-i`, read their HOME, expanded `~` against it, and hardcoded `host: true`. none of that is a proxy's business — the sidecar is the process that calls pty.spawn, so it is the one that should know what to spawn and where. the config now carries only what the bridge actually knows: sessionId, the folder the panel was opened on, and the client's cols/rows. shell, args, home and cwd resolution moved into the sidecar. home comes from HOME_DIR ?? HOME, mirroring data-path.ts:getOwnerHomeDir — terminal was the one host-executing surface reading process.env.HOME directly, which is identical here and divergent anywhere HOME_DIR is set to something else. deleted the bwrap sandbox branch rather than moving it. it was selected by `config.host`, which officer hardcoded to true, so it never ran — and it expected `shell` to contain a fully-built bwrap command that nothing on either side ever built. it could not have worked. a terminal here is the owner's own shell on the owner's own machine by design (platform/CLAUDE.md), so there is no jail to preserve. its ensureUserFiles half duplicated api/users/provision.ts:seedShellConfigs, which is the live seeder of those same templates and stays. also deleted the 'cwd' handler that turned a message into `cd \r` typed at the shell. no frontend has ever sent that message — the browser composes its own cd — so it was unreachable, and synthesizing keystrokes is not something a relay should do. the integration test pins SHELL and HOME_DIR now that the sidecar reads them, and asserts the shell starts in the resolved `~` rather than officer having resolved it. Co-Authored-By: Claude Opus 4.8 --- src/servers/api/terminal/pty-sidecar.mjs | 113 +++++-------------- src/servers/api/terminal/pty-sidecar.test.ts | 33 +++--- src/servers/api/terminal/websocket.ts | 36 ++---- src/servers/sidecar/protocol.ts | 9 +- 4 files changed, 57 insertions(+), 134 deletions(-) diff --git a/src/servers/api/terminal/pty-sidecar.mjs b/src/servers/api/terminal/pty-sidecar.mjs index f2b4f5a1..72cc72d6 100644 --- a/src/servers/api/terminal/pty-sidecar.mjs +++ b/src/servers/api/terminal/pty-sidecar.mjs @@ -10,24 +10,10 @@ process.on('SIGINT', () => { }); process.on('SIGTERM', () => process.emit('SIGINT')); -import { existsSync } from 'node:fs'; -import { cp, mkdir } from 'node:fs/promises'; -import { join, dirname } from 'node:path'; -import { fileURLToPath } from 'node:url'; -import { execFile } from 'node:child_process'; +import { join } from 'node:path'; import WebSocket from 'ws'; import * as pty from 'node-pty'; -const run = (cmd, args, opts = {}) => - new Promise((resolve) => { - const proc = execFile(cmd, args, { stdio: 'ignore', ...opts }, () => resolve()); - proc.on('error', () => resolve()); - }); - -const __dirname = dirname(fileURLToPath(import.meta.url)); - -const templateDir = join(__dirname, 'templates'); - import 'dotenv/config'; const API_URL = process.env.API_URL ?? `ws://127.0.0.1:${process.env.PORT ?? '5000'}`; @@ -36,7 +22,31 @@ const REGISTER_URL = `${API_URL}/api/sidecar/register`; const BUFFER_MAX = 50 * 1024; const RECONNECT_DELAYS = [200, 500, 1000, 2000, 4000, 8000, 15000]; -/** @type {Map} */ +// ── What kind of shell this process runs ── +// +// These used to arrive inside every pty:init, which meant officer chose the owner's shell and read the +// owner's HOME to do it. They are this process's business: it is the one that spawns the thing. +// +// HOME_DIR mirrors `data-path.ts:getOwnerHomeDir` — on a host where the owner's real login home differs +// from this process's HOME, the shell should open in the former, like every other host-executing surface. +const HOME_DIR = process.env.HOME_DIR ?? process.env.HOME ?? process.cwd(); +const SHELL = { command: process.env.SHELL ?? '/bin/zsh', args: ['-i'] }; +// A terminal is always a plain host shell: the owner is the only account and it is their own machine +// (`platform/CLAUDE.md` — do not add a jail without being asked). There used to be a second branch here +// for a bwrap sandbox, selected by `config.host`, which officer hardcoded to true. Nothing ever built the +// bwrap command it expected, on either side, so it could not have run — it is in git history if the +// decision is ever revisited, and the `ensureUserFiles` half of it duplicated +// `api/users/provision.ts:seedShellConfigs`, which is the live seeder of those templates. + +const resolveCwd = (cwd) => { + if (!cwd || cwd === '~') return HOME_DIR; + if (cwd.startsWith('~/')) return join(HOME_DIR, cwd.slice(2)); + if (cwd.startsWith('/')) return cwd; + // Relative paths have no meaning here — this process's cwd is the repo, not the user's folder. + return HOME_DIR; +}; + +/** @type {Map} */ const sessions = new Map(); // ── Helpers ── @@ -59,32 +69,6 @@ const sendJson = (msg) => { } }; -const ensureUserFiles = async (homeDir) => { - await mkdir(homeDir, { recursive: true }); - await mkdir(join(homeDir, '.config'), { recursive: true }); - await mkdir(join(homeDir, '.local', 'bin'), { recursive: true }); - - const zshrcPath = join(homeDir, '.zshrc'); - if (!existsSync(zshrcPath)) { - await cp(join(templateDir, '.zshrc'), zshrcPath); - } - - const tmuxconfPath = join(homeDir, '.tmux.conf'); - if (!existsSync(tmuxconfPath)) { - await cp(join(templateDir, '.tmux.conf'), tmuxconfPath); - } - - const starshipPath = join(homeDir, '.config', 'starship-officer.toml'); - if (!existsSync(starshipPath)) { - await cp(join(templateDir, 'starship-officer.toml'), starshipPath); - } - - const ohMyZshPath = join(homeDir, '.oh-my-zsh'); - if (!existsSync(ohMyZshPath)) { - await run('git', ['clone', '--depth=1', 'https://github.com/ohmyzsh/ohmyzsh.git', ohMyZshPath]); - } -}; - const appendBuffer = (session, data) => { session.buffer += data; if (session.buffer.length > BUFFER_MAX) { @@ -130,47 +114,18 @@ async function handleCommand(msg) { } // New session — spawn PTY - const shell = config.shell ?? { command: '/bin/bash', args: ['-i'] }; - const cwd = config.cwd ?? process.cwd(); - const homeDir = config.homeDir ?? process.cwd(); - const userLabel = config.userLabel ?? 'officer'; + const cwd = resolveCwd(config.cwd); const cols = config.cols ?? 80; const rows = config.rows ?? 24; - const isHost = !!config.host; - - let spawnCommand; - let spawnArgs; - let ptyEnv; - - if (isHost) { - spawnCommand = shell.command; - spawnArgs = shell.args ?? []; - ptyEnv = { ...process.env, TERM: 'xterm-256color', ...(config.env ?? {}) }; - } else { - // Sandboxed mode: shell config contains the full bwrap command - spawnCommand = shell.command; - spawnArgs = shell.args ?? []; - - try { - await ensureUserFiles(homeDir); - } catch (err) { - console.error('[pty-sidecar] ensureUserFiles failed:', err); - } - - // bwrap sets env vars internally via --setenv, so use minimal host env - ptyEnv = { TERM: 'xterm-256color' }; - } - - const ptyCwd = isHost ? cwd : undefined; let term; try { - term = pty.spawn(spawnCommand, spawnArgs, { + term = pty.spawn(SHELL.command, SHELL.args, { name: 'xterm-256color', cols, rows, - cwd: ptyCwd, - env: ptyEnv, + cwd, + env: { ...process.env, TERM: 'xterm-256color' }, }); } catch (err) { const message = err instanceof Error ? err.message : 'Failed to start terminal'; @@ -179,13 +134,7 @@ async function handleCommand(msg) { return; } - const session = { - term, - buffer: '', - cols, - rows, - initConfig: { shell, cwd, homeDir, userLabel }, - }; + const session = { term, buffer: '', cols, rows }; sessions.set(sessionId, session); term.onData((output) => { diff --git a/src/servers/api/terminal/pty-sidecar.test.ts b/src/servers/api/terminal/pty-sidecar.test.ts index ba774e01..000de990 100644 --- a/src/servers/api/terminal/pty-sidecar.test.ts +++ b/src/servers/api/terminal/pty-sidecar.test.ts @@ -81,31 +81,28 @@ describe('pty sidecar', () => { const port = officer.port; child = Bun.spawn(['node', SIDECAR], { - env: { ...process.env, API_URL: `ws://127.0.0.1:${port}` }, + env: { + ...process.env, + API_URL: `ws://127.0.0.1:${port}`, + // The sidecar now chooses the shell and the home itself, so the test pins both rather than + // spawning the owner's interactive zsh (which would read their rc files and their history). + SHELL: '/bin/sh', + HOME_DIR: '/tmp', + ENV: '/dev/null', + }, stdout: 'ignore', stderr: 'ignore', }); await officer.await((f) => f.type === 'register' && f.capabilities?.includes('terminal')); - // A plain non-interactive shell: no rc files, no prompt, so the assertions below are about output - // the test itself caused. - officer.send({ - type: 'pty:init', - id: 'i1', - sessionId, - config: { - sessionId, - host: true, - shell: { command: '/bin/sh', args: [] }, - cwd: '/tmp', - homeDir: '/tmp', - cols: 80, - rows: 24, - }, - }); + // `~` is resolved by the sidecar against its own HOME_DIR, not by officer. + officer.send({ type: 'pty:init', id: 'i1', sessionId, config: { sessionId, cwd: '~', cols: 80, rows: 24 } }); await officer.await((f) => f.type === 'pty:ready' && f.sessionId === sessionId); + officer.send({ type: 'pty:input', id: 'in0', sessionId, data: 'pwd\n' }); + await officer.await(isOutput(sessionId, '/tmp')); + officer.send({ type: 'pty:input', id: 'in1', sessionId, data: 'echo before-restart\n' }); await officer.await(isOutput(sessionId, 'before-restart')); @@ -121,7 +118,7 @@ describe('pty sidecar', () => { // Re-attaching replays the scrollback, marked as history so the client can rebuild rather than // append — and it contains what happened on both sides of the restart. - officer.send({ type: 'pty:init', id: 'i2', sessionId, config: { sessionId, host: true, cols: 80, rows: 24 } }); + officer.send({ type: 'pty:init', id: 'i2', sessionId, config: { sessionId, cols: 80, rows: 24 } }); const replay = await officer.await((f) => f.type === 'pty:replay' && f.sessionId === sessionId); expect(String(replay.data)).toContain('before-restart'); expect(String(replay.data)).toContain('after-restart'); diff --git a/src/servers/api/terminal/websocket.ts b/src/servers/api/terminal/websocket.ts index 045fe9e0..97dcb95e 100644 --- a/src/servers/api/terminal/websocket.ts +++ b/src/servers/api/terminal/websocket.ts @@ -1,5 +1,4 @@ import type { ServerWebSocket } from 'bun'; -import { join } from 'node:path'; import { sendPtyCommand, sendPtyCommandAsync, on, isTerminalConnected } from '@@/sidecar-registry'; import type { PtyInitConfig } from '../../sidecar/protocol'; @@ -34,13 +33,6 @@ const sendOutput = (ws: ServerWebSocket, data: string) => { } }; -const resolveCwd = (home: string, cwd?: string) => { - if (!cwd || cwd === '~') return home; - if (cwd.startsWith('~/')) return join(home, cwd.slice(2)); - if (cwd.startsWith('/')) return cwd; - return home; -}; - export const terminalWebsocket = { async open(ws: ServerWebSocket) { const { email, username } = ws.data; @@ -54,17 +46,10 @@ export const terminalWebsocket = { const sessionId = ws.data.sessionId ?? `host-${ws.data.userId}`; - // The server owner is the only account, so the terminal is always a plain host shell. - const config: PtyInitConfig = { - sessionId, - host: true, - shell: { command: process.env.SHELL ?? '/bin/zsh', args: ['-i'] }, - cwd: resolveCwd(process.env.HOME!, ws.data.cwd), - homeDir: process.env.HOME!, - userLabel: email, - cols: ws.data.cols, - rows: ws.data.rows, - }; + // Everything this bridge knows: which session, which folder the panel was opened on, and how big the + // client's viewport is. The shell, its arguments and the home directory are the sidecar's — it is the + // process that spawns them, and officer has no business reading the owner's SHELL and HOME to guess. + const config: PtyInitConfig = { sessionId, cwd: ws.data.cwd, cols: ws.data.cols, rows: ws.data.rows }; // The sidecar emits one global stream, so each frame is filtered down to this session and relabelled. const relay = (event: 'pty:output' | 'pty:replay' | 'pty:exit', clientType: string) => @@ -118,16 +103,9 @@ export const terminalWebsocket = { }); } break; - case 'cwd': - if (msg.path) { - sendPtyCommand({ - type: 'pty:input', - id: nextId(), - sessionId: session.sessionId, - data: `cd ${JSON.stringify(msg.path)}\r`, - }); - } - break; + // There was a 'cwd' case here that typed `cd \r` into the user's shell. No frontend sends + // that message — the browser composes its own `cd` (Terminal.tsx / CommandTerminalWrapper.tsx) — + // so it was unreachable, and synthesizing keystrokes is not a thing a proxy should do. } } catch { // ignore malformed messages diff --git a/src/servers/sidecar/protocol.ts b/src/servers/sidecar/protocol.ts index 522df93d..f9177f72 100644 --- a/src/servers/sidecar/protocol.ts +++ b/src/servers/sidecar/protocol.ts @@ -130,16 +130,15 @@ export type VncSessionInfo = { // ── PTY types ── +// What officer knows about a terminal, and nothing more. The shell, its arguments, the home directory and +// whether the shell is sandboxed are the sidecar's own decisions — they used to travel in here, which is +// how officer ended up reading the owner's SHELL and HOME and hardcoding `host: true`. export type PtyInitConfig = { sessionId: string; - shell?: { command: string; args?: string[] }; + /** The folder the panel was opened on. `~`, `~/x` and absolute paths only; resolved by the sidecar. */ cwd?: string; - homeDir?: string; - userLabel?: string; - host?: boolean; cols?: number; rows?: number; - env?: Record; }; // PTY commands (API → PTY sidecar)