From 7e70f6f0c3bc0b7e57638e2ea287974313dc5fdf Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 6 Nov 2026 07:10:49 +0100 Subject: [PATCH] fix: keep an adopted session off the direct-PTY fallback (release review) A wrapper that could not start fell back to a direct PTY: `claude --dangerously-skip-permissions` or a server-account shell in /tmp on this host, under a tab still carrying the foreign session's name, so someone who opened an ssh or container session ended up typing into this machine. startInteractive and startShell now fail the start for an adopted session and leave `_useMux` on, so selecting the tab tries the wrapper again. The `error` event carries the reason, since the browser's re-attach does not read the route's answer. A session with no mux at all is refused the same way. Co-Authored-By: Claude Opus 5.5 --- docs/api-reference.md | 8 ++- docs/architecture-invariants.md | 2 +- src/session.ts | 24 +++++++ test/foreign-adoption-guards.test.ts | 93 +++++++++++++++++++++++++++- 4 files changed, 121 insertions(+), 6 deletions(-) diff --git a/docs/api-reference.md b/docs/api-reference.md index c733198e..00e64663 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1188,9 +1188,11 @@ session: respawn, Ralph (its config and the `fix-plan/write`, `fix-plan/read` an `ralph-prompt/write` file routes), auto-clear, auto-compact, auto-resume, custom model endpoints, prompt uploads and the image watcher. `stop` waits are a 400, and so are `blocked` waits unless Codeman reads that CLI's dialogs off its screen (codex), the git -routes answer `unsupported` with `reason: "adopted"`, and `last-response` answers empty, -since Codeman knows no transcript for it. Its `paneExit` stays absent, because the pane -holds a `tmux attach` and not the agent. Reboot restore does not rebuild one. +routes answer `unsupported` with `reason: "adopted"`, and +`last-response` answers empty, since Codeman knows no transcript for it. Its `paneExit` +stays absent, because the pane holds a `tmux attach` and not the agent. Reboot restore +does not rebuild one. A wrapper that cannot start answers `OPERATION_FAILED` and leaves +the tab to be tried again: Codeman never starts a local process in its place. ## Approvals Inbox diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 75db68ce..90238859 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -109,7 +109,7 @@ Input arriving during a wake is buffered: a chunk over 4 KB is dropped whole, ne **No workspace.** ⚠️ An adopted session keeps its connection on `adopt.remote`/`adopt.docker`, so `session.remote` and `session.docker` are undefined, and its `workingDir` is the neutral `/tmp`. Code that branches on those two reads an in-container or over-ssh adoption as a local session working in `/tmp`. So anything that treats `workingDir` as the session's workspace must also test `session.isAdopted` (`adopt` on a pure view): uploads, the upload GC, git status, git fetch and its scheduler, the image watcher, Read My Mind's git signals, the Discord folder, `last-response`, guarded input and Ralph's `@fix_plan.md` and `@ralph_prompt.md` routes all do. The file surfaces (Files panel, File Viewer and its edit mode) still treat `/tmp` as the workspace, which is harmless: it is world-writable anyway, and adoption is admin-only in multi-user mode. A wrapper reports `statusLineExporter: 'remote'`, which is also what keeps keep-warm off it. -**Lifecycle.** `paneExitApplies` is false for a wrapper, so an attach that ended is never `paneExit` and the sweep never closes the tab. Boot recovery re-attaches a dead wrapper, which prints that the session is gone and exits. Reboot restore refuses one, since all Codeman ever had was an attach command. +**Lifecycle.** `paneExitApplies` is false for a wrapper, so an attach that ended is never `paneExit` and the sweep never closes the tab. Boot recovery re-attaches a dead wrapper, which prints that the session is gone and exits. Reboot restore refuses one, since all Codeman ever had was an attach command. ⚠️ A wrapper that cannot start fails the start, and selecting the tab tries it again (`_failAdoptedStart()`). It never takes the direct-PTY fallback, which would run a new local `claude --dangerously-skip-permissions` or a server-account shell in `/tmp` under the foreign session's name. ### Docker cases diff --git a/src/session.ts b/src/session.ts index 03b03f7c..5bc1176c 100644 --- a/src/session.ts +++ b/src/session.ts @@ -3205,6 +3205,26 @@ export class Session extends EventEmitter { this.emit('output', data); } + /** + * End a start that cannot reach an adopted session's wrapper. + * + * ⚠️ Never the direct-PTY fallback. What that starts is a NEW process on THIS host + * (`claude --dangerously-skip-permissions`, or a server-account shell) in the neutral + * `/tmp`, under a tab still carrying the foreign session's name, so someone who opened an + * ssh or container session would end up typing into this machine. `_useMux` stays on, so + * the next start, which is what selecting the tab sends, tries the wrapper again. + * + * The `error` event is what the tab shows: the browser's re-attach does not read the + * route's answer. + */ + private _failAdoptedStart(err: unknown): never { + const message = `Could not attach to ${this._adopt?.targetSession}: ${getErrorMessage(err)}`; + console.error(`[Session] ${message}`); + this._status = 'stopped'; + this.emit('error', message); + throw new Error(message); + } + async startInteractive(): Promise { if (this.ptyProcess) { throw new Error('Session already has a running process'); @@ -3441,6 +3461,7 @@ export class Session extends EventEmitter { this._armPaneSettle(true); } } catch (err) { + if (this._adopt) this._failAdoptedStart(err); console.error('[Session] Failed to create mux session, falling back to direct PTY:', err); this._useMux = false; this._muxSession = null; @@ -3460,6 +3481,7 @@ export class Session extends EventEmitter { if (getCli(this.mode)?.capabilities.requiresMux) { throw new Error(`${getModeLabel(this.mode)} sessions require tmux. Direct PTY fallback is not supported.`); } + if (this._adopt) this._failAdoptedStart(new Error('tmux is not available')); try { // Pass --session-id to use the SAME ID as the Codeman session // This ensures subagents can be directly matched to the correct tab. @@ -4463,6 +4485,7 @@ export class Session extends EventEmitter { }, 100); } } catch (err) { + if (this._adopt) this._failAdoptedStart(err); console.error('[Session] Failed to create mux session, falling back to direct PTY:', err); this._useMux = false; this._muxSession = null; @@ -4471,6 +4494,7 @@ export class Session extends EventEmitter { // Fallback to direct PTY if mux is not used if (!this.ptyProcess) { + if (this._adopt) this._failAdoptedStart(new Error('tmux is not available')); try { this.ptyProcess = spawnPtyWithHelperRepair(() => pty.spawn(shell, [], { diff --git a/test/foreign-adoption-guards.test.ts b/test/foreign-adoption-guards.test.ts index ece62c1a..bed71d24 100644 --- a/test/foreign-adoption-guards.test.ts +++ b/test/foreign-adoption-guards.test.ts @@ -1,7 +1,7 @@ /** * @fileoverview The adoption guards that are not reachable over HTTP: the - * boot-time workspace-hook sweep, the workspace-trust auto-accept and the - * new-shell clear. + * boot-time workspace-hook sweep, the workspace-trust auto-accept, the new-shell + * clear and the direct-PTY fallback. * * Each acts on a session's WORKSPACE or its PANE, each runs unattended, and each * previously keyed off `session.remote`, the mode or nothing at all, none of which @@ -24,6 +24,20 @@ vi.mock('../src/hooks-config.js', async (importOriginal) => { return { ...actual, applyWorkspaceHooks: (...args: unknown[]) => applyWorkspaceHooks(...(args as [])) }; }); +// The direct-PTY fallback must never run the real `claude` or a real login shell from a +// test, and the spawn is the one thing that proves whether it happened at all. +const ptySpawn = vi.fn(); +vi.mock('node-pty', async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + spawn: (file: string, args: string[] | string, options: import('node-pty').IPtyForkOptions) => { + ptySpawn(file, args); + return real.spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], options); + }, + }; +}); + const { WebServer } = await import('../src/web/server.js'); const { Session } = await import('../src/session.js'); @@ -163,3 +177,78 @@ describe('the new-shell clear', () => { expect(await bytesWrittenAfterStart(adopt)).toEqual([]); }); }); + +describe('the direct-PTY fallback', () => { + const adopt = { location: 'remote', socketPath: '/tmp/tmux-0/default', targetSession: 'work', viewSession: '' }; + + /** A wrapper that cannot start: `createSession` threw (a loaded tmux server timing out, say). */ + const session = (mode: 'claude' | 'shell', opts: { adopt?: typeof adopt; useMux?: boolean } = {}) => { + const s = new Session({ + name: 'work', + workingDir: '/tmp', + mode, + useMux: opts.useMux ?? true, + mux: {}, + ...(opts.adopt ? { adopt: opts.adopt } : {}), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + const errors: string[] = []; + s.on('error', (message: string) => errors.push(message)); + const priv = s as unknown as Record; + priv._setupOrAttachMuxSession = async () => { + throw new Error('tmux new-session timed out'); + }; + return { s, priv, errors }; + }; + + beforeEach(() => ptySpawn.mockClear()); + + it('still catches a session Codeman started, whose fallback is a real PTY', async () => { + // The control: without it, "nothing spawned" below is equally explained by a mock + // that never sees a spawn. A shell, because a claude fallback also probes the real binary. + const { s } = session('shell'); + await s.startShell(); + expect(ptySpawn).toHaveBeenCalledTimes(1); + await s.stop(true).catch(() => {}); + }); + + it.each([ + ['claude', 'startInteractive'], + ['shell', 'startShell'], + ] as const)('never starts a local %s for an adopted session (%s)', async (mode, start) => { + // ⚠️ The tab still says it is someone else's session. A fallback here runs + // `claude --dangerously-skip-permissions` or a server-account shell in `/tmp` on THIS + // host, so whoever opened an ssh or container session ends up typing into this one. + const { s, errors } = session(mode, { adopt }); + await expect(s[start]()).rejects.toThrow(/tmux new-session timed out/); + expect(ptySpawn).not.toHaveBeenCalled(); + expect(s.pid).toBeNull(); + expect(s.status).toBe('stopped'); + // The browser's re-attach does not read the POST's answer, so this is what the tab shows. + expect(errors.join()).toMatch(/work/); + }); + + it('leaves the wrapper to be tried again', async () => { + const { s, priv } = session('shell', { adopt }); + await expect(s.startShell()).rejects.toThrow(); + const disposable = () => ({ dispose() {} }); + priv._setupOrAttachMuxSession = async () => { + priv.ptyProcess = { write: vi.fn(), onData: disposable, onExit: disposable, kill() {}, pid: 1 }; + return { isRestored: true, respawnedDeadPane: false }; + }; + await s.startShell(); + expect(s.pid).toBe(1); + expect(ptySpawn).not.toHaveBeenCalled(); + priv.ptyProcess = null; + await s.stop(true).catch(() => {}); + }); + + it.each([ + ['claude', 'startInteractive'], + ['shell', 'startShell'], + ] as const)('never starts a local %s for an adopted session with no mux at all (%s)', async (mode, start) => { + const { s } = session(mode, { adopt, useMux: false }); + await expect(s[start]()).rejects.toThrow(/tmux/); + expect(ptySpawn).not.toHaveBeenCalled(); + }); +});