From 853681f9703ef53609d40cf2d1b69d849b85ba50 Mon Sep 17 00:00:00 2001 From: timkjr Date: Thu, 27 Aug 2026 14:44:46 -0500 Subject: [PATCH] harden(omp): resume-path test coverage, silent-fallback logging, cwd validation Follow-up from a full-branch review pass (Opus) of the omp-mode integration: - Add pinning tests for resolveOmpConfigForCreate() (session-routes.ts), exported to make it testable: the exact "resume this OMP row from history" pipeline that mangleOmpWorkingDir's earlier bug lived in had zero coverage despite being the resolver module's whole reason to exist. - Log a warning when findLatestOmpSessionId() finds nothing on disk and continuation silently degrades to omp's own ambiguous --continue, in both call sites (session create and respawn pinning) - previously silent, making the degradation invisible to anyone debugging it. - Require an absolute cwd before trusting a session file's working directory in omp-transcript.ts's parser, so a corrupted/malformed session file can't point a downstream resume at a relative or empty path. - Document (don't speculatively fix) an unverified symlinked-$HOME edge case in mangleOmpWorkingDir(): the review's suggested realpath() fix assumes omp itself resolves symlinks before mangling, which is unconfirmed - guessing wrong there would trade one silent mismatch for a different one. - Incidental: fixed unrelated pre-existing prettier drift in session-routes.ts (antigravity/opencode dynamic import line-wrapping) that was blocking the pre-commit formatting gate on this file. Confirmed as a non-issue: the model-name regex allowing "/" is intentional (provider/model ids like "crof/glm-5.2" were used successfully in live testing). Co-Authored-By: Claude Sonnet 5 --- src/omp-transcript.ts | 5 ++- src/session.ts | 3 ++ src/utils/omp-session-resolver.ts | 7 ++++ src/web/routes/session-routes.ts | 7 +++- test/omp-session-resolver.test.ts | 59 +++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 2 deletions(-) diff --git a/src/omp-transcript.ts b/src/omp-transcript.ts index 922a61d0..9bf25b6c 100644 --- a/src/omp-transcript.ts +++ b/src/omp-transcript.ts @@ -100,7 +100,10 @@ function parseOmpSessionFile(filePath: string): OmpHistorySession | null { } if (!entry || typeof entry !== 'object') continue; const e = entry as Record; - if (e.type === 'session' && typeof e.id === 'string' && typeof e.cwd === 'string') { + if (e.type === 'session' && typeof e.id === 'string' && typeof e.cwd === 'string' && e.cwd.startsWith('/')) { + // A corrupted or malformed session file could carry a relative or empty + // cwd; requiring an absolute path keeps a downstream resume attempt + // from being pointed at a nonsense working directory. sessionId = e.id; workingDir = e.cwd; } else if (e.type === 'message') { diff --git a/src/session.ts b/src/session.ts index dfbb8860..fb082a9c 100644 --- a/src/session.ts +++ b/src/session.ts @@ -1686,6 +1686,9 @@ export class Session extends EventEmitter { } // 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. + 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 }; } diff --git a/src/utils/omp-session-resolver.ts b/src/utils/omp-session-resolver.ts index d17897e6..f76291ee 100644 --- a/src/utils/omp-session-resolver.ts +++ b/src/utils/omp-session-resolver.ts @@ -37,6 +37,13 @@ const OMP_SESSION_FILE_PATTERN = /^.+_([a-zA-Z0-9-]+)\.jsonl$/; * Pure so it's unit-testable without touching the filesystem. */ export function mangleOmpWorkingDir(workingDir: string): string { + // UNVERIFIED EDGE CASE: if $HOME is itself a symlink, this compares against + // the literal homedir() string, not a realpath()-resolved one. Whether that + // matches omp's own behavior is unconfirmed — we only empirically verified + // omp strips a literal $HOME prefix (2026-08-27), not that it canonicalizes + // symlinks first. Do not "fix" this with realpathSync() without confirming + // omp's actual behavior on a symlinked-home setup; guessing wrong here would + // trade one silent mismatch for a different one. const home = homedir(); const relative = workingDir === home || workingDir.startsWith(home + sep) ? workingDir.slice(home.length) : workingDir; diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 577e7391..7d5a7cf0 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -760,7 +760,7 @@ async function injectAgentSkill(casePath: string): Promise { * dead-pane-respawn path in session.ts does, so even the FIRST relaunch of a * resumed conversation is pinned rather than guessed. */ -function resolveOmpConfigForCreate( +export function resolveOmpConfigForCreate( mode: SessionMode, workingDir: string, ompConfig: OmpConfig | undefined @@ -770,6 +770,11 @@ function resolveOmpConfigForCreate( return ompConfig; } const resolvedId = findLatestOmpSessionId(workingDir); + if (!resolvedId) { + console.warn( + `[Session] OMP: no session file found under ${workingDir} to pin --resume; falling back to ambiguous --continue` + ); + } return resolvedId ? { ...ompConfig, resumeSessionId: resolvedId } : ompConfig; } diff --git a/test/omp-session-resolver.test.ts b/test/omp-session-resolver.test.ts index f500c8a2..ed6d2f60 100644 --- a/test/omp-session-resolver.test.ts +++ b/test/omp-session-resolver.test.ts @@ -21,6 +21,7 @@ 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'; +import { resolveOmpConfigForCreate } from '../src/web/routes/session-routes.js'; describe('mangleOmpWorkingDir', () => { it('strips the home prefix before dash-replacing a home-relative path', () => { @@ -67,3 +68,61 @@ describe('findLatestOmpSessionId', () => { expect(findLatestOmpSessionId(join(homedir(), 'never-launched'))).toBeNull(); }); }); + +describe('resolveOmpConfigForCreate', () => { + // The exact pipeline "resume this OMP row from the history list" drives: + // POST /api/sessions with mode:'omp' + ompConfig:{continueSession:true} + // must come back with resumeSessionId PINNED to the real omp transcript + // uuid, not left as the ambiguous continueSession flag alone. This was the + // one path flagged by review as having zero coverage despite being the + // exact mechanism the whole resolver module exists to serve. + const workingDir = join(homedir(), 'codeman-cases', 'resume-test'); + const sessionDir = join(homedir(), '.omp', 'agent', 'sessions', '-codeman-cases-resume-test'); + + afterEach(() => { + rmSync(join(homedir(), '.omp'), { recursive: true, force: true }); + }); + + it('pins resumeSessionId from disk when resuming with only continueSession set', () => { + mkdirSync(sessionDir, { recursive: true }); + writeFileSync(join(sessionDir, '2026-08-27T17-31-08-001Z_real-omp-uuid.jsonl'), '{}'); + + const resolved = resolveOmpConfigForCreate('omp', workingDir, { continueSession: true }); + + expect(resolved).toEqual({ continueSession: true, resumeSessionId: 'real-omp-uuid' }); + }); + + it('does not attempt resolution when resumeSessionId is already explicit', () => { + mkdirSync(sessionDir, { recursive: true }); + writeFileSync(join(sessionDir, '2026-08-27T17-31-08-001Z_disk-uuid.jsonl'), '{}'); + + const resolved = resolveOmpConfigForCreate('omp', workingDir, { + continueSession: true, + resumeSessionId: 'already-pinned', + }); + + // Must return the caller's id unchanged, never overwrite it with whatever + // happens to be newest on disk. + expect(resolved).toEqual({ continueSession: true, resumeSessionId: 'already-pinned' }); + }); + + it('leaves ompConfig unchanged when continueSession is not set', () => { + const resolved = resolveOmpConfigForCreate('omp', workingDir, {}); + expect(resolved).toEqual({}); + }); + + it('leaves ompConfig unchanged when nothing is on disk to resolve', () => { + const resolved = resolveOmpConfigForCreate('omp', join(homedir(), 'never-launched'), { + continueSession: true, + }); + expect(resolved).toEqual({ continueSession: true }); + }); + + it('returns undefined for a non-omp mode regardless of ompConfig', () => { + expect(resolveOmpConfigForCreate('claude', workingDir, { continueSession: true })).toBeUndefined(); + }); + + it('returns undefined when ompConfig is undefined', () => { + expect(resolveOmpConfigForCreate('omp', workingDir, undefined)).toBeUndefined(); + }); +});