From 47e793527482ea6d89e80aa825652fb8361cf355 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Mon, 21 Sep 2026 13:36:27 +0200 Subject: [PATCH] fix(session): resume the conversation when respawning a dead pane 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. The dead-pane respawn in `_setupOrAttachMuxSession()` passed the bare launch line, so recovering such a session relaunched a CLI that died on startup, the pane went dead again at once, and the conversation was stranded behind a tab that looked merely idle. `restartCli()` has pinned a resume id against this since the custom-model work, and its comment states the assumption that made the other path look safe: "Unlike the dead-pane respawn, this one kills a WORKING pane whose conversation already has a transcript". A pane whose agent exited has a transcript too. Both relaunch paths now build options through `_buildRespawnPaneOptionsWithResumePin()`, and so does the create-path fallback after a failed respawn, which otherwise met the same refusal that made it the fallback. Four gates guard the pin, each standing for 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 already render a self-healing `--session-id || --resume`, and both flip to resume-first once the resume id differs; the conversation lives on the far side, so a local id resolves to nothing there and the `--session-id` fallback then collides with the transcript the far side does hold. The id comes from the conversation CHAIN rather than `_claudeSessionId`, which also holds history-correlated guesses keyed on the working directory. `_recordClaudeSessionInChain()` refuses those so they cannot "write a foreign conversation into this pane's permanent record", and launching from one is worse than the display bug that rule prevents. The chain tail also outranks the launch seed, which is written once at construction and never moves off a `/clear`. A pin no transcript backs is dropped, because the fallback branch keeps `--session-id ` and would collide. A synthetic `restored-` id from socket discovery is dropped too, and logged: it fails claude's `uuid` token pattern, so the renderer would emit the unpinned command while the caller believed otherwise. Tests cover each gate and the rendered command. Four of them fail against the unfixed source; the remote and docker ones were separately checked against a build with only that guard removed, since they pass on master for the wrong reason. Co-Authored-By: Claude Opus 5 (1M context) --- src/session.ts | 119 +++++++-- src/utils/claude-transcript.ts | 57 +++++ test/respawn-session-id-collision.test.ts | 282 ++++++++++++++++++++++ 3 files changed, 441 insertions(+), 17 deletions(-) create mode 100644 src/utils/claude-transcript.ts create mode 100644 test/respawn-session-id-collision.test.ts 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'); + }); +});