From 2b89f355999505fc520ba29fe6534c4d8e382f05 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Wed, 5 Aug 2026 01:40:37 +0200 Subject: [PATCH] fix(shell,remote-ssh): allowlist the login flags, and keep only CRASHED remote panes Follow-up to #209 and #210. Both land a real fix (a pane that is a login shell picks up /etc/profile and the per-user PATH entries an ssh remote command never sees, which is what was failing agent CLIs with exit 127). Three corrections: 1. `-i -l` is no longer hardcoded onto the resolved shell. That path ultimately comes from the passwd entry, which is user data and can name anything, and a shell that rejects an unknown flag exits on the spot: nushell, elvish and xonsh take neither flag, so a user with one of those in passwd would have gotten a dead pane on arrival, which is exactly the #208 failure #209 builds on top of. loginShellArgs() applies them only to the POSIX-family shells verified to accept both, and a test really launches every allowlisted shell present on the machine rather than trusting the set. csh/tcsh are excluded deliberately: tcsh honors -l only when it is the ONLY flag. 2. `remain-on-exit on` -> `failed`, moved LAST in the tmux command chain. `on` keeps the pane after a CLEAN exit too, so typing `exit` in a remote shell stranded a dead pane, the session outlived it, and the next launch's `-A` reattached to that corpse: "Pane is dead (status 0)" instead of a shell, permanently, on the DEFAULT path. Verified against a real tmux, as was the fix: `failed` tears the session down on status 0 and keeps the pane on 127 with the "command not found" still on screen, which is the case #210 wanted. It is last because tmux aborts the remaining commands of a `\;` sequence once one errors (also verified) and `failed` needs tmux >= 3.2 on the REMOTE host; leading, a rejection there would have silently dropped status/mouse/prefix/ escape-time/window-size along with it. 3. `$SHELL` -> `"${SHELL:-/bin/sh}"`, via one shared remoteLoginShellCommand() helper instead of the string being rebuilt in tmux-manager as well. Also corrects the rationale both PRs carried: a tmux pane already hands the shell a tty, so it was interactive all along ($- contains i for a bare /bin/bash in a pane) and ~/.bashrc was always being sourced. `-l` is the flag doing the work. End-to-end verified, not just unit-tested: the emitted remote pane command was run through all three quoting layers under a minimal sshd-style PATH with the CLI installed only on a login-shell PATH entry, and it resolved and launched the CLI with its arguments intact and a space-containing remote path preserved. Co-Authored-By: Claude Opus 5 (1M context) --- src/remote-hosts.ts | 43 ++++++++++++++++++++++---- src/tmux-manager.ts | 50 +++++++++++++++++++++---------- src/utils/index.ts | 2 +- src/utils/shell-resolver.ts | 33 ++++++++++++++++++++ test/antigravity-mode.test.ts | 2 +- test/remote-hosts.test.ts | 8 +++-- test/remote-ssh-options.test.ts | 7 +++-- test/shell-session-launch.test.ts | 46 ++++++++++++++++++++++++---- test/tmux-manager.test.ts | 14 +++++++-- 9 files changed, 169 insertions(+), 36 deletions(-) diff --git a/src/remote-hosts.ts b/src/remote-hosts.ts index 7bcddb44..753d45ee 100644 --- a/src/remote-hosts.ts +++ b/src/remote-hosts.ts @@ -58,6 +58,37 @@ export async function writeRemoteCases(configDir: string, cases: RemoteCase[]): await writeJsonArray(configDir, remoteCasesPath(configDir), cases); } +/** + * The remote user's login shell, defaulted and quoted. + * + * The default is belt-and-braces, not a live bug: an empty `$SHELL` would expand + * to `exec -i -l`, which the shell reads as `exec -i` — "not found", pane dead on + * arrival, the #208 failure all over again (verified: `sh -c 'exec $SHELL -i -l'` + * with SHELL unset prints `exec: -i: not found`). In practice tmux always exports + * SHELL into a pane from its own `default-shell` option, so the command as USED + * here is safe either way (also verified). The default matters because these + * strings are the seed values a per-host `commands.*` override is edited from, and + * nothing constrains where an edited one ends up running. Quoted for a shell path + * containing spaces. `/bin/sh` exists on every POSIX host. + */ +const REMOTE_LOGIN_SHELL = '"${SHELL:-/bin/sh}"'; + +/** + * Run `command` through the remote user's interactive login shell, so per-user + * PATH entries (~/.local/bin, ~/.opencode/bin, …) are resolved before the CLI name + * is looked up. ssh's remote-command execution is neither interactive nor login, + * so a bare `exec claude` sees only sshd's minimal default PATH and dies with + * "command not found" (exit 127). + * + * Shells that take neither flag (nushell, elvish, …) cannot be detected from here + * the way `loginShellArgs()` detects them locally, since the shell is whatever the + * REMOTE passwd says. A host like that is what the per-host `commands.*` override + * is for. + */ +export function remoteLoginShellCommand(command: string): string { + return `exec ${REMOTE_LOGIN_SHELL} -i -l -c ${shellescape(command)}`; +} + export function defaultRemoteCommandForMode(mode: SessionMode): string { // Agent CLIs (claude/opencode/codex/gemini/antigravity) are typically installed // under per-user paths like ~/.local/bin or ~/.opencode/bin, added to PATH only by @@ -73,15 +104,15 @@ export function defaultRemoteCommandForMode(mode: SessionMode): string { // /etc/passwd entry, so this launches their actual login shell (zsh, // fish, etc.). -i -l so it sources rc files (~/.zshrc etc.), matching // the local shell-mode launch. - shell: 'exec $SHELL -i -l', + shell: `exec ${REMOTE_LOGIN_SHELL} -i -l`, // 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 $SHELL -i -l -c ${shellescape('claude --dangerously-skip-permissions')}`, - opencode: `exec $SHELL -i -l -c ${shellescape('opencode')}`, - codex: `exec $SHELL -i -l -c ${shellescape('codex')}`, - gemini: `exec $SHELL -i -l -c ${shellescape('gemini')}`, - antigravity: `exec $SHELL -i -l -c ${shellescape('agy')}`, + claude: remoteLoginShellCommand('claude --dangerously-skip-permissions'), + opencode: remoteLoginShellCommand('opencode'), + codex: remoteLoginShellCommand('codex'), + gemini: remoteLoginShellCommand('gemini'), + antigravity: remoteLoginShellCommand('agy'), }; return commands[mode as RemoteCommandMode] || commands.shell; } diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index d1a8420c..0ba44df3 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -49,7 +49,12 @@ import { type DockerCommandMode, } from './types.js'; import { buildEffortCliArgs } from './session-cli-builder.js'; -import { buildSshConnectionArgs, defaultRemoteCommandForMode, remoteSshTarget } from './remote-hosts.js'; +import { + buildSshConnectionArgs, + defaultRemoteCommandForMode, + remoteLoginShellCommand, + remoteSshTarget, +} from './remote-hosts.js'; import { buildDockerBaseArgs, buildDockerCreateArgs, @@ -72,6 +77,7 @@ import { resolveGeminiDir, resolveAntigravityDir, resolveLocalShell, + loginShellArgs, } from './utils/index.js'; import type { TerminalMultiplexer, @@ -784,11 +790,15 @@ export function buildSpawnCommand(options: { // SERVER process's env — empty in containers and system systemd units, leaving // the pane command ending in a dangling `&&` ("syntax error: unexpected end of // file", pane dead on arrival). Resolve it in Node and quote the result. - // -i -l: interactive login shell, so the resolved shell sources the user's rc - // files (~/.zshrc, ~/.bashrc, etc.) instead of running as a bare non-interactive - // child of the non-interactive `bash -c` that launches the pane — without this, - // aliases, PATH additions, and tool init (zoxide, nvm, etc.) are silently dropped. - return `${shellescape(resolveLocalShell())} -i -l`; + // #209: launch it as a LOGIN shell, which is what tmux itself does for a pane + // with no `default-command`, so a Codeman shell tab matches a hand-started tmux + // one. That is what picks up /etc/profile and /etc/profile.d/* — a systemd + // --user service never sourced them, so its PATH is what every pane inherited. + // The flags come from loginShellArgs() rather than being hardcoded: they are + // appended to a path that ultimately comes from the passwd entry, and a shell + // that rejects an unknown flag exits on the spot, which is #208 all over again. + const shell = resolveLocalShell(); + return `${shellescape(shell)}${loginShellArgs(shell)}`; } /** @@ -867,7 +877,7 @@ export function buildRemoteLaunchCommand(options: { const modeCommand = override ? override : mode === 'claude' - ? `exec $SHELL -i -l -c ${shellescape(`claude${buildClaudePermissionFlags(claudeMode, allowedTools)}`)}` + ? remoteLoginShellCommand(`claude${buildClaudePermissionFlags(claudeMode, allowedTools)}`) : defaultRemoteCommandForMode(mode); const remoteName = remoteTmuxSessionName(sessionId); @@ -882,14 +892,6 @@ export function buildRemoteLaunchCommand(options: { // shared remote tmux server's other sessions keep their own prefix/mouse. const tmuxInvocation = [ `tmux -L ${REMOTE_TMUX_SOCKET} new-session -A -s ${remoteName} -c ${shellescape(remote.remotePath)} ${shellescape(paneCommand)}`, - // Without this, tmux's default behavior destroys the pane -> window -> - // session (and, being the only session, the whole remote server) the - // instant paneCommand exits for ANY reason -- even something transient. - // That tears down the local ssh -t attach along with it (dead pane, - // status whatever ssh reported), and reconnect's `-A` then creates a - // fresh session with no trace of what actually happened. Set first, so - // it applies as early as possible after the pane starts. - `set -t ${remoteName} remain-on-exit on`, `set -t ${remoteName} status off`, `set -t ${remoteName} mouse off`, `set -t ${remoteName} prefix C-q`, @@ -901,6 +903,24 @@ export function buildRemoteLaunchCommand(options: { // Per-session scoped (`set -t `, matching #145's hardening) so a shared // remote tmux server's other sessions keep their own sizing behavior. `set -t ${remoteName} window-size latest`, + // #210: keep a CRASHED pane so the failure is still on screen. Without this, + // tmux destroys the pane -> window -> session (and, being the only session, + // the whole remote server) the instant the pane command exits, which tears the + // local `ssh -t` attach down with it; reconnect's `-A` then builds a fresh + // session and the cycle can repeat as a flap loop with no evidence surviving. + // That is how the exit-127 PATH bug fixed above stayed invisible. + // + // `failed`, NOT `on`: `on` keeps the pane on a CLEAN exit too, so typing + // `exit` in a remote shell leaves a dead pane behind, the session outlives it, + // and the next launch's `-A` reattaches to that corpse ("Pane is dead (status + // 0)") instead of starting a shell — verified against a real tmux. `failed` + // keeps the pane only on a non-zero exit, which is exactly the diagnostic case. + // + // LAST in the chain on purpose: tmux aborts the remaining commands of a `\;` + // sequence once one errors (also verified), and `failed` needs tmux >= 3.2 on + // the REMOTE host. Trailing, a rejection costs only this option; leading, it + // would silently drop status/mouse/prefix/escape-time/window-size with it. + `set -t ${remoteName} remain-on-exit failed`, ].join(' \\; '); // ssh runs its trailing args through the remote login shell, so the entire diff --git a/src/utils/index.ts b/src/utils/index.ts index 98a34dd0..4c813f42 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -26,7 +26,7 @@ export { isSafePushEndpoint } from './push-endpoint-validation.js'; export { stringSimilarity, fuzzyPhraseMatch, todoContentHash } from './string-similarity.js'; export { assertNever } from './type-safety.js'; export { wrapWithNice } from './nice-wrapper.js'; -export { resolveLocalShell } from './shell-resolver.js'; +export { resolveLocalShell, loginShellArgs } from './shell-resolver.js'; export { findClaudeDir, getAugmentedPath, getClaudeCliVersion, getClaudeBinaryPath } from './claude-cli-resolver.js'; export { spawnPtyWithHelperRepair } from './node-pty-repair.js'; export { resolveOpenCodeDir } from './opencode-cli-resolver.js'; diff --git a/src/utils/shell-resolver.ts b/src/utils/shell-resolver.ts index 8d20fde6..36d78c61 100644 --- a/src/utils/shell-resolver.ts +++ b/src/utils/shell-resolver.ts @@ -33,6 +33,18 @@ const FALLBACK_SHELLS = ['/bin/bash', '/bin/zsh', '/bin/sh']; */ const NON_INTERACTIVE_SHELLS = new Set(['nologin', 'false', 'true', 'sync']); +/** + * Shells verified to accept BOTH `-i` and `-l`. Deliberately an allowlist, not a + * blocklist: a shell that rejects an unknown flag exits immediately, which is the + * dead-pane-on-arrival failure this module exists to prevent (#208). The passwd + * entry is user data and can name anything — nushell, elvish, and xonsh all take + * neither flag in this form, so they get a bare launch instead of a dead tab. + * + * csh/tcsh are excluded on purpose: tcsh honors `-l` only when it is the ONLY + * flag, so `-i -l` would silently not be a login shell there anyway. + */ +const LOGIN_FLAG_SHELLS = new Set(['sh', 'bash', 'dash', 'ash', 'zsh', 'ksh', 'ksh93', 'mksh', 'pdksh', 'fish']); + function isUsableShell(candidate: string): boolean { if (!candidate.startsWith('/')) return false; const base = candidate.slice(candidate.lastIndexOf('/') + 1); @@ -76,3 +88,24 @@ export function resolveLocalShell(): string { // best guess and is far better than emitting an empty command. return '/bin/sh'; } + +/** + * Flags that make `shellPath` a login shell, or `''` when it takes none we trust. + * + * A tmux pane already hands the shell a tty, so it is interactive with or without + * `-i` (verified: `$-` contains `i` for a bare `/bin/bash` in a pane, which is why + * `~/.bashrc` has always been sourced). The flag that actually changes anything is + * `-l`: it makes the pane a LOGIN shell, matching what tmux itself does when it + * spawns a pane with no `default-command`, and picking up the `/etc/profile` and + * `/etc/profile.d/*` PATH entries that a systemd-spawned server never sourced. + * + * `-i` is kept alongside it because for bash the two select different files — + * login reads `~/.bash_profile`, interactive-non-login reads `~/.bashrc` — and + * asking for both is the closest thing to "the shell the user actually gets". + * + * Returns a string ready to append to an already-escaped shell path. + */ +export function loginShellArgs(shellPath: string): string { + const base = shellPath.slice(shellPath.lastIndexOf('/') + 1); + return LOGIN_FLAG_SHELLS.has(base) ? ' -i -l' : ''; +} diff --git a/test/antigravity-mode.test.ts b/test/antigravity-mode.test.ts index 43f288d3..dca7e669 100644 --- a/test/antigravity-mode.test.ts +++ b/test/antigravity-mode.test.ts @@ -120,6 +120,6 @@ describe('Antigravity mode gates', () => { expect(defaultDockerCommandForMode('antigravity')).toBe('exec agy'); // Routed through an interactive login shell so per-user PATH entries resolve — // same fix as the other remote agent CLIs (see defaultRemoteCommandForMode). - expect(defaultRemoteCommandForMode('antigravity')).toBe("exec $SHELL -i -l -c 'agy'"); + expect(defaultRemoteCommandForMode('antigravity')).toBe('exec "${SHELL:-/bin/sh}" -i -l -c \'agy\''); }); }); diff --git a/test/remote-hosts.test.ts b/test/remote-hosts.test.ts index af50989d..0934ab89 100644 --- a/test/remote-hosts.test.ts +++ b/test/remote-hosts.test.ts @@ -55,13 +55,15 @@ describe('remote-hosts domain', () => { }); it('returns safe mode defaults and remote display values', () => { - expect(defaultRemoteCommandForMode('shell')).toBe('exec $SHELL -i -l'); + expect(defaultRemoteCommandForMode('shell')).toBe('exec "${SHELL:-/bin/sh}" -i -l'); // Routed through an interactive login shell so per-user PATH entries (e.g. // ~/.local/bin, ~/.opencode/bin) resolve — a bare `exec codex` sees only // sshd's minimal default PATH and fails with "command not found". - expect(defaultRemoteCommandForMode('codex')).toBe("exec $SHELL -i -l -c 'codex'"); + expect(defaultRemoteCommandForMode('codex')).toBe('exec "${SHELL:-/bin/sh}" -i -l -c \'codex\''); // Mirrors the local claude default so the remote agent runs non-interactively. - expect(defaultRemoteCommandForMode('claude')).toBe("exec $SHELL -i -l -c 'claude --dangerously-skip-permissions'"); + expect(defaultRemoteCommandForMode('claude')).toBe( + 'exec "${SHELL:-/bin/sh}" -i -l -c \'claude --dangerously-skip-permissions\'' + ); expect(remoteSshTarget({ id: 'h1', label: 'H1', host: 'box.local', username: 'aamer' })).toBe('aamer@box.local'); expect(remoteDisplayPath({ username: 'aamer', host: 'box.local', path: '/opt/work' })).toBe( 'aamer@box.local:/opt/work' diff --git a/test/remote-ssh-options.test.ts b/test/remote-ssh-options.test.ts index 978b1233..f13fc8b4 100644 --- a/test/remote-ssh-options.test.ts +++ b/test/remote-ssh-options.test.ts @@ -150,16 +150,19 @@ describe('COD-107 buildRemoteLaunchCommand — threads connection args', () => { const sh = (s: string) => "'" + s.replace(/'/g, "'\\''") + "'"; const remoteName = `codeman-ssh-${SESSION_ID.slice(0, 8)}`; const path = sh('/home/ubuntu/work'); - const paneCommand = `cd ${path} && exec $SHELL -i -l`; + const paneCommand = `cd ${path} && exec "\${SHELL:-/bin/sh}" -i -l`; const tmuxInvocation = [ `tmux -L codeman-remote new-session -A -s ${remoteName} -c ${path} ${sh(paneCommand)}`, - `set -t ${remoteName} remain-on-exit on`, `set -t ${remoteName} status off`, `set -t ${remoteName} mouse off`, `set -t ${remoteName} prefix C-q`, 'set -s escape-time 0', // COD-106 — shared/collaborative sizing, per-session scoped (never -g). `set -t ${remoteName} window-size latest`, + // #210 — keep a CRASHED pane for diagnosis. `failed` (not `on`, which would + // also strand a pane after a clean `exit`), and LAST because tmux aborts the + // remaining commands of a `\;` chain on error and `failed` needs tmux >= 3.2. + `set -t ${remoteName} remain-on-exit failed`, ].join(' \\; '); // Connection args (with the default -o ConnectTimeout=10) sit after -t. const expected = `ssh -o BatchMode=yes -t -o ConnectTimeout=10 ${remoteSshTarget(baseRemote)} ${sh(tmuxInvocation)}`; diff --git a/test/shell-session-launch.test.ts b/test/shell-session-launch.test.ts index 078ad48a..89e1acc0 100644 --- a/test/shell-session-launch.test.ts +++ b/test/shell-session-launch.test.ts @@ -17,7 +17,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { execFileSync } from 'node:child_process'; import { buildSpawnCommand } from '../src/tmux-manager.js'; -import { resolveLocalShell } from '../src/utils/shell-resolver.js'; +import { loginShellArgs, resolveLocalShell } from '../src/utils/shell-resolver.js'; describe('resolveLocalShell', () => { const originalShell = process.env.SHELL; @@ -70,6 +70,40 @@ describe('resolveLocalShell', () => { }); }); +describe('loginShellArgs (#209 login flags, allowlisted)', () => { + it('asks for a login shell on the POSIX-family shells that accept the flags', () => { + for (const shell of ['/bin/sh', '/bin/bash', '/bin/dash', '/usr/bin/zsh', '/usr/local/bin/fish', '/bin/ksh']) { + expect(loginShellArgs(shell)).toBe(' -i -l'); + } + }); + + it('adds nothing for shells that take neither flag, so the pane cannot die on arrival', () => { + // The shell path can come from the passwd entry, which is user data and can + // name anything. A shell that rejects an unknown flag exits immediately — + // indistinguishable from the #208 dead-pane-on-arrival this module prevents. + // csh/tcsh are here too: tcsh honors -l only when it is the ONLY flag. + for (const shell of ['/usr/bin/nu', '/usr/bin/elvish', '/usr/bin/xonsh', '/bin/tcsh', '/bin/csh']) { + expect(loginShellArgs(shell)).toBe(''); + } + }); + + it('really launches for every allowlisted shell present on this machine', () => { + // The whole point of the allowlist is that the flags are ACCEPTED, so prove it + // against the real binaries rather than trusting the set. + for (const shell of ['/bin/sh', '/bin/bash', '/bin/dash', '/usr/bin/zsh', '/bin/ksh']) { + let exists = true; + try { + execFileSync('/bin/sh', ['-c', `test -x ${shell}`]); + } catch { + exists = false; + } + if (!exists) continue; + const out = execFileSync('/bin/sh', ['-c', `${shell} -i -l -c 'echo ok' 2>/dev/null`], { encoding: 'utf8' }); + expect(out).toContain('ok'); + } + }); +}); + describe('shell-mode spawn command (issue #208)', () => { const originalShell = process.env.SHELL; @@ -88,10 +122,12 @@ describe('shell-mode spawn command (issue #208)', () => { expect(cmd.trim()).not.toBe(''); }); - it('launches an interactive login shell, so rc files (~/.zshrc, ~/.bashrc, etc.) are sourced', () => { - // Without -i -l, the resolved shell runs as a bare non-interactive child of - // the non-interactive `bash -c` that launches the pane, silently dropping - // aliases, PATH additions, and tool init (zoxide, nvm, etc.). + it('launches a LOGIN shell, matching what tmux does for a pane with no default-command', () => { + // A tmux pane already hands the shell a tty, so it is interactive either way + // (`$-` contains `i` for a bare /bin/bash in a pane, which is why ~/.bashrc has + // always been sourced). `-l` is the flag that changes anything: it is what + // picks up /etc/profile and /etc/profile.d/*, which a systemd --user service + // never sourced, so its minimal PATH is what every pane used to inherit. const cmd = buildSpawnCommand({ mode: 'shell', sessionId: 'abc123de-0000-0000-0000-000000000000' }); expect(cmd.trim().endsWith('-i -l')).toBe(true); }); diff --git a/test/tmux-manager.test.ts b/test/tmux-manager.test.ts index 5cb43f21..38745e21 100644 --- a/test/tmux-manager.test.ts +++ b/test/tmux-manager.test.ts @@ -146,8 +146,16 @@ describe('TmuxManager (unit)', () => { sessionId: 'abc123def456', }); - expect(command).toContain('exec $SHELL -i -l'); - expect(command).toContain('remain-on-exit on'); + expect(command).toContain('exec "${SHELL:-/bin/sh}" -i -l'); + // `failed`, not `on`: `on` also keeps the pane after a CLEAN exit, so typing + // `exit` in a remote shell strands a dead pane that the next launch's `-A` + // reattaches to instead of starting a shell. + expect(command).toContain('remain-on-exit failed'); + expect(command).not.toContain('remain-on-exit on'); + // Last in the chain: tmux aborts the rest of a `\;` sequence after an error, + // and `failed` needs tmux >= 3.2 on the REMOTE host. Trailing, a rejection + // costs only this option instead of every setting after it. + expect(command.trimEnd().endsWith("remain-on-exit failed'")).toBe(true); }); it('defaults claude to a non-interactive launch (--dangerously-skip-permissions)', () => { @@ -161,7 +169,7 @@ describe('TmuxManager (unit)', () => { // interactive nor login, so a bare `exec claude` fails with "command not found". // The inner quoting is escaped twice over (once per shellescape() layer), so // assert on the unescaped substrings rather than the literal quoted form. - expect(command).toContain('exec $SHELL -i -l -c'); + expect(command).toContain('exec "${SHELL:-/bin/sh}" -i -l -c'); expect(command).toContain('claude --dangerously-skip-permissions'); }); });