fix(omp): retire the old row on resume, and let DELETE remove persisted-only sessions

Every non-claude "Resume" click creates a brand-new Codeman session
(there is no id to reattach to), but the old row was never cleaned up
-- click resume on the same conversation a few times and the session
list fills up with duplicate rows sharing one name. resumeHistorySession
now retires the row it resumed from after the new one starts.

That retirement needs DELETE to actually work on a row that was never
live in the first place (the normal case for anything showing up in
"Resume Conversation"): findSessionOrFail only checks the in-memory
live-session map, so DELETE 404s on a persisted-only entry today. Give
the route a fallback: when the id isn't live, look it up in persisted
state instead and demote/remove it there (respecting the existing
pinned-session protection). Verified live against a real persisted-only
row via the API, and added route-test coverage for both the success
and still-truly-unknown-id cases (which needed a demoteOrRemoveSession
mock the route harness didn't have).

Also includes an unrelated pre-existing prettier drift fix picked up
by npm run format (omp-cli-resolver.ts, antigravity/opencode import
wrapping in session-routes.ts).
This commit is contained in:
timkjr
2026-08-28 11:32:30 -05:00
parent 253599ce9c
commit 4c332c6141
5 changed files with 67 additions and 3 deletions
+5 -1
View File
@@ -76,7 +76,11 @@ function probeOmpVersion(binPath: string): string | null {
type OmpVersionProbe = (binPath: string) => string | null;
function createOmpResolver(host?: CliResolverHost, versionProbe: OmpVersionProbe = probeOmpVersion, now?: () => number) {
function createOmpResolver(
host?: CliResolverHost,
versionProbe: OmpVersionProbe = probeOmpVersion,
now?: () => number
) {
return createCliExecutableResolver<string>(
{
binary: 'omp',
+10
View File
@@ -2969,6 +2969,16 @@ Object.assign(CodemanApp.prototype, {
// Start interactive
await fetch(`/api/sessions/${newSessionId}/interactive`, { method: 'POST' });
// Retire the row being resumed: a non-claude "resume" is really a brand
// new Codeman session pointed at the same directory (there is no id to
// reattach to), so without this every resume leaves the old row behind
// as a duplicate — click it 3 times, see the same name 3 times. Claude
// rows are left alone: `sessionId` there is a claudeSessionId, which
// usually has no live/persisted Codeman session of its own to delete.
if (effectiveMode !== 'claude' && sessionId !== newSessionId) {
fetch(`/api/sessions/${sessionId}?killMux=true`, { method: 'DELETE' }).catch(() => {});
}
this.terminal.writeln(`\x1b[90m Session ${name} ready\x1b[0m`);
await this.selectSession(newSessionId);
this.terminal.focus();
+25 -2
View File
@@ -1134,8 +1134,31 @@ export function registerSessionRoutes(
const query = req.query as { killMux?: string };
const killMux = query.killMux !== 'false'; // Default to true
// Security: owner-scoped lookup 404s foreign/missing sessions uniformly (no existence leak, no cross-user kill).
const session = findSessionOrFail(ctx, id, req);
// A resumed/detached-but-never-live row (e.g. a non-claude "Resume" that
// relaunched into a NEW session and wants to retire the old one it can no
// longer reattach to) has no entry in ctx.sessions at all — only in
// persisted state. Fall back to removing that persisted record directly
// rather than 404ing: the caller means "make this row go away", and a
// stale duplicate row is exactly what's left behind otherwise. Pinned
// sessions keep their existing demote-not-delete protection.
const session = ctx.sessions.get(id);
if (!session) {
const persisted = ctx.store.getSession(id);
if (!persisted || !canAccessOwned(getAuthUser(req), persisted.owner)) {
throw Object.assign(new Error(`Session ${id} not found`), {
statusCode: 404,
body: createErrorResponse(ApiErrorCode.NOT_FOUND, `Session ${id} not found`),
});
}
ctx.store.demoteOrRemoveSession(id);
return {};
}
if (req && !canAccessOwned(getAuthUser(req), session.owner)) {
throw Object.assign(new Error(`Session ${id} not found`), {
statusCode: 404,
body: createErrorResponse(ApiErrorCode.NOT_FOUND, `Session ${id} not found`),
});
}
await ctx.cleanupSession(session.id, killMux, 'user_delete');
return {};
+1
View File
@@ -86,6 +86,7 @@ export function createMockRouteContext(options?: {
getSession: vi.fn(),
setSession: vi.fn(),
removeSession: vi.fn(),
demoteOrRemoveSession: vi.fn(() => 'removed' as const),
getSettings: vi.fn(() => ({})),
setSettings: vi.fn(),
getRalphLoopState: vi.fn(() => ({})),
+26
View File
@@ -341,6 +341,32 @@ describe('session-routes', () => {
const body = JSON.parse(res.body);
expect(body.success).toBe(false);
});
it('removes a persisted-only session (not live) via the state store, without touching cleanupSession', async () => {
vi.mocked(harness.ctx.store.getSession).mockReturnValueOnce({
id: 'ghost-session',
owner: undefined,
} as never);
const res = await harness.app.inject({
method: 'DELETE',
url: '/api/sessions/ghost-session',
});
expect(res.statusCode).toBe(200);
const body = JSON.parse(res.body);
expect(body.success).toBe(true);
expect(harness.ctx.store.demoteOrRemoveSession).toHaveBeenCalledWith('ghost-session');
expect(harness.ctx.cleanupSession).not.toHaveBeenCalled();
});
it('404s a persisted-only session id the state store does not recognize either', async () => {
vi.mocked(harness.ctx.store.getSession).mockReturnValueOnce(null);
const res = await harness.app.inject({
method: 'DELETE',
url: '/api/sessions/truly-nonexistent',
});
expect(res.statusCode).toBe(404);
expect(harness.ctx.store.demoteOrRemoveSession).not.toHaveBeenCalled();
});
});
// ========== DELETE /api/sessions (delete all) ==========