diff --git a/.changeset/remote-launch-expansion.md b/.changeset/remote-launch-expansion.md new file mode 100644 index 00000000..f7f1d5ea --- /dev/null +++ b/.changeset/remote-launch-expansion.md @@ -0,0 +1,5 @@ +--- +"aicodeman": patch +--- + +Remote sessions: new sessions no longer die with `tmux: command not found` (exit 127) when the remote user's home differs from the Codeman host's. The launch line reached the local shell in double quotes, so `$HOME` and `$PATH` in the remote PATH bootstrap expanded on the Codeman host before ssh ran. Launches now reach the far end verbatim, which also means a remote session starts in the remote user's own shell rather than the Codeman host's, and a Codeman host whose `$SHELL` path does not exist on the remote no longer kills the pane. Codeman's own pane command now runs through `/bin/sh` so fish hosts keep working, while a per-host `commands.*` override still runs in the remote user's shell. Behaviour change: in a `commands.*` override, `$` now expands on the far end (remote host or container) instead of on the Codeman host, and a bare `$VAR` in a Docker `extraCreateArgs`/`extraExecArgs` value is passed to `docker` literally, so write the value out. Thanks to @Randalix for the report and the fix. diff --git a/docs/remote-sessions.md b/docs/remote-sessions.md index 1c36a9ff..95308d22 100644 --- a/docs/remote-sessions.md +++ b/docs/remote-sessions.md @@ -133,6 +133,24 @@ Key points: **login shell**, not the agent. - The **whole tmux invocation is a single shell-quoted ssh argument**, and the pane command is independently quoted, so a `remotePath` with spaces is safe. +- **That argument reaches the far end verbatim** (#760): the launch goes to the + local `bash -c` in single quotes, so `$HOME`, `$PATH` and `$SHELL` are the + remote user's. In double quotes the local `sh` expanded them first, which sent + this host's home and PATH to the remote. ⚠️ Every `$` in a `commands.*` override + now expands on the far end too. A Docker launch is verbatim as well, so a bare + `$VAR` in `extraCreateArgs`/`extraExecArgs` reaches `docker` as those literal + characters: write the value out. +- **tmux picks the shell by argument count** (measured on 3.4): one argument runs + through the remote `default-shell`, two or more are exec'd directly. Codeman's own + pane command is POSIX, so it goes to `/bin/sh -c` as separate arguments, and a fish + login shell (or a `default-shell` set in the remote `~/.tmux.conf`) never parses it: + fish refuses `${SHELL:-/bin/sh}` with status 127, which is a dead pane. A `commands.*` + override stays one argument on purpose, since it is written for the user's own shell + (`source` and `[[` work in bash and zsh and die under a dash `/bin/sh`). +- **Login shells**: POSIX shells and fish work. tcsh refuses the `-i -l` pair and needs + a `commands.*` override per mode (`exec claude`). nushell and elvish cannot run the + PATH bootstrap at all, so no override helps. A `remotePath` holding an apostrophe does + not start under fish. - Connection options come from the **same `buildSshConnectionArgs(remote)`** as the prereq probe; `-t` is inserted right after `ssh -o BatchMode=yes`, preserving historical token order. diff --git a/src/docker-hosts.ts b/src/docker-hosts.ts index f2ce722d..b00653cb 100644 --- a/src/docker-hosts.ts +++ b/src/docker-hosts.ts @@ -392,7 +392,7 @@ export function isAdoptedContainer(docker: Pick): boolea /** * POSIX single-quote shell-escaping (end-quote, escaped-quote, restart-quote). * Mirror of the helper in remote-hosts.ts / tmux-manager.ts. Every dynamic value - * interpolated into the outer `bash -c "..."` launch layer is escaped through + * interpolated into the `bash -c` launch layer is escaped through * this so a path with spaces stays a single shell token. Operator-entered fields * are ALSO schema-rejected for `$`/backtick (NO_SHELL_META) as defense in depth. */ diff --git a/src/remote-hosts.ts b/src/remote-hosts.ts index c443945e..7a8afa7e 100644 --- a/src/remote-hosts.ts +++ b/src/remote-hosts.ts @@ -66,10 +66,11 @@ export async function writeRemoteCases(configDir: string, cases: RemoteCase[]): * 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. + * here is safe either way (also verified). Quoted for a shell path containing + * spaces. `/bin/sh` exists on every POSIX host. + * + * ⚠️ This is POSIX text. fish refuses `${…}` (status 127), so a launch hands it to + * `/bin/sh -c`, never to the remote default-shell (`buildRemoteLaunchCommand`). */ const REMOTE_LOGIN_SHELL = '"${SHELL:-/bin/sh}"'; @@ -80,10 +81,12 @@ const REMOTE_LOGIN_SHELL = '"${SHELL:-/bin/sh}"'; * 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. + * Shells that take neither flag 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 + * (tcsh, which rejects `-l` combined with `-i`). It only goes so far: the PATH + * bootstrap below runs in the login shell first, so a shell that cannot parse + * `export` at all (nushell, elvish) cannot be rescued by an override. */ export function remoteLoginShellCommand(command: string): string { return `exec ${REMOTE_LOGIN_SHELL} -i -l -c ${shellescape(command)}`; @@ -143,8 +146,9 @@ function overlayCliCommand(mode: SessionMode, location: 'remote' | 'docker'): st * The per-host `commands.*` override remains the escape hatch for every mode. */ export function defaultRemoteCommandForMode(mode: SessionMode): string { - // $SHELL, not a hardcoded bash: sshd sets it from the remote user's /etc/passwd entry, so - // this launches their actual login shell (zsh, fish, …). `-i -l` so it sources rc files, + // $SHELL, not a hardcoded bash: sshd sets it from the remote user's /etc/passwd entry, and + // tmux exports its default-shell as SHELL into the pane, so this launches their actual + // login shell, or the default-shell their tmux.conf picks (zsh, fish, …). `-i -l` so it sources rc files, // matching the local shell-mode launch. Not templatable as overlay data: the shell is // whatever the REMOTE passwd says, which is why `shell` is the one arm still written here. const shellCommand = `exec ${REMOTE_LOGIN_SHELL} -i -l`; diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index f596f9b2..d7546c59 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -1303,10 +1303,12 @@ export function remoteTmuxLocation(remote: SessionRemote, sessionId: string): { * - 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 - * shell-quoted inside it too. This keeps escaping correct through every layer - * even when the remote path contains spaces. + * runs it), so it is shell-quoted as one unit. The `cd && exec` command is the + * pane command: Codeman's own text goes to `/bin/sh -c` as separate tmux + * arguments, and a per-host `commands.*` override stays one argument, which + * tmux hands to the remote default-shell (see the comment at `paneCommand`). + * Each is shell-quoted, so escaping stays correct through every layer even + * when the remote path contains spaces. */ export function buildRemoteLaunchCommand(options: { mode: SessionMode; @@ -1389,17 +1391,25 @@ export function buildRemoteLaunchCommand(options: { } const remoteName = remoteTmuxSessionName(sessionId); - // Innermost: the command tmux runs in the new pane. Run via `/bin/sh -c` by - // tmux, so the path needs shell-quoting here. `exec` replaces the shell with - // the CLI so the pane PID is the agent itself. + // Innermost: the command tmux runs in the new pane. `exec` replaces the shell + // with the CLI so the pane PID is the agent itself. + // + // tmux runs a ONE-argument command through the remote default-shell, which is + // the user's login shell, and bypasses it for two or more arguments (both + // measured on tmux 3.4). Codeman's own text is POSIX (`"${SHELL:-/bin/sh}"` is + // a parse error in fish, status 127, a dead pane on every new session), so it + // goes to `/bin/sh -c` as separate arguments and no login shell has to parse it. + // An override is the user's own text for THEIR shell: it stays one argument, + // because `source` and `[[` work in bash and zsh and die in a dash `/bin/sh`. const paneCommand = `cd ${shellescape(remote.remotePath)} && ${modeCommand}`; + const paneArgv = `${override ? '' : '/bin/sh -c '}${shellescape(paneCommand)}`; // The tmux command line, with `\;` separating commands so the config `set`s // 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 ${REMOTE_TMUX_SOCKET} new-session -A -s ${remoteName} -c ${shellescape(remote.remotePath)} ${shellescape(paneCommand)}`, + `tmux -L ${REMOTE_TMUX_SOCKET} new-session -A -s ${remoteName} -c ${shellescape(remote.remotePath)} ${paneArgv}`, `set -t ${remoteName} status off`, `set -t ${remoteName} mouse off`, `set -t ${remoteName} prefix ${NESTED_TMUX_PREFIX}`, @@ -1574,7 +1584,7 @@ export interface DockerLaunchOptions { * Build the ONE `bash -c` launch string for a docker session: image-check -> * ensure (inspect-or-create) -> start -> `exec docker exec -it` into the durable * in-container tmux (resume-aware). PURE and unit-testable. The escaping survives - * four layers: outer `bash -c "…"` (JSON.stringify at respawn-pane) -> the joined + * four layers: outer `bash -c '…'` (`bashCArg()` at respawn-pane) -> the joined * command -> `docker exec … sh -lc ''` -> tmux `''`. */ export function buildDockerLaunchCommand(opts: DockerLaunchOptions): string { @@ -1602,8 +1612,8 @@ export function buildDockerLaunchCommand(opts: DockerLaunchOptions): string { } else if (resumeSessionId) { modeCommand = appendResumeFlag(modeCommand, mode, resumeSessionId); } - // Run by tmux via /bin/sh -c, so the path is shell-quoted here. `exec` makes the - // pane PID the agent itself. + // Run by tmux through the container's default-shell, so the path is shell-quoted + // here. `exec` makes the pane PID the agent itself. const paneCommand = `cd ${workdir} && ${modeCommand}`; // `setenv -g` primes the session id so reattaches / newly-created panes inherit @@ -1665,11 +1675,10 @@ export function buildDockerLaunchCommand(opts: DockerLaunchOptions): string { const ensure = adopted ? `${base} inspect ${name} >/dev/null 2>&1 || { echo ${notFoundMsg}; exit 1; }` : `${base} inspect ${name} >/dev/null 2>&1 || ${createCommand}`; - // ⚠️ No double quotes and no `$(…)` in the ADOPTED arms. This whole chain is - // embedded in an outer `bash -c "…"`, so an unescaped `"` closes that string early, - // the rest is re-tokenized, and tmux fails to exec with a bare `execvp(3) failed`. - // A `grep -qx` pipeline reads the same answer using only the single-quoted form - // every other line in this builder already uses. + // A `grep -qx` pipeline, in the single-quoted form every other line in this builder + // uses. This chain reaches `bash -c` verbatim now (`bashCArg()`), but it went through + // a double-quoted layer for a long time, where a `"` closed that string early and tmux + // failed with a bare `execvp(3) failed`, so keep it free of double quotes and `$(…)`. const start = adopted ? `${base} inspect -f ${shellescape('{{.State.Running}}')} ${name} 2>/dev/null | grep -qx true || { echo ${notRunningMsg}; exit 1; }` : `${base} start ${name} >/dev/null 2>&1 || { echo ${startFailMsg}; exit 1; }`; @@ -4672,6 +4681,10 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { * a remote user with another home found no tmux and every new remote pane exited 127. * `${SHELL:-/bin/sh}` in the remote launch was the local shell for the same reason. * Local commands keep their double-quoted form. + * + * ⚠️ So nothing in a verbatim launch is expanded on this host any more, the docker client's + * own arguments included: a bare `$VAR` in `extraCreateArgs`/`extraExecArgs` reaches docker + * as those literal characters. */ function bashCArg(launchCmd: string, verbatim: boolean): string { return verbatim ? shellescape(launchCmd) : JSON.stringify(launchCmd); diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 5c72e74a..10ef069a 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -878,11 +878,11 @@ const RemoteCommandOverridesSchema = z const NO_SHELL_INJECTION = /^[^\n\r\0`]*$/; const noCommandSubstitution = (s: string) => !s.includes('$('); -// `remotePath`/`identityFile` are shell-escaped, then the whole launch command is -// embedded via `JSON.stringify(...)` inside `bash -c "..."` (tmux-manager). That -// outer DOUBLE-quote layer re-exposes `$(...)`, backticks, and `$VAR` even though -// the inner value is single-quoted — so a `$(cmd)` in the path would run LOCALLY at -// launch. Reject `$` and backtick (and newline/CR/NUL) entirely at the boundary. +// `remotePath`/`identityFile` are shell-escaped, and a remote launch reaches `bash -c` +// single-quoted (`bashCArg()` in tmux-manager). They are also pasted into strings the +// far end's shells parse, and hosts stored before a launch-quoting change are read with +// no schema, so `$` and backtick (and newline/CR/NUL) stay rejected at the boundary as +// defense in depth, whatever the current quoting does with them. const NO_SHELL_META = /^[^\n\r\0`$]*$/; export const RemoteHostSchema = z.object({ @@ -982,10 +982,10 @@ export const RemoteCaseLinkSchema = z.object({ // // Docker mode is a location overlay on cases (see docs/docker-cases-plan.md), // the analog of the remote-SSH schemas above. `image`, `hostWorkspacePath`, -// `containerWorkdir`, and `container` all reach the outer `bash -c "..."` launch -// layer, so they carry NO_SHELL_META (rejects `$`/backtick that survive the -// double-quote layer) exactly like remotePath/identityFile. `--privileged` and -// any docker-socket mount are structurally unrepresentable (never accepted). +// `containerWorkdir`, and `container` all reach the `bash -c` launch layer, so they +// carry NO_SHELL_META (rejects `$`/backtick) exactly like remotePath/identityFile. +// `--privileged` and any docker-socket mount are structurally unrepresentable +// (never accepted). const DockerResourceLimitsSchema = z .object({ diff --git a/test/remote-pane-command-shell.test.ts b/test/remote-pane-command-shell.test.ts new file mode 100644 index 00000000..a44f2223 --- /dev/null +++ b/test/remote-pane-command-shell.test.ts @@ -0,0 +1,81 @@ +/** + * @fileoverview Which shell parses a remote pane command (#760). + * + * tmux runs a ONE-argument command through the remote default-shell, the user's login shell, and + * bypasses it for two or more arguments. Codeman's own text carries `"${SHELL:-/bin/sh}"`, a parse + * error in fish that kills the pane with status 127, so it must reach `/bin/sh -c` as separate + * arguments. A per-host `commands.*` override is the user's text for their own shell: `source` and + * `[[` work in bash and zsh and die under a dash `/bin/sh`, so it must stay one argument. + * + * CI has no fish, so this reads the argv tmux would receive instead: the real builder's string goes + * through a real sh with `ssh` and `tmux` defined as functions that report their arguments. + */ +import { execFileSync } from 'node:child_process'; +import { describe, expect, it } from 'vitest'; +import { listClis } from '../src/config/cli-registry/registry.js'; +import { buildRemoteLaunchCommand } from '../src/tmux-manager.js'; +import type { SessionMode, SessionRemote } from '../src/types.js'; + +const SESSION_ID = 'abcd1234-0000-0000-0000-000000000000'; +const remote: SessionRemote = { + hostId: 'gpu', + label: 'GPU', + host: '10.0.0.42', + username: 'ubuntu', + remotePath: '/home/ubuntu/it is work', +}; + +/** The one argument ssh would send, as the login shell on the far end receives it. */ +function sshArgument(launch: string): string { + return execFileSync('/bin/sh', ['-c', `ssh() { for a; do l="$a"; done; printf '%s' "$l"; }; ${launch}`], { + encoding: 'utf8', + }); +} + +/** What follows `new-session … -c `, up to the end of that tmux command. */ +function paneArgv(launch: string): string[] { + const argv = execFileSync('/bin/sh', ['-c', `tmux() { printf '%s\\0' "$@"; }; ${sshArgument(launch)}`], { + encoding: 'utf8', + }) + .split('\0') + .slice(0, -1); + const head = argv.slice(0, argv.indexOf(';')); + return head.slice(head.indexOf('-c', head.indexOf('new-session')) + 2); +} + +const launches: Array<[string, Parameters[0]]> = [ + ...listClis().map((cli): [string, Parameters[0]] => [ + cli.id, + { mode: cli.id as SessionMode, remote, sessionId: SESSION_ID }, + ]), + ['shell', { mode: 'shell', remote, sessionId: SESSION_ID }], + ['claude resume', { mode: 'claude', remote, sessionId: SESSION_ID, resumeSessionId: SESSION_ID }], + ['omp resume', { mode: 'omp', remote, sessionId: SESSION_ID, resumeSessionId: 'abc123' }], +]; + +describe('the shell that parses a remote pane command', () => { + it.each(launches)('%s: Codeman text goes to /bin/sh, never the default-shell', (_name, options) => { + const argv = paneArgv(buildRemoteLaunchCommand(options)); + expect(argv.slice(0, 2)).toEqual(['/bin/sh', '-c']); + expect(argv).toHaveLength(3); + expect(argv[2]).toContain(`cd '/home/ubuntu/it is work' && `); + }); + + it('leaves an override as one argument, parsed by the remote default-shell', () => { + const override = 'source ~/env.sh && [[ -x ~/.local/bin/codx ]] && exec codx'; + const argv = paneArgv( + buildRemoteLaunchCommand({ + mode: 'codex', + remote: { ...remote, commands: { codex: override } }, + sessionId: SESSION_ID, + }) + ); + expect(argv).toEqual([`cd '/home/ubuntu/it is work' && ${override}`]); + }); + + it('keeps the rest of the tmux chain behind the longer pane command', () => { + const sent = sshArgument(buildRemoteLaunchCommand({ mode: 'shell', remote, sessionId: SESSION_ID })); + expect(sent).toContain(String.raw` \; set -t codeman-ssh-abcd1234 status off \; `); + expect(sent).toMatch(/set -t codeman-ssh-abcd1234 remain-on-exit failed$/); + }); +}); diff --git a/test/remote-ssh-options.test.ts b/test/remote-ssh-options.test.ts index ad3bee72..5acdbaa8 100644 --- a/test/remote-ssh-options.test.ts +++ b/test/remote-ssh-options.test.ts @@ -159,7 +159,7 @@ describe('COD-107 buildRemoteLaunchCommand — threads connection args', () => { const path = sh('/home/ubuntu/work'); 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)}`, + `tmux -L codeman-remote new-session -A -s ${remoteName} -c ${path} /bin/sh -c ${sh(paneCommand)}`, `set -t ${remoteName} status off`, `set -t ${remoteName} mouse off`, `set -t ${remoteName} prefix C-q`, diff --git a/test/tmux-manager.test.ts b/test/tmux-manager.test.ts index 32cf39b4..0bc51700 100644 --- a/test/tmux-manager.test.ts +++ b/test/tmux-manager.test.ts @@ -939,6 +939,38 @@ describe('TmuxManager (unit)', () => { } }); + it('hands a docker override to bash with $HOME unexpanded', async () => { + const NonTestTmuxManager = await importWithTmuxCommandsEnabled(); + const nonTestManager = new NonTestTmuxManager(); + + try { + await nonTestManager.createSession({ + sessionId: 'abc12345-1234-5678-90ab-cdef12345680', + workingDir: '/w', + mode: 'shell', + docker: { + hostId: 'h', + label: 'L', + engine: 'docker', + image: 'codeman/agent:base', + containerName: 'codeman-case-x', + hostWorkspacePath: '/w', + containerWorkdir: '/w', + commands: { shell: 'echo CONTAINER_HOME=$HOME; exec sleep 30' }, + } as never, + }); + const launch = [...mockedExecSync.mock.calls, ...vi.mocked(exec).mock.calls] + .map(([cmd]) => cmd) + .find((cmd) => typeof cmd === 'string' && cmd.includes(' respawn-pane ')); + const sent = await bashArgAfterLocalShell(launch as string); + expect(sent).toContain('docker'); + expect(sent).toContain('CONTAINER_HOME=$HOME'); + expect(sent).not.toContain('local-home-xyz'); + } finally { + nonTestManager.destroy(); + } + }); + it('changes the global history default on tmux versions that cannot resize panes', async () => { const NonTestTmuxManager = await importWithTmuxCommandsEnabled(); const nonTestManager = new NonTestTmuxManager();