mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-09 00:49:41 +02:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
ed983f898b
commit
853681f970
@@ -100,7 +100,10 @@ function parseOmpSessionFile(filePath: string): OmpHistorySession | null {
|
|||||||
}
|
}
|
||||||
if (!entry || typeof entry !== 'object') continue;
|
if (!entry || typeof entry !== 'object') continue;
|
||||||
const e = entry as Record<string, unknown>;
|
const e = entry as Record<string, unknown>;
|
||||||
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;
|
sessionId = e.id;
|
||||||
workingDir = e.cwd;
|
workingDir = e.cwd;
|
||||||
} else if (e.type === 'message') {
|
} else if (e.type === 'message') {
|
||||||
|
|||||||
@@ -1686,6 +1686,9 @@ export class Session extends EventEmitter {
|
|||||||
}
|
}
|
||||||
// Nothing on disk yet (the dying process never got far enough to write a
|
// 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.
|
// 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 };
|
return { ...this._ompConfig, continueSession: true };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -37,6 +37,13 @@ const OMP_SESSION_FILE_PATTERN = /^.+_([a-zA-Z0-9-]+)\.jsonl$/;
|
|||||||
* Pure so it's unit-testable without touching the filesystem.
|
* Pure so it's unit-testable without touching the filesystem.
|
||||||
*/
|
*/
|
||||||
export function mangleOmpWorkingDir(workingDir: string): string {
|
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 home = homedir();
|
||||||
const relative =
|
const relative =
|
||||||
workingDir === home || workingDir.startsWith(home + sep) ? workingDir.slice(home.length) : workingDir;
|
workingDir === home || workingDir.startsWith(home + sep) ? workingDir.slice(home.length) : workingDir;
|
||||||
|
|||||||
@@ -760,7 +760,7 @@ async function injectAgentSkill(casePath: string): Promise<void> {
|
|||||||
* dead-pane-respawn path in session.ts does, so even the FIRST relaunch of a
|
* dead-pane-respawn path in session.ts does, so even the FIRST relaunch of a
|
||||||
* resumed conversation is pinned rather than guessed.
|
* resumed conversation is pinned rather than guessed.
|
||||||
*/
|
*/
|
||||||
function resolveOmpConfigForCreate(
|
export function resolveOmpConfigForCreate(
|
||||||
mode: SessionMode,
|
mode: SessionMode,
|
||||||
workingDir: string,
|
workingDir: string,
|
||||||
ompConfig: OmpConfig | undefined
|
ompConfig: OmpConfig | undefined
|
||||||
@@ -770,6 +770,11 @@ function resolveOmpConfigForCreate(
|
|||||||
return ompConfig;
|
return ompConfig;
|
||||||
}
|
}
|
||||||
const resolvedId = findLatestOmpSessionId(workingDir);
|
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;
|
return resolvedId ? { ...ompConfig, resumeSessionId: resolvedId } : ompConfig;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -21,6 +21,7 @@ import { homedir } from 'node:os';
|
|||||||
import { join } from 'node:path';
|
import { join } from 'node:path';
|
||||||
import { afterEach, describe, expect, it } from 'vitest';
|
import { afterEach, describe, expect, it } from 'vitest';
|
||||||
import { findLatestOmpSessionId, mangleOmpWorkingDir } from '../src/utils/omp-session-resolver.js';
|
import { findLatestOmpSessionId, mangleOmpWorkingDir } from '../src/utils/omp-session-resolver.js';
|
||||||
|
import { resolveOmpConfigForCreate } from '../src/web/routes/session-routes.js';
|
||||||
|
|
||||||
describe('mangleOmpWorkingDir', () => {
|
describe('mangleOmpWorkingDir', () => {
|
||||||
it('strips the home prefix before dash-replacing a home-relative path', () => {
|
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();
|
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();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user