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-<id> 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 <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
2026-07-12 19:49:58 +02:00
parent 7efc6cd5a8
commit 5deb0d4a4c
15 changed files with 580 additions and 129 deletions
+2 -1
View File
@@ -57,7 +57,8 @@ describe('remote-hosts domain', () => {
it('returns safe mode defaults and remote display values', () => {
expect(defaultRemoteCommandForMode('shell')).toBe('exec bash -l');
expect(defaultRemoteCommandForMode('codex')).toBe('exec codex');
expect(defaultRemoteCommandForMode('claude')).toBe('exec claude');
// Mirrors the local claude default so the remote agent runs non-interactively.
expect(defaultRemoteCommandForMode('claude')).toBe('exec 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'
+31 -19
View File
@@ -51,8 +51,8 @@ const aaDesktop: SessionRemote = {
const SESSION_ID = 'cod107chk';
describe('COD-107 buildSshConnectionArgs — shared ssh connection tokens', () => {
it('always leads with -o BatchMode=yes', () => {
expect(buildSshConnectionArgs(baseRemote)).toEqual(['ssh', '-o BatchMode=yes']);
it('always leads with -o BatchMode=yes then the default -o ConnectTimeout=10', () => {
expect(buildSshConnectionArgs(baseRemote)).toEqual(['ssh', '-o BatchMode=yes', '-o ConnectTimeout=10']);
});
it('emits the full aa-desktop option set in order with escaping + %h %p intact', () => {
@@ -107,10 +107,19 @@ describe('COD-107 buildSshConnectionArgs — shared ssh connection tokens', () =
expect(args.join(' ')).toContain(`-i '${HOME}/.ssh/id_ed25519'`);
});
it('empty options ⇒ exactly the historical token set (back-compat)', () => {
// Today: ssh -o BatchMode=yes (+ -p only when set). Nothing else.
expect(buildSshConnectionArgs(baseRemote)).toEqual(['ssh', '-o BatchMode=yes']);
expect(buildSshConnectionArgs({ ...baseRemote, port: 2200 })).toEqual(['ssh', '-o BatchMode=yes', '-p 2200']);
it('empty options ⇒ BatchMode + the default ConnectTimeout (+ -p only when set)', () => {
expect(buildSshConnectionArgs(baseRemote)).toEqual(['ssh', '-o BatchMode=yes', '-o ConnectTimeout=10']);
expect(buildSshConnectionArgs({ ...baseRemote, port: 2200 })).toEqual([
'ssh',
'-o BatchMode=yes',
'-o ConnectTimeout=10',
'-p 2200',
]);
});
it('omits the default ConnectTimeout when extraSshOptions already sets it (operator wins)', () => {
const args = buildSshConnectionArgs({ ...baseRemote, extraSshOptions: ['ConnectTimeout=3'] });
expect(args.filter((a) => a.includes('ConnectTimeout'))).toEqual(["-o 'ConnectTimeout=3'"]);
});
});
@@ -124,7 +133,7 @@ describe('COD-107 buildRemoteLaunchCommand — threads connection args', () => {
expect(command).toContain("-o 'StrictHostKeyChecking=accept-new'");
expect(command).toContain('-t');
expect(command).toContain('aakht@192.168.55.170');
expect(command).toContain('tmux -L codeman new-session -A');
expect(command).toContain('tmux -L codeman-remote new-session -A');
// Connection options come BEFORE -t / the target / the tmux command.
const idxProxy = command.indexOf('ProxyCommand=');
@@ -132,32 +141,35 @@ describe('COD-107 buildRemoteLaunchCommand — threads connection args', () => {
expect(idxProxy).toBeLessThan(idxTarget);
});
it('back-compat: a remote with no advanced options is byte-identical to the historical form', () => {
it('a remote with no advanced options is byte-identical to the expected form', () => {
const command = buildRemoteLaunchCommand({ mode: 'shell', remote: baseRemote, sessionId: SESSION_ID });
// Reconstruct the historical command using the SAME nested POSIX
// single-quote escaping the production code uses, to prove byte-identity.
// Reconstruct the command using the SAME nested POSIX single-quote escaping the
// production code uses, to prove byte-identity. Session runs on the DEDICATED
// `-L codeman-remote` socket under a `codeman-ssh-` name that a remote Codeman's
// discovery ignores; set-options are scoped per-session (never `-g`).
const sh = (s: string) => "'" + s.replace(/'/g, "'\\''") + "'";
const remoteName = `codeman-${SESSION_ID.slice(0, 8)}`;
const remoteName = `codeman-ssh-${SESSION_ID.slice(0, 8)}`;
const path = sh('/home/ubuntu/work');
const paneCommand = `cd ${path} && exec bash -l`;
const tmuxInvocation = [
`tmux -L codeman new-session -A -s ${remoteName} -c ${path} ${sh(paneCommand)}`,
'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 ${remoteName} -c ${path} ${sh(paneCommand)}`,
`set -t ${remoteName} status off`,
`set -t ${remoteName} mouse off`,
`set -t ${remoteName} prefix C-q`,
'set -s escape-time 0',
].join(' \\; ');
const expected = `ssh -o BatchMode=yes -t ${remoteSshTarget(baseRemote)} ${sh(tmuxInvocation)}`;
// 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)}`;
expect(command).toBe(expected);
});
it('back-compat: port-only remote matches the historical -p placement', () => {
it('port-only remote places -p after the -t/ConnectTimeout tokens', () => {
const command = buildRemoteLaunchCommand({
mode: 'shell',
remote: { ...baseRemote, port: 2222 },
sessionId: SESSION_ID,
});
expect(command).toMatch(/^ssh -o BatchMode=yes -t -p 2222 ubuntu@10\.0\.0\.42 /);
expect(command).toMatch(/^ssh -o BatchMode=yes -t -o ConnectTimeout=10 -p 2222 ubuntu@10\.0\.0\.42 /);
});
});
+107
View File
@@ -53,9 +53,20 @@ vi.mock('../../src/hooks-config.js', () => ({
writeHooksConfig: vi.fn(async () => {}),
}));
// Stub the remote-tmux prereq probe so remote-link tests never shell out to ssh
// (readRemoteHosts/writeRemoteHosts stay real, backed by the mocked fs).
vi.mock('../../src/remote-hosts.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/remote-hosts.js')>();
return {
...actual,
checkRemoteTmuxAvailable: vi.fn(async () => ({ ok: true, tmuxPath: '/usr/bin/tmux' })),
};
});
// Import mocked modules for test control
import { existsSync, mkdirSync, readdirSync } from 'node:fs';
import fs from 'node:fs/promises';
import { checkRemoteTmuxAvailable } from '../../src/remote-hosts.js';
const mockedExistsSync = vi.mocked(existsSync);
const mockedMkdirSync = vi.mocked(mkdirSync);
@@ -63,6 +74,7 @@ const mockedReaddirSync = vi.mocked(readdirSync);
const mockedReaddir = vi.mocked(fs.readdir);
const mockedReadFile = vi.mocked(fs.readFile);
const mockedWriteFile = vi.mocked(fs.writeFile);
const mockedCheckRemoteTmux = vi.mocked(checkRemoteTmuxAvailable);
interface CaseRouteHarness {
app: FastifyInstance;
@@ -375,6 +387,101 @@ describe('case-routes', () => {
expect(deleted.statusCode).toBe(200);
expect(JSON.parse(deleted.body)).toEqual({ success: true, data: { name: 'gpu-work' } });
});
// Injection hardening: remotePath/identityFile are shell-escaped, then embedded
// via JSON.stringify() inside `bash -c "..."` — a DOUBLE-quote layer that
// re-exposes `$(...)`/backticks even inside the inner single quotes. The schema
// MUST reject those before they reach the launch command.
it('rejects an identityFile containing $(...) command substitution', async () => {
setupRemoteConfigStore();
const create = await harness.app.inject({
method: 'POST',
url: '/api/remote-hosts',
payload: {
id: 'evil-host',
label: 'evil',
host: '10.0.0.9',
username: 'ubuntu',
identityFile: '/home/u/$(touch /tmp/pwned)',
},
});
expect(create.statusCode).toBe(httpStatusForErrorCode(ApiErrorCode.INVALID_INPUT));
expect(JSON.parse(create.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.INVALID_INPUT });
});
it('rejects an identityFile containing a backtick', async () => {
setupRemoteConfigStore();
const create = await harness.app.inject({
method: 'POST',
url: '/api/remote-hosts',
payload: {
id: 'evil-host2',
label: 'evil2',
host: '10.0.0.9',
username: 'ubuntu',
identityFile: '/home/u/`touch /tmp/pwned`',
},
});
expect(create.statusCode).toBe(httpStatusForErrorCode(ApiErrorCode.INVALID_INPUT));
expect(JSON.parse(create.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.INVALID_INPUT });
});
it('rejects a remotePath containing $(...) command substitution', async () => {
setupRemoteConfigStore();
await harness.app.inject({
method: 'POST',
url: '/api/remote-hosts',
payload: { id: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu' },
});
const link = await harness.app.inject({
method: 'POST',
url: '/api/cases/remote-link',
payload: { name: 'gpu-work', hostId: 'gpu-box', remotePath: '/tmp/$(touch /tmp/pwned)' },
});
expect(link.statusCode).toBe(httpStatusForErrorCode(ApiErrorCode.INVALID_INPUT));
expect(JSON.parse(link.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.INVALID_INPUT });
});
it('rejects a remotePath containing a backtick', async () => {
setupRemoteConfigStore();
await harness.app.inject({
method: 'POST',
url: '/api/remote-hosts',
payload: { id: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu' },
});
const link = await harness.app.inject({
method: 'POST',
url: '/api/cases/remote-link',
payload: { name: 'gpu-work', hostId: 'gpu-box', remotePath: '/tmp/`touch /tmp/pwned`' },
});
expect(link.statusCode).toBe(httpStatusForErrorCode(ApiErrorCode.INVALID_INPUT));
expect(JSON.parse(link.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.INVALID_INPUT });
});
it('refuses remote-link when the remote host lacks tmux (courtesy prereq probe)', async () => {
setupRemoteConfigStore();
mockedCheckRemoteTmux.mockResolvedValueOnce({
ok: false,
error: 'remote host 10.0.0.42 needs tmux installed for durable remote sessions',
});
await harness.app.inject({
method: 'POST',
url: '/api/remote-hosts',
payload: { id: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu' },
});
const link = await harness.app.inject({
method: 'POST',
url: '/api/cases/remote-link',
payload: { name: 'gpu-work', hostId: 'gpu-box', remotePath: '/home/ubuntu/work' },
});
expect(link.statusCode).toBe(httpStatusForErrorCode(ApiErrorCode.OPERATION_FAILED));
expect(JSON.parse(link.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.OPERATION_FAILED });
});
});
// ========== POST /api/cases ==========
+64 -2
View File
@@ -29,13 +29,19 @@ vi.mock('node:child_process', async (orig) => {
});
// In-memory remote store so remote-case tests can inject hosts/cases without real JSON files.
const remoteStore = vi.hoisted(() => ({ hosts: [] as unknown[], cases: [] as unknown[] }));
const remoteStore = vi.hoisted(() => ({
hosts: [] as unknown[],
cases: [] as unknown[],
tmuxCheck: { ok: true, tmuxPath: '/usr/bin/tmux' } as { ok: boolean; tmuxPath?: string; error?: string },
}));
vi.mock('../../src/remote-hosts.js', async (orig) => {
const actual = await orig<typeof import('../../src/remote-hosts.js')>();
return {
...actual,
readRemoteHosts: vi.fn(async () => remoteStore.hosts),
readRemoteCases: vi.fn(async () => remoteStore.cases),
// Stub the remote-tmux prereq probe so quick-start never shells out to ssh.
checkRemoteTmuxAvailable: vi.fn(async () => remoteStore.tmuxCheck),
};
});
@@ -90,9 +96,10 @@ describe('session-routes', () => {
beforeEach(async () => {
harness = await createEnvelopeHarness(registerSessionRoutes);
// Reset remote store so tests start with empty hosts/cases
// Reset remote store so tests start with empty hosts/cases and a passing tmux probe
remoteStore.hosts = [];
remoteStore.cases = [];
remoteStore.tmuxCheck = { ok: true, tmuxPath: '/usr/bin/tmux' };
});
afterEach(async () => {
@@ -861,6 +868,61 @@ describe('session-routes', () => {
}
});
it('rejects a remote quick-start that carries envOverrides (inert over ssh)', async () => {
remoteStore.hosts = [{ id: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu' }];
remoteStore.cases = [{ name: 'gpu-work', type: 'remote', hostId: 'gpu-box', remotePath: '/home/ubuntu/work' }];
const res = await harness.app.inject({
method: 'POST',
url: '/api/quick-start',
payload: { caseName: 'gpu-work', mode: 'claude', envOverrides: { CLAUDE_CODE_FOO: 'bar' } },
});
expect(res.statusCode).toBe(httpStatusForErrorCode(ApiErrorCode.INVALID_INPUT));
expect(JSON.parse(res.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.INVALID_INPUT });
});
it('rejects a remote quick-start when the remote host lacks tmux', async () => {
remoteStore.hosts = [{ id: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu' }];
remoteStore.cases = [{ name: 'gpu-work', type: 'remote', hostId: 'gpu-box', remotePath: '/home/ubuntu/work' }];
remoteStore.tmuxCheck = {
ok: false,
error: 'remote host 10.0.0.42 needs tmux installed for durable remote sessions',
};
const res = await harness.app.inject({
method: 'POST',
url: '/api/quick-start',
payload: { caseName: 'gpu-work', mode: 'shell' },
});
expect(res.statusCode).toBe(httpStatusForErrorCode(ApiErrorCode.OPERATION_FAILED));
expect(JSON.parse(res.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.OPERATION_FAILED });
});
it('does not run local codex availability check for a remote codex case', async () => {
// A remote codex case must NOT be blocked by the LOCAL codex availability gate
// (the CLI runs on the remote host). Probe is stubbed ok in remoteStore.tmuxCheck.
const startInteractive = vi.spyOn(Session.prototype, 'startInteractive').mockResolvedValue(undefined);
try {
remoteStore.hosts = [
{ id: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu', commands: { codex: 'exec codx' } },
];
remoteStore.cases = [{ name: 'gpu-work', type: 'remote', hostId: 'gpu-box', remotePath: '/home/ubuntu/work' }];
const res = await harness.app.inject({
method: 'POST',
url: '/api/quick-start',
payload: { caseName: 'gpu-work', mode: 'codex' },
});
expect(res.statusCode).toBe(200);
expect(JSON.parse(res.body).success).toBe(true);
} finally {
startInteractive.mockRestore();
}
});
it('creates session with valid resumeSessionId', async () => {
const res = await harness.app.inject({
method: 'POST',
+20
View File
@@ -174,4 +174,24 @@ describe('session attachment history', () => {
).toBe('/tmp');
expect(resolveMuxAttachCwd('/opt/projects/Codeman')).toBe('/opt/projects/Codeman');
});
it('round-trips remote metadata through the Session constructor (restart-recovery contract)', () => {
// restoreMuxSessions() reconstructs recovered sessions via
// `new Session({ ..., remote: muxSession.remote ?? savedState.remote })`. If that
// remote does not survive toState(), the next persistSessionState() erases it from
// state.json AND the attach cwd falls back to the (nonexistent-locally) remote path.
const remote = {
hostId: 'gpu-box',
label: 'GPU Box',
host: '10.0.0.42',
username: 'ubuntu',
remotePath: '/home/ubuntu/work',
commands: { claude: 'exec claude --dangerously-skip-permissions' },
};
const restored = new Session({ workingDir: '/home/ubuntu/work', remote });
const state = restored.toState();
expect(state.remote).toEqual(remote);
// Recovered attach cwd must be a LOCAL path, never the remote-only workingDir.
expect(resolveMuxAttachCwd(state.workingDir, state.remote)).toBe('/tmp');
});
});
+36 -1
View File
@@ -10,6 +10,7 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import {
TmuxManager,
buildRemoteKillCommand,
buildRemoteLaunchCommand,
formatPaneSnapshot,
parsePaneList,
@@ -116,8 +117,11 @@ describe('TmuxManager (unit)', () => {
expect(command).toContain('BatchMode=yes');
expect(command).toContain('ubuntu@10.0.0.42');
expect(command).toContain('/home/ubuntu/work');
expect(command).toContain('tmux -L codeman new-session -A');
// Dedicated socket + a name that fails a remote Codeman's SAFE_MUX_NAME_PATTERN.
expect(command).toContain('tmux -L codeman-remote new-session -A -s codeman-ssh-abc123de');
expect(command).toContain('exec codx personal');
// Session options are scoped per-session, never global (-g).
expect(command).not.toContain('set -g');
});
it('uses default shell command when no override is configured', () => {
@@ -135,6 +139,37 @@ describe('TmuxManager (unit)', () => {
expect(command).toContain('exec bash -l');
});
it('defaults claude to a non-interactive launch (--dangerously-skip-permissions)', () => {
const command = buildRemoteLaunchCommand({
mode: 'claude',
remote: { hostId: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu', remotePath: '/w' },
sessionId: 'abc123def456',
});
expect(command).toContain('exec claude --dangerously-skip-permissions');
});
});
describe('remote kill command builder', () => {
it('kills the durable remote tmux session on the dedicated socket via ssh', () => {
const command = buildRemoteKillCommand({
remote: {
hostId: 'gpu-box',
label: 'GPU Box',
host: '10.0.0.42',
username: 'ubuntu',
remotePath: '/home/ubuntu/work',
},
sessionId: 'abc123def456',
});
expect(command).toContain('ssh');
// Shares the default ConnectTimeout so an unreachable host fails fast (never blocks kill).
expect(command).toContain('-o ConnectTimeout=10');
expect(command).toContain('ubuntu@10.0.0.42');
expect(command).toContain('tmux -L codeman-remote kill-session -t');
expect(command).toContain('codeman-ssh-abc123de');
});
});
describe('getAttachCommand', () => {
+38 -7
View File
@@ -66,7 +66,14 @@ describe('TmuxManager restart recovery (test mode safety)', () => {
mode: 'claude',
attached: false,
name: 'Recovery Test',
respawnConfig: { enabled: true, idleTimeoutMs: 10000, updatePrompt: 'continue', interStepDelayMs: 2000, sendClear: false, sendInit: true },
respawnConfig: {
enabled: true,
idleTimeoutMs: 10000,
updatePrompt: 'continue',
interStepDelayMs: 2000,
sendClear: false,
sendInit: true,
},
});
const result = await manager.reconcileSessions();
@@ -86,6 +93,34 @@ describe('TmuxManager restart recovery (test mode safety)', () => {
expect(result.discovered).toHaveLength(0);
});
it('preserves remote SSH metadata across reconcile (mux-sessions.json round-trip source)', async () => {
manager.registerSession({
sessionId: 'remote-recovery-1',
muxName: 'codeman-de51ecaf',
pid: 1,
createdAt: Date.now(),
workingDir: '/home/ubuntu/work',
mode: 'claude',
attached: false,
name: 'Remote Recovery',
remote: {
hostId: 'gpu-box',
label: 'GPU Box',
host: '10.0.0.42',
username: 'ubuntu',
remotePath: '/home/ubuntu/work',
},
});
const result = await manager.reconcileSessions();
expect(result.alive).toContain('remote-recovery-1');
// restoreMuxSessions() reads MuxSession.remote off exactly this map to rebuild
// the recovered Session — if it were dropped here the session would respawn LOCAL.
const recovered = manager.getSession('remote-recovery-1');
expect(recovered?.remote).toMatchObject({ hostId: 'gpu-box', host: '10.0.0.42', remotePath: '/home/ubuntu/work' });
});
it('should not execute any tmux commands in test mode', async () => {
manager.registerSession({
sessionId: 'alive-session',
@@ -101,9 +136,7 @@ describe('TmuxManager restart recovery (test mode safety)', () => {
await manager.reconcileSessions();
// Verify no tmux commands were executed
const tmuxCalls = mockedExecSync.mock.calls.filter(
([cmd]) => typeof cmd === 'string' && cmd.includes('tmux')
);
const tmuxCalls = mockedExecSync.mock.calls.filter(([cmd]) => typeof cmd === 'string' && cmd.includes('tmux'));
expect(tmuxCalls).toHaveLength(0);
});
@@ -155,9 +188,7 @@ describe('TmuxManager restart recovery (test mode safety)', () => {
expect(manager.getSession('kill-me')).toBeUndefined();
// Verify no real kill commands were executed
const killCalls = mockedExecSync.mock.calls.filter(
([cmd]) => typeof cmd === 'string' && cmd.includes('kill')
);
const killCalls = mockedExecSync.mock.calls.filter(([cmd]) => typeof cmd === 'string' && cmd.includes('kill'));
expect(killCalls).toHaveLength(0);
});
});