diff --git a/src/session.ts b/src/session.ts index 0a22811b..8a0608aa 100644 --- a/src/session.ts +++ b/src/session.ts @@ -57,7 +57,7 @@ import { type SessionRemote, type SessionDocker, } from './types.js'; -import { findLatestOmpSessionId } from './utils/omp-session-resolver.js'; +import { resolveAndClaimOmpSessionId } from './utils/omp-session-resolver.js'; import { probeDockerCliVersion } from './docker-hosts.js'; import { probeRemoteCliVersion } from './remote-hosts.js'; import type { TerminalMultiplexer, MuxSession } from './mux-interface.js'; @@ -1514,7 +1514,11 @@ export class Session extends EventEmitter { let needsNewSession = false; if (this._muxSession && mux.isPaneDead(this._muxSession.muxName)) { console.log('[Session] Dead pane detected, respawning:', this._muxSession.muxName); - const newPid = await mux.respawnPane(options.respawnPaneOptions); + // Confirmed dead — safe to resolve/pin now (see `_pinOmpRespawnId()`). + // `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()); if (!newPid) { console.error('[Session] Failed to respawn pane, will create new session'); needsNewSession = true; @@ -1605,6 +1609,9 @@ export class Session extends EventEmitter { return false; } + // Confirmed the mux session (and thus the pane) exists but this reattach + // is about to respawn it — safe to resolve/pin now. + this._pinOmpRespawnId(); const newPid = await mux.respawnPane(this._buildRespawnPaneOptions()); if (!newPid) { console.error('[Session] reattachRemote: respawnPane failed for', this._muxSession.muxName); @@ -1637,21 +1644,16 @@ export class Session extends EventEmitter { piConfig: this._piConfig, grokConfig: this._grokConfig, deepSeekConfig: this._deepSeekConfig, - // Respawning a dead pane means the CLI process exited (crash, idle - // respawn, or the user's own /exit) but this is still the same - // conversation from the user's perspective — unlike a brand-new - // `createSession` call, defaulting to continuation here is the honest - // behavior. `--continue` alone is ambiguous the moment ANY other omp - // conversation has touched this directory more recently (ours resumed - // elsewhere, a second Codeman session opened here, ...) since it just - // picks the newest session file — so resolve and PIN the exact id the - // pane that just died was writing to, once. The dead pane's file is - // already fully flushed at this point, so "newest file" here is - // unambiguous by construction; every later respawn then reuses the - // pinned id instead of re-guessing. Only when the session already - // carries an explicit resumeSessionId does this skip straight past it - // (that one always wins in buildOmpCommand regardless). - ompConfig: this._resolvedOmpRespawnConfig(), + // OMP resolution/pinning does NOT happen here. This object is built + // EAGERLY — including on every boot-recovery reattach, before anyone + // knows whether the pane is actually dead — so resolving here mutated + // `_ompConfig`/`_claudeSessionId` even for a pane that was simply being + // reattached to, not respawned; with two omp tabs in the same case dir + // that mis-pinned the ALIVE session onto whichever file happened to be + // newest on disk (reported live in the Ark0N/Codeman#353 review). The + // real pin now happens in `_pinOmpRespawnId()`, called by callers ONLY + // once they've confirmed an actual respawn is about to happen. + ompConfig: this._ompConfig, resumeSessionId: this._resumeSessionId, envOverrides: this._envOverrides, effort: this._effort, @@ -1671,23 +1673,18 @@ export class Session extends EventEmitter { * "newest file on disk" is safe here specifically. Non-omp modes and a * session that already carries an explicit id pass through untouched. */ - private _resolvedOmpRespawnConfig(): OmpConfig | undefined { - if (this.mode !== 'omp') return this._ompConfig; - if (this._ompConfig?.resumeSessionId) return this._ompConfig; - // Resolving-and-pinning is only correct when a mux session ALREADY exists for - // this Session object — a dead-pane respawn, or a boot-recovery reattach (the - // constructor sets _muxSession from persisted state before startInteractive() - // ever runs there). A genuinely brand-new session (Run OMP -> POST - // /api/quick-start -> a fresh Session with no muxSession in its create config) - // has _muxSession still null at this point. Without this guard, the eager - // `respawnPaneOptions: this._buildRespawnPaneOptions()` in startInteractive() - // mutates this._ompConfig via the side effect below BEFORE - // createSessionOptions.ompConfig is even read in the SAME object literal, so a - // fresh "Run OMP" click silently inherited whatever omp conversation happened - // to be newest on disk for this working directory instead of starting clean - // (reported live 2026-08-27). - if (!this._muxSession) return this._ompConfig; - const resolvedId = findLatestOmpSessionId(this.workingDir); + private _pinOmpRespawnId(): void { + if (this.mode !== 'omp') return; + if (this._ompConfig?.resumeSessionId) return; + // Callers MUST call this only immediately before an ACTUAL respawn (a + // confirmed-dead pane, or a genuine remote reattach) — never while merely + // building options that might not lead to a respawn. A fresh "Run OMP" + // click has no _muxSession yet and must never inherit whatever omp + // conversation happens to be newest on disk for this working directory + // (reported live 2026-08-27, fixed in 13a19f79); this guard keeps that + // fix intact now that resolution has moved out of the eager options build. + if (!this._muxSession) return; + const resolvedId = resolveAndClaimOmpSessionId(this.workingDir); if (resolvedId) { this._ompConfig = { ...this._ompConfig, resumeSessionId: resolvedId }; // Alias omp's own session uuid to this Codeman id — see the @@ -1695,14 +1692,15 @@ export class Session extends EventEmitter { // (generically-named) mechanism that folds a Past-Sessions row back // into its live/persisted session instead of duplicating it. this._claudeSessionId = resolvedId; - return this._ompConfig; + return; } - // Nothing on disk yet (the dying process never got far enough to write a - // session file) — fall back to the CLI's own "most recent" heuristic. + // Nothing unclaimed on disk (the dying process never got far enough to + // write a session file, or a sibling already claimed the only candidate) + // — fall back to the CLI's own "most recent" heuristic. console.warn( `[Session] OMP: no session file found under ${this.workingDir} to pin --resume on respawn; falling back to ambiguous --continue` ); - return { ...this._ompConfig, continueSession: true }; + this._ompConfig = { ...this._ompConfig, continueSession: true }; } /** @@ -1968,10 +1966,11 @@ export class Session extends EventEmitter { }); // Set claudeSessionId — when resuming, the Claude conversation ID is the - // resumed one. `_resolvedOmpRespawnConfig()` (called above while building - // respawnPaneOptions) may have JUST aliased this to omp's own session - // uuid — that already-resolved id must win over the generic - // `this.id` fallback, or this line clobbers it back to the Codeman id + // resumed one. `_pinOmpRespawnId()` (called just above, inside + // `_setupOrAttachMuxSession()`'s dead-pane branch) may have JUST aliased + // this to omp's own session uuid — that already-resolved id must win + // over the generic `this.id` fallback, or this line clobbers it back + // to the Codeman id // on every single respawn. this._claudeSessionId = this._resumeSessionId || this._ompConfig?.resumeSessionId || this.id; @@ -2394,7 +2393,7 @@ export class Session extends EventEmitter { /** * A brand-new omp session (never yet respawned, so - * {@link _resolvedOmpRespawnConfig} has never run) has no captured + * {@link _pinOmpRespawnId} has never run) has no captured * omp-native session id: `_claudeSessionId` still defaults to this * session's OWN Codeman id from the constructor. Until something aliases * it, the omp history scan's row for this exact conversation (keyed by @@ -2407,7 +2406,7 @@ export class Session extends EventEmitter { private _maybeCaptureOmpSessionId(): void { if (this.mode !== 'omp' || this._claudeSessionId !== this.id) return; try { - const resolvedId = findLatestOmpSessionId(this.workingDir); + const resolvedId = resolveAndClaimOmpSessionId(this.workingDir); if (resolvedId) { this._claudeSessionId = resolvedId; this._ompConfig = { ...this._ompConfig, resumeSessionId: resolvedId }; diff --git a/src/utils/omp-session-resolver.ts b/src/utils/omp-session-resolver.ts index f76291ee..7b1a5ed7 100644 --- a/src/utils/omp-session-resolver.ts +++ b/src/utils/omp-session-resolver.ts @@ -15,7 +15,7 @@ * @module utils/omp-session-resolver */ -import { readdirSync, statSync } from 'node:fs'; +import { closeSync, openSync, readdirSync, readSync, statSync } from 'node:fs'; import { homedir } from 'node:os'; import { join, sep } from 'node:path'; @@ -92,3 +92,95 @@ export function findLatestOmpSessionId(workingDir: string): string | null { } return newestId; } + +/** + * The session header line is always near the top of the file (the + * transcript's own "second line" — see omp-transcript.ts), so identifying a + * file never needs reading the whole thing (up to multi-MB, per that same + * module's size cap). Bounded read only. + */ +const HEADER_READ_BYTES = 8 * 1024; + +function readOmpSessionHeader(filePath: string): { id: string; cwd: string } | null { + let raw: string; + try { + const fd = openSync(filePath, 'r'); + try { + const buf = Buffer.alloc(HEADER_READ_BYTES); + const bytesRead = readSync(fd, buf, 0, HEADER_READ_BYTES, 0); + raw = buf.toString('utf-8', 0, bytesRead); + } finally { + closeSync(fd); + } + } catch { + return null; + } + for (const line of raw.split('\n')) { + if (!line) continue; + let entry: unknown; + try { + entry = JSON.parse(line); + } catch { + continue; + } + if (!entry || typeof entry !== 'object') continue; + const e = entry as Record; + if (e.type === 'session' && typeof e.id === 'string' && typeof e.cwd === 'string') { + return { id: e.id, cwd: e.cwd }; + } + } + return null; +} + +/** + * Process-wide registry of OMP session ids already pinned to a live Codeman + * session. Two omp tabs in the same case dir (`w1-foo`, `w2-foo`) resolve + * against the SAME directory on disk — without this, both could pick the + * newest file and alias onto each other's conversation (found in upstream PR + * review, Ark0N/Codeman#353). Never released: this holds at most a handful of + * short ids per real omp conversation ever pinned in this process's lifetime, + * immaterial memory even after weeks of uptime — correctness here matters + * more than reclaiming it. + */ +const claimedOmpSessionIds = new Set(); + +/** + * Safe variant of {@link findLatestOmpSessionId} for callers where two omp + * sessions CAN share the same case directory — a dead-pane respawn, a + * boot-recovery reattach, or a first-idle capture — instead of the narrower + * cases where "newest file" is unambiguous by construction. Verifies each + * candidate's own header `cwd` against `workingDir` (mangling is a lossy + * one-way transform — see {@link mangleOmpWorkingDir} — so trusting the + * filename-derived id alone isn't enough) and skips any id a sibling session + * has already claimed. Claims the id it returns so a concurrent caller + * resolving the same directory in the same tick can't double-claim it. + */ +export function resolveAndClaimOmpSessionId(workingDir: string): string | null { + const dir = join(resolveOmpHome(), 'agent', 'sessions', mangleOmpWorkingDir(workingDir)); + let entries: string[]; + try { + entries = readdirSync(dir); + } catch { + return null; + } + + let newestMtime = -Infinity; + let newestId: string | null = null; + for (const entry of entries) { + if (!OMP_SESSION_FILE_PATTERN.test(entry)) continue; + const filePath = join(dir, entry); + let mtimeMs: number; + try { + mtimeMs = statSync(filePath).mtimeMs; + } catch { + continue; + } + if (mtimeMs <= newestMtime) continue; + const header = readOmpSessionHeader(filePath); + if (!header || header.cwd !== workingDir || claimedOmpSessionIds.has(header.id)) continue; + newestMtime = mtimeMs; + newestId = header.id; + } + if (newestId) claimedOmpSessionIds.add(newestId); + return newestId; +} diff --git a/test/omp-fresh-run-no-resume.test.ts b/test/omp-fresh-run-no-resume.test.ts index 21f80f75..49c9f48e 100644 --- a/test/omp-fresh-run-no-resume.test.ts +++ b/test/omp-fresh-run-no-resume.test.ts @@ -1,18 +1,23 @@ /** - * @fileoverview Pins the "Run OMP always resumes" bug found live 2026-08-27. + * @fileoverview Pins the "Run OMP always resumes" bug found live 2026-08-27, + * and its follow-on fix for the sibling-aliasing bug found in upstream PR + * review (Ark0N/Codeman#353). * - * Session._resolvedOmpRespawnConfig() resolves-and-pins the newest on-disk omp - * conversation as a side effect on `this._ompConfig`. That is correct when - * reattaching to an ALREADY-TRACKED mux session (a dead-pane respawn, or a - * boot-recovery reattach — the constructor sets `_muxSession` from persisted - * state before startInteractive() ever runs there). It is wrong for a - * genuinely brand-new session: startInteractive() computes - * `respawnPaneOptions: this._buildRespawnPaneOptions()` EAGERLY in the same - * object literal that builds `createSessionOptions.ompConfig: this._ompConfig`, - * so the resolve-and-pin side effect ran and poisoned `this._ompConfig` before - * that field was even read — a fresh "Run OMP" click in a working directory - * with any prior omp history silently launched `--resume ` instead of - * a clean `omp` invocation. + * Session._pinOmpRespawnId() resolves-and-pins the newest on-disk omp + * conversation as a side effect on `this._ompConfig`. That is correct ONLY + * immediately before an ACTUAL respawn (a confirmed-dead pane, or a genuine + * remote reattach) — never while merely building options that might not + * lead to one. It used to run eagerly inside `_buildRespawnPaneOptions()`, + * which startInteractive() calls unconditionally (including for a genuinely + * brand-new session, and for a boot-recovery reattach to a pane that turns + * out to still be alive): a fresh "Run OMP" click in a working directory + * with any prior omp history silently launched `--resume ` instead + * of a clean `omp` invocation, and — with two omp tabs in the same case dir + * — a live pane's `_ompConfig`/`claudeSessionId` could get mis-pinned to + * whichever sibling's file happened to be newest on disk, even though + * nothing was actually being respawned. Resolution now happens only inside + * `_pinOmpRespawnId()`, called by a caller that has already confirmed a + * real respawn is happening. */ import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { homedir } from 'node:os'; @@ -35,7 +40,11 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut function seedOmpSessionFile(id: string) { mkdirSync(workingDir, { recursive: true }); mkdirSync(sessionDir, { recursive: true }); - writeFileSync(join(sessionDir, `2026-08-27T17-31-08-001Z_${id}.jsonl`), '{}'); + // resolveAndClaimOmpSessionId() verifies the file's own header (not just + // the filename), mirroring the real `omp` session-file shape — the + // header's `cwd` must match `workingDir` for the candidate to count. + const header = `${JSON.stringify({ type: 'session', id, cwd: workingDir })}\n`; + writeFileSync(join(sessionDir, `2026-08-27T17-31-08-001Z_${id}.jsonl`), header); } it('a brand-new session (no prior mux session) never inherits an on-disk conversation', async () => { @@ -56,8 +65,13 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut expect(session.claudeSessionId).toBe(session.id); }); - it('a reattach to an existing tracked mux session still resolves and pins the real id', async () => { - seedOmpSessionFile('real-omp-uuid'); + it('a plain reattach to an existing mux session (pane still alive) does NOT pin', async () => { + // Regression for the sibling-aliasing bug: pinning must never be a side + // effect of merely building respawn options for a pane that might still + // be alive (isPaneDead is unconditionally false under IS_TEST_MODE, + // which is what a real "just reattaching, nothing died" boot recovery + // looks like from Session's perspective). + seedOmpSessionFile('sibling-conversation-id'); const muxSession: MuxSession = { sessionId: 'placeholder', @@ -81,7 +95,35 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut await session.startInteractive(); const state = session.toState(); - expect(state.ompConfig?.resumeSessionId).toBe('real-omp-uuid'); + expect(state.ompConfig?.resumeSessionId).toBeUndefined(); + expect(session.claudeSessionId).toBe(session.id); + }); + + it('_pinOmpRespawnId() resolves and pins the real id once a respawn is confirmed', () => { + seedOmpSessionFile('real-omp-uuid'); + + const muxSession: MuxSession = { + sessionId: 'placeholder', + muxName: 'codeman-deadbeef', + pid: 1, + createdAt: Date.now(), + workingDir, + mode: 'omp', + attached: false, + }; + + const session = new Session({ + workingDir, + mode: 'omp', + mux: new TmuxManager(), + useMux: true, + muxSession, + }); + sessions.push(session); + + (session as unknown as { _pinOmpRespawnId(): void })._pinOmpRespawnId(); + + expect(session.toState().ompConfig?.resumeSessionId).toBe('real-omp-uuid'); expect(session.claudeSessionId).toBe('real-omp-uuid'); }); });