From 5deb0d4a4c33cd1aacdd99efe70b58ce4283b39f Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 12 Jul 2026 19:49:39 +0200 Subject: [PATCH] fix(review): harden + wire remote-host SSH cases end-to-end (PR #145) - UI: add the missing data-tab="case-remote" tab button; dispatch it through submitCaseModal()/switchCaseModalTab() to linkRemoteCase() (was dead code). - Restore: restoreMuxSessions() now passes remote (muxSession.remote ?? savedState.remote) into the Session constructor, so remote metadata round-trips on restart instead of reattaching from a local cwd / respawning LOCAL / being erased from state.json. Recovery tests added. - Run flows: runClaude()/runShell() route remote cases through /api/quick-start (POST /api/sessions stat-validates workingDir locally); run*() skip the /api/*/status pre-check and omit inert config/env for remote cases. - Quick-start: resolve the remote case BEFORE the local CLI availability gates and skip isCodex/Gemini/OpenCodeAvailable() when remote; REJECT envOverrides/effort/codex/gemini/openCode config for remote (they don't cross ssh) instead of silently dropping them. - Injection: reject $, backtick, $( in remotePath + identityFile at the schema layer (they survive shellescape into the bash -c launch double-quote layer). Regression tests for $(...) and backtick payloads added. - Remote socket/name: launch on a DEDICATED -L codeman-remote socket under a codeman-ssh- name that fails a remote Codeman's SAFE_MUX_NAME_PATTERN, so a remote instance can't adopt the session; scope tmux set-options per-session (never -g) so they don't mutate other sessions. - Kill: best-effort ssh 'tmux -L codeman-remote kill-session' on remote session kill (fire-and-forget, never blocks/throws the local kill) so the remote agent isn't orphaned forever. - Probe: wire checkRemoteTmuxAvailable() into POST /api/quick-start (structured OPERATION_FAILED) and as courtesy validation in remote-link; add a default -o ConnectTimeout=10 to buildSshConnectionArgs (overridable via extraSshOptions). - Command default: remote claude default is now 'exec claude --dangerously-skip-permissions' (per-host override stays the escape hatch), mirroring local non-interactive semantics. Co-Authored-By: Claude Fable 5 --- src/remote-hosts.ts | 19 ++-- src/tmux-manager.ts | 86 +++++++++++++--- src/web/public/index.html | 1 + src/web/public/session-ui.js | 130 ++++++++++++++++++------ src/web/routes/case-routes.ts | 9 ++ src/web/routes/session-routes.ts | 106 ++++++++++++------- src/web/schemas.ts | 24 +++-- src/web/server.ts | 6 ++ test/remote-hosts.test.ts | 3 +- test/remote-ssh-options.test.ts | 50 +++++---- test/routes/case-routes.test.ts | 107 +++++++++++++++++++ test/routes/session-routes.test.ts | 66 +++++++++++- test/session-attachment-history.test.ts | 20 ++++ test/tmux-manager.test.ts | 37 ++++++- test/tmux-restart-recovery.test.ts | 45 ++++++-- 15 files changed, 580 insertions(+), 129 deletions(-) diff --git a/src/remote-hosts.ts b/src/remote-hosts.ts index a1d1acde..dbf82cff 100644 --- a/src/remote-hosts.ts +++ b/src/remote-hosts.ts @@ -60,7 +60,10 @@ export async function writeRemoteCases(configDir: string, cases: RemoteCase[]): export function defaultRemoteCommandForMode(mode: SessionMode): string { const commands: Record = { shell: 'exec bash -l', - claude: 'exec claude', + // Mirror the LOCAL claude default so the remote agent runs non-interactively + // (no trust-folder/permission prompt that nothing on the remote answers). The + // per-host `commands.claude` override stays the escape hatch. + claude: 'exec claude --dangerously-skip-permissions', opencode: 'exec opencode', codex: 'exec codex', gemini: 'exec gemini', @@ -105,6 +108,7 @@ function expandIdentityPath(identityFile: string): string { * Returns the leading tokens of an ssh command line (NOT including `-t`, the * target, or any remote command). Order: * ssh -o BatchMode=yes + * [-o ConnectTimeout=10] (default; suppressed if extraSshOptions sets it) * [-p ] * [-i ] (~/$HOME expanded, then shellescaped) * [-J ] (shellescaped, single token) @@ -115,11 +119,14 @@ function expandIdentityPath(identityFile: string): string { * - The ProxyCommand is emitted as a single shellescaped `-o KEY=VALUE`, so the * whole value (spaces + `%h`/`%p`) reaches ssh as one argument and `%h %p` * survive verbatim — ssh expands them to the real host/port, not the shell. - * - Empty options ⇒ `['ssh', '-o BatchMode=yes']` (+ `-p` only when set), i.e. - * byte-identical to the historical behavior. + * - A default `-o ConnectTimeout=10` bounds the wait on an unreachable/blackholed + * host (else the pane hangs on the OS TCP timeout). It is omitted when the + * operator already set ConnectTimeout via extraSshOptions, so their value wins. */ export function buildSshConnectionArgs(remote: RemoteSshOptions & Pick): string[] { const parts: string[] = ['ssh', '-o BatchMode=yes']; + const hasConnectTimeout = (remote.extraSshOptions ?? []).some((opt) => /^ConnectTimeout=/i.test(opt)); + if (!hasConnectTimeout) parts.push('-o ConnectTimeout=10'); if (remote.port) parts.push(`-p ${remote.port}`); if (remote.identityFile) parts.push(`-i ${shellescape(expandIdentityPath(remote.identityFile))}`); if (remote.jumpHost) parts.push(`-J ${shellescape(remote.jumpHost)}`); @@ -146,10 +153,8 @@ export function buildSshConnectionArgs(remote: RemoteSshOptions & Pick & RemoteSshOptions ): string { - const [ssh, ...connectionArgs] = buildSshConnectionArgs(host); - const parts = [ssh, connectionArgs[0], '-o ConnectTimeout=10', ...connectionArgs.slice(1)]; - parts.push(remoteSshTarget(host), "'command -v tmux'"); - return parts.join(' '); + // ConnectTimeout is now a default of buildSshConnectionArgs (shared with the launch). + return [...buildSshConnectionArgs(host), remoteSshTarget(host), "'command -v tmux'"].join(' '); } export interface RemoteTmuxCheckResult { diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index 0024f35f..9ac41d3a 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -671,16 +671,32 @@ function buildSpawnCommand(options: { return '$SHELL'; } +/** + * Dedicated socket for Codeman-launched REMOTE tmux servers, distinct from the + * canonical local `-L codeman` socket. A remote host that runs its OWN Codeman + * would otherwise share the `-L codeman` socket AND the `codeman-` discovery + * name, so its `reconcileSessions()` would ADOPT our session (attach a PTY, + * resize, respawn-pane it locally) — the cross-machine form of the "2nd instance + * attaches live sessions" hazard. A private socket keeps our remote sessions off + * that instance's radar entirely. + */ +const REMOTE_TMUX_SOCKET = 'codeman-remote'; + /** * Deterministic, reattach-stable remote tmux session name for a Codeman session. * - * Derived from the same stable field the LOCAL muxName uses - * (`codeman-${sessionId.slice(0, 8)}`), so reconnecting (which re-issues the - * exact same `ssh … new-session -A`) lands back in the SAME remote session. - * Must NOT be random/time-based — it has to be stable across reconnects. + * Derived from the same stable field the LOCAL muxName uses (the first 8 chars of + * the sessionId), so reconnecting (which re-issues the exact same + * `ssh … new-session -A`) lands back in the SAME remote session. Must NOT be + * random/time-based — it has to be stable across reconnects. + * + * The `codeman-ssh-` prefix is deliberately chosen to FAIL a remote Codeman's + * `SAFE_MUX_NAME_PATTERN` (`^codeman-[a-f0-9-]+$`) — the `s`/`h` letters mean a + * remote instance's discovery never treats this as one of its own sessions (belt + * to the dedicated-socket suspenders above). */ export function remoteTmuxSessionName(sessionId: string): string { - return `codeman-${sessionId.slice(0, 8)}`; + return `codeman-ssh-${sessionId.slice(0, 8)}`; } /** @@ -690,16 +706,22 @@ export function remoteTmuxSessionName(sessionId: string): string { * * Emits: * ssh -o BatchMode=yes -t [] user@host \ - * 'tmux -L codeman new-session -A -s codeman- -c "cd && exec " \ - * \; set -g status off \; set -g mouse off \; set -sg escape-time 0 \; set -g prefix C-q' + * 'tmux -L codeman-remote new-session -A -s codeman-ssh- -c "cd && exec " \ + * \; set -t codeman-ssh- status off \; set -t codeman-ssh- mouse off \ + * \; set -t codeman-ssh- prefix C-q \; set -s escape-time 0' * * COD-107 — the connection options (`-p`, `-i`, `-J`, SOCKS `-o ProxyCommand`, * arbitrary `-o`) come from the shared `buildSshConnectionArgs(remote)`, so the * prereq tmux probe and this launch connect with identical options. * - * - `new-session -A -s codeman-` = attach-if-exists-else-create (idempotent), - * so reconnect re-runs the same command and reattaches the still-running agent. - * - `-L codeman` = canonical remote socket (for Phase 2/3 discovery). + * - `new-session -A -s codeman-ssh-` = attach-if-exists-else-create + * (idempotent), so reconnect re-runs the same command and reattaches the + * still-running agent. + * - `-L codeman-remote` = a DEDICATED socket, NOT the canonical `-L codeman` a + * remote Codeman would use, so our session never collides with / gets adopted by + * an instance running on the remote host. + * - The `set` options are scoped per-session (`set -t ` / server-level + * `set -s`), never `-g`, so they never mutate other sessions' prefix/mouse. * - The whole tmux invocation is a SINGLE ssh argument (the remote login shell * runs it), so it is shell-quoted as one unit; the `cd && exec` command is in * turn a single tmux argument (tmux runs it via `/bin/sh -c`), so the path is @@ -721,13 +743,15 @@ export function buildRemoteLaunchCommand(options: { const paneCommand = `cd ${shellescape(remote.remotePath)} && ${modeCommand}`; // The tmux command line, with `\;` separating commands so the config `set`s - // apply on the SAME connection (and are idempotent on reattach). + // apply on the SAME connection (and are idempotent on reattach). Options are + // scoped per-session (`set -t ` / server `set -s`), NEVER `-g`, so a + // shared remote tmux server's other sessions keep their own prefix/mouse. const tmuxInvocation = [ - `tmux -L codeman new-session -A -s ${remoteName} -c ${shellescape(remote.remotePath)} ${shellescape(paneCommand)}`, - 'set -g status off', - 'set -g mouse off', - 'set -sg escape-time 0', - 'set -g prefix C-q', + `tmux -L ${REMOTE_TMUX_SOCKET} new-session -A -s ${remoteName} -c ${shellescape(remote.remotePath)} ${shellescape(paneCommand)}`, + `set -t ${remoteName} status off`, + `set -t ${remoteName} mouse off`, + `set -t ${remoteName} prefix C-q`, + 'set -s escape-time 0', ].join(' \\; '); // ssh runs its trailing args through the remote login shell, so the entire @@ -743,6 +767,22 @@ export function buildRemoteLaunchCommand(options: { return sshParts.join(' '); } +/** + * Build the SSH command that kills the durable remote tmux session created by + * `buildRemoteLaunchCommand`. Because that session lives on a private socket + * (`-L codeman-remote`) under a stable name, killing the LOCAL ssh wrapper alone + * would orphan the remote agent forever (invisible to Codeman, still burning plan + * quota). This is fired best-effort on session kill; the shared connection args + * carry the default `-o ConnectTimeout=10` so an unreachable host fails fast. + */ +export function buildRemoteKillCommand(options: { remote: SessionRemote; sessionId: string }): string { + const { remote, sessionId } = options; + const remoteName = remoteTmuxSessionName(sessionId); + const killCmd = `tmux -L ${REMOTE_TMUX_SOCKET} kill-session -t ${shellescape(remoteName)}`; + const [ssh, ...connectionArgs] = buildSshConnectionArgs(remote); + return [ssh, ...connectionArgs, remoteSshTarget(remote), shellescape(killCmd)].join(' '); +} + /** * Set sensitive environment variables on a tmux session via setenv. * These are inherited by panes but not visible in ps output or tmux history. @@ -1635,6 +1675,20 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { } } + // Strategy 3b: Remote sessions run a DURABLE tmux server on the remote host + // (survives ssh drops), so killing only the local ssh wrapper above would + // orphan the remote agent forever. Fire a best-effort `ssh … tmux kill-session` + // — fire-and-forget so it NEVER blocks or throws the local kill (bounded by the + // shared ConnectTimeout on an unreachable host). + if (session.remote) { + try { + const remoteKillCmd = buildRemoteKillCommand({ remote: session.remote, sessionId }); + exec(remoteKillCmd, { timeout: EXEC_TIMEOUT_MS }, () => {}); + } catch { + // Best-effort — a failure here must not affect the local kill result. + } + } + // Strategy 4: Direct kill by PID as final fallback if (this.isProcessAlive(currentPid)) { try { diff --git a/src/web/public/index.html b/src/web/public/index.html index 7a3974c6..e6347716 100644 --- a/src/web/public/index.html +++ b/src/web/public/index.html @@ -1621,6 +1621,7 @@