From ed983f898b12e7622ad6f67f998158d0e6817f97 Mon Sep 17 00:00:00 2001 From: timkjr Date: Thu, 27 Aug 2026 12:50:29 -0500 Subject: [PATCH] fix(omp): resolve claudeSessionId alias on boot-recovery reattach Two bugs compounded to break continuation pinning on every real OMP case (only /tmp-based manual testing happened to work by coincidence): 1. startInteractive() had a second, unconditional claudeSessionId assignment after the mux branch that clobbered its correctly resolved value back to the session's own id on every mux path. 2. mangleOmpWorkingDir() assumed omp mirrors Claude Code's directory naming (home prefix kept), but omp actually strips $HOME first. findLatestOmpSessionId() was silently returning null for every case under ~/codeman-cases/, so resumeSessionId never resolved for any real case dir - only /tmp paths (outside $HOME) worked, which is every dir this feature was previously tested against. Verified live: killed and relaunched the omp-verify server process mid-session (plain reattach, pane stayed alive) and confirmed claudeSessionId now resolves to the real omp transcript uuid instead of the Codeman session's own id. Co-Authored-By: Claude Sonnet 5 --- src/session.ts | 7 +++- src/utils/omp-session-resolver.ts | 21 ++++++++-- test/omp-session-resolver.test.ts | 69 +++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 test/omp-session-resolver.test.ts diff --git a/src/session.ts b/src/session.ts index 413211c4..dfbb8860 100644 --- a/src/session.ts +++ b/src/session.ts @@ -2075,7 +2075,12 @@ export class Session extends EventEmitter { } // Set claudeSessionId — when resuming, the Claude conversation ID is the resumed one. - this._claudeSessionId = this._resumeSessionId || this.id; + // Mirrors the mux branch above and must not clobber it: this line runs + // unconditionally after both the mux and direct-PTY paths, so it also needs + // the ompConfig fallback or it stomps the mux branch's correctly-resolved + // OMP alias back to this.id on every mux/plain-reattach boot recovery + // (the "third reset point" — see DECISIONS.md). + this._claudeSessionId = this._resumeSessionId || this._ompConfig?.resumeSessionId || this.id; this._pid = this.ptyProcess.pid; console.log('[Session] Interactive PTY spawned with PID:', this._pid); diff --git a/src/utils/omp-session-resolver.ts b/src/utils/omp-session-resolver.ts index 37b67a44..d17897e6 100644 --- a/src/utils/omp-session-resolver.ts +++ b/src/utils/omp-session-resolver.ts @@ -17,17 +17,30 @@ import { readdirSync, statSync } from 'node:fs'; import { homedir } from 'node:os'; -import { join } from 'node:path'; +import { join, sep } from 'node:path'; /** A real OMP session file is `_.jsonl`; only the uuid matters here. */ const OMP_SESSION_FILE_PATTERN = /^.+_([a-zA-Z0-9-]+)\.jsonl$/; /** - * Mirrors `omp`'s own directory mangling: every path separator becomes a - * dash. Pure so it's unit-testable without touching the filesystem. + * Mirrors `omp`'s own directory mangling. Confirmed empirically against real + * `~/.omp/agent/sessions/` directory names (2026-08-27): unlike Claude Code's + * `~/.claude/projects/*`, which keeps the home prefix (`-home-user-dev-foo`), + * omp collapses a home-relative workingDir to its home-relative remainder + * FIRST (`/home/user/dev/foo` -> `/dev/foo`) and only then dash-replaces + * (`-dev-foo`) — a path outside $HOME (e.g. `/tmp/...`) is dash-replaced as-is. + * Getting this wrong doesn't error, it just silently returns an empty + * directory listing: findLatestOmpSessionId() below then always falls through + * to null, so continuation pinning quietly degrades to omp's own ambiguous + * `--continue` for every case under $HOME (i.e. virtually all real Codeman + * cases) while appearing to work in `/tmp`-based manual testing. + * Pure so it's unit-testable without touching the filesystem. */ export function mangleOmpWorkingDir(workingDir: string): string { - return workingDir.replace(/\//g, '-'); + const home = homedir(); + const relative = + workingDir === home || workingDir.startsWith(home + sep) ? workingDir.slice(home.length) : workingDir; + return relative.replace(/\//g, '-'); } /** `~/.omp` — no known env override exists (unlike DSH_HOME); revisit if omp adds one. */ diff --git a/test/omp-session-resolver.test.ts b/test/omp-session-resolver.test.ts new file mode 100644 index 00000000..f500c8a2 --- /dev/null +++ b/test/omp-session-resolver.test.ts @@ -0,0 +1,69 @@ +/** + * @fileoverview Tests for OMP session-id resolution from disk. + * + * Pins the home-relative directory mangling bug found 2026-08-27: omp + * collapses a home-relative workingDir to its home-relative remainder BEFORE + * dash-replacing (`/home/user/dev/foo` -> `-dev-foo`), unlike Claude Code's + * `~/.claude/projects/*` convention (`-home-user-dev-foo`) this module was + * originally written to mirror. Getting this wrong doesn't throw — it just + * makes findLatestOmpSessionId() silently return null for every case under + * $HOME (virtually all real Codeman cases), so continuation pinning quietly + * degraded to omp's own ambiguous `--continue` while appearing to work in + * manual testing done entirely under /tmp (which sits outside $HOME and was + * mangled correctly by coincidence). + * + * test/setup.ts gives this file its own temp $HOME, so homedir() below is + * already sandboxed — writing real files under it is safe and exercises the + * exact home-relative path the bug hid behind. + */ +import { mkdirSync, rmSync, utimesSync, writeFileSync } from 'node:fs'; +import { homedir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { findLatestOmpSessionId, mangleOmpWorkingDir } from '../src/utils/omp-session-resolver.js'; + +describe('mangleOmpWorkingDir', () => { + it('strips the home prefix before dash-replacing a home-relative path', () => { + const home = homedir(); + expect(mangleOmpWorkingDir(join(home, 'codeman-cases', 'testcase'))).toBe('-codeman-cases-testcase'); + }); + + it('dash-replaces a path outside $HOME as-is', () => { + expect(mangleOmpWorkingDir('/tmp/omp-verify-case')).toBe('-tmp-omp-verify-case'); + }); + + it('treats workingDir === home as the empty remainder', () => { + expect(mangleOmpWorkingDir(homedir())).toBe(''); + }); + + it('does not false-positive on a sibling directory sharing a prefix with $HOME', () => { + const sibling = `${homedir()}-other/dev/foo`; + expect(mangleOmpWorkingDir(sibling)).toBe(sibling.replace(/\//g, '-')); + }); +}); + +describe('findLatestOmpSessionId', () => { + const sessionDir = join(homedir(), '.omp', 'agent', 'sessions', '-codeman-cases-testcase'); + + afterEach(() => { + rmSync(join(homedir(), '.omp'), { recursive: true, force: true }); + }); + + it('finds the newest session file under a home-relative workingDir', () => { + const workingDir = join(homedir(), 'codeman-cases', 'testcase'); + mkdirSync(sessionDir, { recursive: true }); + writeFileSync(join(sessionDir, '2026-08-27T17-15-57-989Z_older-id.jsonl'), '{}'); + const newer = join(sessionDir, '2026-08-27T17-31-08-001Z_newer-id.jsonl'); + writeFileSync(newer, '{}'); + // Force a deterministic mtime order regardless of filesystem timestamp resolution. + const now = Date.now() / 1000; + utimesSync(join(sessionDir, '2026-08-27T17-15-57-989Z_older-id.jsonl'), now, now); + utimesSync(newer, now + 1, now + 1); + + expect(findLatestOmpSessionId(workingDir)).toBe('newer-id'); + }); + + it('returns null when the mangled directory does not exist', () => { + expect(findLatestOmpSessionId(join(homedir(), 'never-launched'))).toBeNull(); + }); +});