diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 47182165..bb83cc5a 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -2942,12 +2942,19 @@ Object.assign(CodemanApp.prototype, { grok: 'grokConfig', omp: 'ompConfig', }[effectiveMode]; + // codex/gemini/antigravity have no wired continuation here yet (their + // configs use an exact conversation id, not a "continue most recent" + // flag, and the row's own `sessionId` is not verified to carry that + // id for these three modes) — `continuesSomething` below is what keeps + // their row from being retired for a resume that didn't actually + // continue anything. const modeConfig = modeConfigKey ? { [modeConfigKey]: { continueSession: true } } : effectiveMode === 'deepseek' ? { deepSeekConfig: { resumeSession: true } } : {}; + const continuesSomething = Boolean(modeConfigKey) || effectiveMode === 'deepseek'; const createRes = await fetch('/api/sessions', { method: 'POST', headers: { 'Content-Type': 'application/json' }, @@ -2975,7 +2982,12 @@ Object.assign(CodemanApp.prototype, { // 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) { + // Gated on `continuesSomething`: for codex/gemini/antigravity (no + // continuation wired above), this is really a FRESH session with no + // relation to the old row's conversation, so retiring it would discard + // the old conversation with no recovery — worse than the duplicate row + // this guard exists to prevent for the modes that DO continue. + if (effectiveMode !== 'claude' && continuesSomething && sessionId !== newSessionId) { fetch(`/api/sessions/${sessionId}?killMux=true`, { method: 'DELETE' }).catch(() => {}); } diff --git a/src/web/route-helpers.ts b/src/web/route-helpers.ts index 8585358c..fb02fd61 100644 --- a/src/web/route-helpers.ts +++ b/src/web/route-helpers.ts @@ -12,7 +12,7 @@ import { homedir } from 'node:os'; import type { z } from 'zod'; import type { FastifyReply, FastifyRequest } from 'fastify'; import { Session } from '../session.js'; -import { ApiErrorCode, createErrorResponse, type AuthUser } from '../types.js'; +import { ApiErrorCode, createErrorResponse, type AuthUser, type SessionState } from '../types.js'; import { MAX_CONCURRENT_SESSIONS } from '../config/map-limits.js'; import { parseRalphLoopConfig, extractCompletionPhrase } from '../ralph-config.js'; import { SseEvent } from './sse-events.js'; @@ -264,6 +264,18 @@ export function revokeUserSessions( return removed; } +/** + * The 404 both session-lookup helpers below throw. A missing session and one + * the caller isn't allowed to see get the IDENTICAL error (never 403), so + * existence of another user's session is never leaked. + */ +function sessionNotFoundError(sessionId: string): Error & { statusCode: number; body: unknown } { + return Object.assign(new Error(`Session ${sessionId} not found`), { + statusCode: 404, + body: createErrorResponse(ApiErrorCode.NOT_FOUND, `Session ${sessionId} not found`), + }); +} + /** * Look up a session by ID or throw a structured error. * Replaces the pattern: `const session = sessions.get(id); if (!session) return createErrorResponse(...)`. @@ -274,15 +286,31 @@ export function revokeUserSessions( */ export function findSessionOrFail(ctx: SessionPort, sessionId: string, req?: FastifyRequest): Session { const session = ctx.sessions.get(sessionId); - if (!session || (req && !canAccessOwned(getAuthUser(req), session.owner))) { - throw Object.assign(new Error(`Session ${sessionId} not found`), { - statusCode: 404, - body: createErrorResponse(ApiErrorCode.NOT_FOUND, `Session ${sessionId} not found`), - }); - } + if (!session) throw sessionNotFoundError(sessionId); + if (req && !canAccessOwned(getAuthUser(req), session.owner)) throw sessionNotFoundError(sessionId); return session; } +/** + * Like {@link findSessionOrFail}, for a session that exists ONLY in persisted + * state — a resumed-but-never-reattached row (e.g. a non-claude "Resume" that + * relaunched into a new session and wants to retire the row it can no longer + * reattach to) has no live `Session` instance for `findSessionOrFail` to + * return, so this returns the persisted record instead. Same ownership + * enforcement, same 404-not-403 leak protection — this is that function's + * missing other half, not a separate check reimplemented inline. + */ +export function findPersistedSessionOrFail( + store: { getSession(id: string): SessionState | null }, + sessionId: string, + req?: FastifyRequest +): SessionState { + const persisted = store.getSession(sessionId); + if (!persisted) throw sessionNotFoundError(sessionId); + if (req && !canAccessOwned(getAuthUser(req), persisted.owner)) throw sessionNotFoundError(sessionId); + return persisted; +} + /** Shortest prefix accepted for a parent session id (see resolveParentSessionId). */ const PARENT_SESSION_ID_MIN_PREFIX = 8; diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index fe50bd92..9aa0736b 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -67,6 +67,7 @@ import { autoConfigureRalph, canAccessOwned, CASES_DIR, + findPersistedSessionOrFail, findSessionOrFail, getAuthUser, isAdmin, @@ -1191,25 +1192,19 @@ export function registerSessionRoutes( // 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`), - }); - } + if (!ctx.sessions.has(id)) { + // Called for its existence/ownership 404 side effect only — demoteOrRemoveSession + // below re-looks-up the record by id, so the returned SessionState is unused here. + findPersistedSessionOrFail(ctx.store, id, req); ctx.store.demoteOrRemoveSession(id); + // Mirrors the broadcast at the tail of the live-session cleanup path + // (_doCleanupSession in server.ts) — without it, other open tabs keep + // showing the retired row until their next unrelated fetch. + ctx.broadcast(SseEvent.SessionDeleted, { 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`), - }); - } + const session = findSessionOrFail(ctx, id, req); await ctx.cleanupSession(session.id, killMux, 'user_delete'); return {}; }); diff --git a/test/resume-history-mode-fidelity.test.ts b/test/resume-history-mode-fidelity.test.ts new file mode 100644 index 00000000..85619ae4 --- /dev/null +++ b/test/resume-history-mode-fidelity.test.ts @@ -0,0 +1,143 @@ +/** + * @fileoverview Upstream review fix (Ark0N/Codeman#353, PR #3): resumeHistorySession() + * threads the row's own mode through session creation via a `modeConfigKey` map + * (opencode/pi/grok/omp → `continueSession: true`), then retires the old row via + * DELETE. codex/gemini/antigravity were missing from that map, so resuming one of + * their rows created a session with NO continuation while still deleting the row + * it came from — data loss dressed as a fix. The correction: only retire the row + * when the new session actually continues something. + * + * Loaded via `vm` against a stub CodemanApp, same harness as resume-name.test.ts. + * `fetch` is a shared mutable stub so each test can inspect exactly which requests + * fired without a real network/server. + */ + +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/* eslint-disable @typescript-eslint/no-explicit-any */ + +/** The fetch the vm's shipping code calls; swapped per test (see beforeEach). */ +let currentFetch: (...args: unknown[]) => unknown = () => { + throw new Error('fetch not stubbed for this test'); +}; + +function loadTerminalUiPrototype(): Record unknown> { + const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/terminal-ui.js'), 'utf8'); + const context = vm.createContext({ + console, + CodemanApp: class CodemanApp {}, + setInterval: vi.fn(), + clearInterval: vi.fn(), + setTimeout, + clearTimeout, + requestAnimationFrame: vi.fn(), + document: { addEventListener: vi.fn(), getElementById: vi.fn(() => null) }, + window: { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + fetch: (...args: unknown[]) => currentFetch(...args), + }); + vm.runInContext(`${source}\nglobalThis.__proto = CodemanApp.prototype;`, context); + return (context as { __proto: Record unknown> }).__proto; +} + +const proto = loadTerminalUiPrototype(); + +function makeApp() { + return { + terminal: { clear: vi.fn(), writeln: vi.fn(), focus: vi.fn() }, + cases: [], + resumeHistorySession: proto.resumeHistorySession as (...args: unknown[]) => Promise, + _closeFolderHistoryModal: vi.fn(), + _resolveResumeName: () => 'w1-case', + loadAppSettingsFromStorage: () => ({}), + getCaseSettings: () => ({}), + buildEnvOverrides: () => ({}), + getEffortSetting: () => undefined, + selectSession: vi.fn(async () => {}), + }; +} + +/** DELETE calls the fetch mock recorded. */ +function deleteCalls(fetchMock: ReturnType): string[] { + return fetchMock.mock.calls + .filter(([, opts]: [string, { method?: string }]) => opts?.method === 'DELETE') + .map(([url]: [string]) => url); +} + +/** POST /api/sessions body the fetch mock recorded. */ +function createBody(fetchMock: ReturnType): any { + const call = fetchMock.mock.calls.find(([url]: [string]) => url === '/api/sessions'); + return call ? JSON.parse((call[1] as { body: string }).body) : undefined; +} + +function stubFetch(newSessionId: string): ReturnType { + const fetchMock = vi.fn(async (url: string) => { + if (url === '/api/sessions') { + return { json: async () => ({ success: true, data: { session: { id: newSessionId } } }) }; + } + return { json: async () => ({ success: true }) }; + }); + currentFetch = fetchMock; + return fetchMock; +} + +describe('resumeHistorySession: row retirement is gated on actual continuation', () => { + let fetchMock: ReturnType; + + beforeEach(() => { + fetchMock = stubFetch('new-session-id'); + }); + + it.each(['codex', 'gemini', 'antigravity'])( + 'does NOT retire the old row for %s (no continuation is wired for it)', + async (mode) => { + const app = makeApp(); + await app.resumeHistorySession.call(app, 'old-id', '/repo', 'w1-repo', mode); + + expect(createBody(fetchMock)).toMatchObject({ mode }); + expect(createBody(fetchMock).codexConfig).toBeUndefined(); + expect(createBody(fetchMock).geminiConfig).toBeUndefined(); + expect(createBody(fetchMock).antigravityConfig).toBeUndefined(); + expect(deleteCalls(fetchMock)).toEqual([]); + } + ); + + it.each([ + ['opencode', 'openCodeConfig'], + ['pi', 'piConfig'], + ['grok', 'grokConfig'], + ['omp', 'ompConfig'], + ])('retires the old row for %s (continueSession is wired via %s)', async (mode, configKey) => { + const app = makeApp(); + await app.resumeHistorySession.call(app, 'old-id', '/repo', 'w1-repo', mode); + + expect(createBody(fetchMock)[configKey]).toEqual({ continueSession: true }); + expect(deleteCalls(fetchMock)).toEqual(['/api/sessions/old-id?killMux=true']); + }); + + it('retires the old row for deepseek (resumeSession is wired)', async () => { + const app = makeApp(); + await app.resumeHistorySession.call(app, 'old-id', '/repo', 'w1-repo', 'deepseek'); + + expect(createBody(fetchMock).deepSeekConfig).toEqual({ resumeSession: true }); + expect(deleteCalls(fetchMock)).toEqual(['/api/sessions/old-id?killMux=true']); + }); + + it('never retires a claude row (resumeSessionId is a claudeSessionId, not a Codeman row id)', async () => { + const app = makeApp(); + await app.resumeHistorySession.call(app, 'claude-uuid', '/repo', 'w1-repo', 'claude'); + + expect(createBody(fetchMock)).toMatchObject({ mode: 'claude', resumeSessionId: 'claude-uuid' }); + expect(deleteCalls(fetchMock)).toEqual([]); + }); + + it('never retires when the new session id equals the old one (no-op resume)', async () => { + fetchMock = stubFetch('same-id'); + const app = makeApp(); + await app.resumeHistorySession.call(app, 'same-id', '/repo', 'w1-repo', 'omp'); + + expect(deleteCalls(fetchMock)).toEqual([]); + }); +}); diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index dfdc3505..839786ad 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -356,6 +356,10 @@ describe('session-routes', () => { expect(body.success).toBe(true); expect(harness.ctx.store.demoteOrRemoveSession).toHaveBeenCalledWith('ghost-session'); expect(harness.ctx.cleanupSession).not.toHaveBeenCalled(); + // Ark0N/Codeman#353 review: the persisted-only branch used to demote/remove + // with no broadcast, so other open tabs kept showing the retired row until + // their next unrelated fetch. + expect(harness.ctx.broadcast).toHaveBeenCalledWith('session:deleted', { id: 'ghost-session' }); }); it('404s a persisted-only session id the state store does not recognize either', async () => {