mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-03 22:19:42 +02:00
fix: don't discard codex/gemini/antigravity conversations on Resume; fix DELETE ownership dup + missing broadcast
resumeHistorySession() creates the resumed row in its own mode via a modeConfigKey map (opencode/pi/grok/omp -> continueSession, deepseek -> resumeSession) and retires the old row afterward. codex, gemini and antigravity were missing from that map, so resuming one of their rows started a brand-new session with NO continuation while still deleting the row it came from -- silent data loss dressed as the duplicate-row fix. Gate row retirement on continuesSomething (true only for modes that actually got a continuation config) instead of wiring an unverified sessionId->native-conversation-id assumption for the three affected CLIs. DELETE /api/sessions/:id reimplemented the ownership 404 check inline in two places instead of going through findSessionOrFail, and its persisted-only-session branch never broadcast session:deleted, so other open tabs kept the retired row until their next unrelated fetch. Extract the shared 404 into sessionNotFoundError(), add findPersistedSessionOrFail() alongside findSessionOrFail() in route-helpers.ts (same ownership contract, returns a SessionState instead of a live Session), and use both from the route instead of inline checks. Add the missing broadcast.
This commit is contained in:
@@ -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<string, (...args: unknown[]) => 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<string, (...args: unknown[]) => 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<void>,
|
||||
_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<typeof vi.fn>): 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<typeof vi.fn>): 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<typeof vi.fn> {
|
||||
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<typeof vi.fn>;
|
||||
|
||||
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([]);
|
||||
});
|
||||
});
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user