From 4c332c6141e16859d7cb24417ef9e1bc7e09596a Mon Sep 17 00:00:00 2001 From: timkjr Date: Wed, 26 Aug 2026 22:07:16 -0500 Subject: [PATCH] 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). --- src/utils/omp-cli-resolver.ts | 6 +++++- src/web/public/terminal-ui.js | 10 ++++++++++ src/web/routes/session-routes.ts | 27 +++++++++++++++++++++++++-- test/mocks/mock-route-context.ts | 1 + test/routes/session-routes.test.ts | 26 ++++++++++++++++++++++++++ 5 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/utils/omp-cli-resolver.ts b/src/utils/omp-cli-resolver.ts index 5da8e1b9..269b64d2 100644 --- a/src/utils/omp-cli-resolver.ts +++ b/src/utils/omp-cli-resolver.ts @@ -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( { binary: 'omp', diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index a3feb0fc..47182165 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -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(); diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 99498a70..80fef58b 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -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 {}; diff --git a/test/mocks/mock-route-context.ts b/test/mocks/mock-route-context.ts index 7dd4222e..8a1bd884 100644 --- a/test/mocks/mock-route-context.ts +++ b/test/mocks/mock-route-context.ts @@ -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(() => ({})), diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index dbe8efa2..dfdc3505 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -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) ==========