diff --git a/src/session.ts b/src/session.ts index fdad22f8..b3226332 100644 --- a/src/session.ts +++ b/src/session.ts @@ -62,6 +62,8 @@ import { type SessionWriteOptions, } from './types.js'; import { resolveAndClaimOmpSessionId } from './utils/omp-session-resolver.js'; +import { claudeTranscriptExists } from './utils/claude-transcript.js'; +import { matchesPattern } from './config/cli-registry/patterns.js'; import { probeDockerCliVersion } from './docker-hosts.js'; import { probeRemoteCliVersion } from './remote-hosts.js'; import type { TerminalMultiplexer, MuxSession } from './mux-interface.js'; @@ -1749,7 +1751,7 @@ export class Session extends EventEmitter { // `options.respawnPaneOptions` was built eagerly before this dead-pane // check ran, so it still carries the pre-pin ompConfig; rebuild it. this._pinOmpRespawnId(); - const newPid = await mux.respawnPane(this._buildRespawnPaneOptions()); + const newPid = await mux.respawnPane(await this._buildRespawnPaneOptionsWithResumePin()); if (!newPid) { console.error('[Session] Failed to respawn pane, will create new session'); needsNewSession = true; @@ -1765,7 +1767,17 @@ export class Session extends EventEmitter { if (isRestored) { console.log('[Session] Attaching to existing mux session:', this._muxSession!.muxName); } else { - // Create a new mux session + // Create a new mux session. When this is the FALLBACK after a failed + // respawn, the eagerly-built create options still carry the unpinned + // launch seed, so a session whose transcript exists would meet the same + // `--session-id ... already in use` refusal the respawn just lost to — + // the recovery of last resort failing for the very reason it was needed. + // A genuinely new session has no chain and no transcript, so the pin + // resolves to its own id and the command shape is unchanged. + if (needsNewSession) { + const pinned = (await this._buildRespawnPaneOptionsWithResumePin()).resumeSessionId; + if (pinned) options.createSessionOptions.resumeSessionId = pinned; + } this._muxSession = await mux.createSession(options.createSessionOptions); console.log('[Session] Created mux session:', this._muxSession.muxName); // No extra sleep — createSession() already waits for tmux readiness @@ -1878,21 +1890,7 @@ export class Session extends EventEmitter { } this._pinOmpRespawnId(); - const options = this._buildRespawnPaneOptions(); - // Unlike the dead-pane respawn, this one kills a WORKING pane whose conversation - // already has a transcript, and a CLI that launches with `--session-id ` refuses - // an id that is already in use (claude: `Error: Session ID ... is already in use.`), - // which turned an endpoint switch into a dead pane and a lost session. A launch that - // declares a `fallback` chain renders `resume || new` once a resume id is set, the - // same `--resume || --session-id ` shape the docker and remote pane commands - // already use, so pin the live conversation id for THIS respawn only. The registry - // shape is the gate, not the CLI's name: an entry whose resume id is minted by the - // CLI itself (codex/pi/omp/grok) never declares that chain, and its resume field is - // read from its own `Config` rather than this top-level one anyway. - if (!options.resumeSessionId && getCli(this.mode)?.launch.chain === 'fallback') { - options.resumeSessionId = this._claudeSessionId ?? this.id; - } - const newPid = await mux.respawnPane(options); + const newPid = await mux.respawnPane(await this._buildRespawnPaneOptionsWithResumePin()); if (!newPid) { console.error('[Session] restartCli: respawnPane failed for', this._muxSession.muxName); return false; @@ -1947,6 +1945,93 @@ export class Session extends EventEmitter { return this._withCustomModelLaunchModel(options); } + /** + * Respawn options for a pane whose command is being REPLACED, with the + * conversation pinned so the relaunch resumes rather than collides. + * + * A CLI that launches with `--session-id ` refuses an id that is already + * in use (claude: `Error: Session ID ... is already in use.`), and every + * session whose agent has been prompted owns a transcript under that id. So + * relaunching such a pane with the bare launch line fails, the pane dies + * again immediately, and the user's conversation is stranded. A launch that + * declares a `fallback` chain renders `resume || new` once a resume id is + * set, which is the shape that survives both cases. + * + * Four conditions gate the pin, each protecting against a way of resuming the + * WRONG conversation or of making a working relaunch fail. + * + * ⚠️ **A remote or docker session is never pinned.** Unlike `restartCli()`, + * whose route refuses both, the dead-pane respawn is reached by every session + * shape. Their pane commands (`buildRemoteLaunchCommand`, + * `claudeDockerPaneCommand`) already render a SELF-HEALING + * `--session-id || --resume `, and both flip to resume-first the + * moment the resume id differs from the session id. The conversation lives on + * the far side, so a local id pinned onto it resolves to nothing there, the + * resume fails, and the `--session-id` fallback then collides with the + * transcript the far side really does hold — both branches fail and the pane + * dies. `_pinOmpRespawnId()` refuses remote for the same reason. + * + * ⚠️ **The id comes from the conversation CHAIN, not from + * `_claudeSessionId`.** That field holds either a first-hand id from the + * CLI's own hook payload or a history correlation, which is a guess keyed on + * the working directory. `_recordClaudeSessionInChain()` refuses a guess + * precisely so it cannot "write a foreign conversation into this pane's + * permanent record", and launching from one would do worse than the display + * bug that rule exists to prevent: the relaunched CLI would open and WRITE to + * a conversation that was never this pane's. The chain's tail is the live + * conversation and is hook-vouched, so it also outranks the launch seed, + * which is written once at construction and never moves off a `/clear`. + * + * ⚠️ **A pin that no transcript backs is dropped.** When the pinned id + * differs from the session id, the fallback branch still carries + * `--session-id `; if the resume finds nothing, that fallback + * collides and the pane dies exactly as it did before this pinning existed. + * The create route pre-validates a resume id for the same reason, though it + * additionally requires the transcript be substantial — here mere existence + * is the question, because a one-line transcript still makes `--session-id` + * collide. + * + * The registry shape is the last gate, not the CLI's name: an entry whose + * resume id is minted by the CLI itself (codex/pi/omp/grok) declares no + * `fallback` chain and reads its resume field from its own `Config`. + * + * `reattachRemote()` deliberately does NOT call this. It re-runs the remote + * session command, which attaches to the durable remote tmux with the agent + * still inside it and renders no local `--session-id` to collide. + */ + private async _buildRespawnPaneOptionsWithResumePin(): Promise { + const options = this._buildRespawnPaneOptions(); + if (this._remote || this._docker) return options; + if (getCli(this.mode)?.launch.chain !== 'fallback') return options; + + const chainTail = this._claudeSessionChain[this._claudeSessionChain.length - 1]; + const pin = chainTail ?? options.resumeSessionId ?? this.id; + // A session Codeman DISCOVERED on the socket rather than created carries a + // synthetic `restored-` id, which fails claude's `uuid` token + // pattern. The renderer would silently drop the resume flag and emit the + // unpinned command, so say so here rather than letting the caller believe + // the pane was pinned. Such a pane keeps the pre-existing behaviour. + const resumeIdPattern = getCli(this.mode)?.launch.params?.resumeId; + if (resumeIdPattern?.type === 'token' && !matchesPattern(resumeIdPattern.pattern, pin)) { + console.log(`[Session] Not pinning resume id ${pin} for relaunch: the CLI cannot accept that id shape`); + return options; + } + // Pinning the session's own id renders the self-healing + // `--session-id || --resume `, which needs no transcript to be + // correct: it starts fresh when there is none and resumes when there is. + if (pin !== this.id && !(await claudeTranscriptExists(pin, this._claudeConfigDir()))) { + console.log(`[Session] Not pinning resume id ${pin} for relaunch: no transcript on disk`); + return options; + } + options.resumeSessionId = pin; + return options; + } + + /** The session's Claude config dir when it has been relocated (#255), else undefined. */ + private _claudeConfigDir(): string | undefined { + return this._envOverrides?.CLAUDE_CONFIG_DIR; + } + /** * Force the custom-model selection's `launchModel` (pi/omp `custom/`, grok's * `[model.]` block name) onto the CLI's `model` launch param. Where that param diff --git a/src/utils/claude-transcript.ts b/src/utils/claude-transcript.ts new file mode 100644 index 00000000..6c6c4b49 --- /dev/null +++ b/src/utils/claude-transcript.ts @@ -0,0 +1,57 @@ +/** + * @fileoverview Does a Claude conversation transcript exist on this host? + * + * Claude writes one `.jsonl` per conversation under + * `/projects//`. Two launch decisions turn on whether + * such a file exists: `--resume ` needs one, and `--session-id ` is + * REFUSED when one exists (`Error: Session ID ... is already in use.`). + * + * The project directory name is derived from the working directory, and a case + * that has been moved or renamed leaves its transcript under the OLD name, so + * the search is across every project directory rather than the one that matches + * the pane's cwd today. + * + * ⚠️ Existence is the whole question here, with no size floor. The create route + * additionally requires ~4 KB before it will resume, which is a "is this + * conversation worth resuming" judgement; for a relaunch the question is the + * opposite one — a one-line transcript still makes `--session-id` collide. + * + * @dependencies none + * @consumedby session (relaunch resume pinning) + * + * @module utils/claude-transcript + */ + +import { readdir, stat } from 'node:fs/promises'; +import { homedir } from 'node:os'; +import { join } from 'node:path'; + +/** `/projects`, honouring a session's relocated `CLAUDE_CONFIG_DIR`. */ +export function claudeProjectsDir(configDir?: string): string { + return join(configDir || join(homedir(), '.claude'), 'projects'); +} + +/** + * True when a transcript for `conversationId` exists under any project + * directory. Returns false for a missing projects dir or an unreadable one: + * the caller's fallback is to skip the resume, which is the safe direction. + */ +export async function claudeTranscriptExists(conversationId: string, configDir?: string): Promise { + if (!conversationId) return false; + const projectsDir = claudeProjectsDir(configDir); + let projectDirs: string[]; + try { + projectDirs = await readdir(projectsDir); + } catch { + return false; + } + for (const projectDir of projectDirs) { + try { + await stat(join(projectsDir, projectDir, `${conversationId}.jsonl`)); + return true; + } catch { + // Not in this project directory; keep looking. + } + } + return false; +} diff --git a/test/respawn-session-id-collision.test.ts b/test/respawn-session-id-collision.test.ts new file mode 100644 index 00000000..34b3f24a --- /dev/null +++ b/test/respawn-session-id-collision.test.ts @@ -0,0 +1,282 @@ +/** + * @fileoverview Relaunching a pane must resume its conversation, not collide + * with it, and must not resume somebody else's. + * + * A CLI that launches with `--session-id ` refuses an id that is already in + * use (claude: `Error: Session ID ... is already in use.`), and every session + * whose agent has been prompted owns a transcript under that id. A relaunch + * that passes the bare launch line therefore dies on startup, the pane goes + * dead again at once, and the conversation is stranded. + * + * `restartCli()` has pinned a resume id for this reason since the custom-model + * work. The dead-pane respawn in `_setupOrAttachMuxSession()` did not, and its + * comment said so explicitly — "Unlike the dead-pane respawn, this one kills a + * WORKING pane whose conversation already has a transcript". That assumption is + * what these tests refute: a pane whose agent exited has a transcript too. + * + * The pin is gated four ways, and most of these tests are about the gates + * rather than the pin, because each gate stands for a way of resuming the WRONG + * conversation or of making a working relaunch fail. + * + * Port: N/A + */ +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { Session } from '../src/session.js'; +import { getCli } from '../src/config/cli-registry/registry.js'; +import { buildSpawnCommandFromRegistry } from '../src/session-cli-registry-bridge.js'; +import type { MuxSession, RespawnPaneOptions, TerminalMultiplexer } from '../src/mux-interface.js'; + +/** Captures the options each respawn is invoked with. */ +function recordingMux() { + const calls: RespawnPaneOptions[] = []; + const mux = { + isAvailable: () => true, + muxSessionExists: () => true, + isPaneDead: () => true, + // Called by the PTY-exit handler during teardown. Absent, it throws + // asynchronously after the test body has already passed, which vitest + // reports as an unhandled error rather than a failure. + setAttached: () => {}, + respawnPane: async (options: RespawnPaneOptions) => { + calls.push(options); + return 4242; + }, + }; + return { mux: mux as unknown as TerminalMultiplexer, calls }; +} + +const muxSession = (muxName = 'codeman-aaaa') => ({ muxName, sessionId: 'aaaa' }) as unknown as MuxSession; + +const CONVERSATION = 'aaaabbbb-cccc-dddd-eeee-ffff00001111'; + +let configDir: string; + +/** A relocated Claude config dir, so the transcript gate reads a real fixture. */ +beforeEach(() => { + configDir = mkdtempSync(join(tmpdir(), 'codeman-transcript-')); +}); + +afterEach(() => { + rmSync(configDir, { recursive: true, force: true }); +}); + +/** Write the `.jsonl` Claude would have written for a conversation. */ +function giveTranscript(conversationId: string): void { + const projectDir = join(configDir, 'projects', '-tmp-case'); + mkdirSync(projectDir, { recursive: true }); + writeFileSync(join(projectDir, `${conversationId}.jsonl`), '{"type":"user"}\n'); +} + +function localSession(extra: Record = {}, mux?: TerminalMultiplexer) { + return new Session({ + workingDir: '/tmp', + mode: 'claude', + useMux: true, + mux, + muxSession: muxSession(), + envOverrides: { CLAUDE_CONFIG_DIR: configDir }, + ...extra, + }); +} + +describe('pinning a conversation onto a relaunch', () => { + it('pins the chain tail on the DEAD-PANE respawn, which is the bug', async () => { + // The path a recovered `/exit`ed session takes, and the one that was + // missing the pin. Driven through `startInteractive()` rather than asserted + // from source: a comment claiming a thing happens is exactly what was wrong + // here before. + giveTranscript(CONVERSATION); + const { mux, calls } = recordingMux(); + const session = localSession({ claudeSessionChain: [CONVERSATION] }, mux); + + await session.startInteractive(); + try { + expect(calls).toHaveLength(1); + expect(calls[0].resumeSessionId).toBe(CONVERSATION); + } finally { + await session.stop(); + } + }); + + it('pins the chain tail on a custom-model restart too', async () => { + giveTranscript(CONVERSATION); + const { mux, calls } = recordingMux(); + const session = localSession({ claudeSessionChain: [CONVERSATION] }, mux); + + expect(await session.restartCli()).toBe(true); + + expect(calls[0].resumeSessionId).toBe(CONVERSATION); + }); + + it('prefers the live chain tail over the launch seed', async () => { + // `_resumeSessionId` is written once at construction and never moves, so a + // `/clear` after launch leaves it pointing at the predecessor. Resuming + // that would reopen an abandoned conversation and strand the live one. + giveTranscript(CONVERSATION); + const { mux, calls } = recordingMux(); + const session = localSession( + { resumeSessionId: 'bbbbcccc-dddd-eeee-ffff-000011112222', claudeSessionChain: [CONVERSATION] }, + mux + ); + + expect(await session.restartCli()).toBe(true); + + expect(calls[0].resumeSessionId).toBe(CONVERSATION); + }); + + it('falls back to the launch seed when no conversation was ever recorded', async () => { + const seed = 'bbbbcccc-dddd-eeee-ffff-000011112222'; + giveTranscript(seed); + const { mux, calls } = recordingMux(); + const session = localSession({ resumeSessionId: seed }, mux); + + expect(await session.restartCli()).toBe(true); + + expect(calls[0].resumeSessionId).toBe(seed); + }); + + it('never resumes an id the conversation chain did not vouch for', async () => { + // `_claudeSessionId` also holds history-CORRELATED guesses, keyed on the + // working directory, which the chain deliberately refuses. Launching from + // one would open and WRITE to a conversation that was never this pane's — + // worse than the display bug that rule exists to prevent. + giveTranscript(CONVERSATION); + const { mux, calls } = recordingMux(); + const session = localSession({}, mux); + session.adoptClaudeSessionId(CONVERSATION); // no firstHand flag: a guess + expect(session.claudeSessionId).toBe(CONVERSATION); + + expect(await session.restartCli()).toBe(true); + + expect(calls[0].resumeSessionId).not.toBe(CONVERSATION); + expect(calls[0].resumeSessionId).toBe(session.id); + }); + + it('drops a pin that no transcript backs', async () => { + // A divergent pin renders `--resume || --session-id `, so a + // resume that finds nothing falls back onto the colliding form and the pane + // dies exactly as it did before any of this. No transcript, no pin. + const { mux, calls } = recordingMux(); + const session = localSession({ claudeSessionChain: [CONVERSATION] }, mux); + + expect(await session.restartCli()).toBe(true); + + expect(calls[0].resumeSessionId).toBeUndefined(); + }); + + it('pins nothing for a remote session, whose conversation lives elsewhere', async () => { + // The dead-pane respawn is reached by every session shape, unlike + // `restartCli()` whose route refuses remote. A local id pinned onto a + // remote pane resolves to nothing there, and the `--session-id` fallback + // then collides with the transcript the remote host really does hold. + giveTranscript(CONVERSATION); + const { mux, calls } = recordingMux(); + const session = localSession( + { + remote: { hostId: 'h1', label: 'box', host: 'box', username: 'dev', remotePath: '/tmp' }, + claudeSessionChain: [CONVERSATION], + }, + mux + ); + + await session.startInteractive(); + try { + expect(calls[0].resumeSessionId).toBeUndefined(); + } finally { + await session.stop(); + } + }); + + it('pins nothing for a docker case, whose pane execs into the container', async () => { + giveTranscript(CONVERSATION); + const { mux, calls } = recordingMux(); + const session = localSession( + { + docker: { hostId: 'd1', label: 'ctr', containerName: 'ctr' }, + claudeSessionChain: [CONVERSATION], + }, + mux + ); + + await session.startInteractive(); + try { + expect(calls[0].resumeSessionId).toBeUndefined(); + } finally { + await session.stop(); + } + }); + + it('pins nothing for a CLI that mints its own resume id', async () => { + // codex/pi/omp/grok declare no `fallback` chain and read their resume id + // from their own config, so a top-level pin would be meaningless at best. + const { mux, calls } = recordingMux(); + const session = localSession({ mode: 'codex' }, mux); + + expect(await session.restartCli()).toBe(true); + + expect(calls[0].resumeSessionId).toBeUndefined(); + }); + + it('pins nothing for a remote reattach, which relaunches no CLI', async () => { + // `reattachRemote()` re-runs the remote session command, which attaches to + // the durable remote tmux with the agent still running inside it. + giveTranscript(CONVERSATION); + const { mux, calls } = recordingMux(); + const session = localSession( + { + remote: { hostId: 'h1', label: 'box', host: 'box', username: 'dev', remotePath: '/tmp' }, + claudeSessionChain: [CONVERSATION], + }, + mux + ); + + expect(await session.reattachRemote()).toBe(true); + + expect(calls[0].resumeSessionId).toBeUndefined(); + }); +}); + +describe('what the pin renders', () => { + // The rendered command is what actually runs, and it is where each gate's + // reason shows. Asserting here rather than counting call sites in the source + // is what would have caught the divergent-pin and synthetic-id cases. + const SID = '0f9c2b14-1111-2222-3333-444455556666'; + const entry = getCli('claude'); + const render = (resumeSessionId?: string) => { + if (!entry) throw new Error('no registry entry for claude'); + return buildSpawnCommandFromRegistry(entry, { + mode: 'claude', + sessionId: SID, + claudeCliVersion: null, + resumeSessionId, + }); + }; + + it('renders the colliding bare form with no pin — the bug itself', () => { + expect(render()).toContain(`--session-id "${SID}"`); + expect(render()).not.toContain('--resume'); + }); + + it('renders a self-healing resume-or-new when the pin is the session id', () => { + expect(render(SID)).toBe( + `claude --dangerously-skip-permissions --resume "${SID}" || claude --dangerously-skip-permissions --session-id "${SID}"` + ); + }); + + it('keeps the SESSION id in the fallback branch when the pin diverges', () => { + // Which is why a pin with no transcript behind it has to be dropped: the + // fallback is the colliding form, so a failed resume dies twice. + expect(render(CONVERSATION)).toContain(`--resume "${CONVERSATION}"`); + expect(render(CONVERSATION)).toContain(`--session-id "${SID}"`); + }); + + it('drops a synthetic discovered id, which fails the uuid token pattern', () => { + // `reconcileSessions()` mints `restored-` for a tmux session + // Codeman found but does not own. The renderer emits the unpinned command, + // so those panes keep the pre-existing behaviour. + expect(render('restored-40568a29')).not.toContain('--resume'); + }); +});