mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 18:39:33 +01:00
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
9b8f853c94
commit
7e70f6f0c3
4 files changed
+121
-6
No files matched your search
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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<void> {
|
||||
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, [], {
|
||||
|
||||
@@ -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<typeof import('node-pty')>();
|
||||
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<string, unknown>;
|
||||
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();
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user