diff --git a/apps/server/src/core/ai-chat/ai-chat.parts-resilience.spec.ts b/apps/server/src/core/ai-chat/ai-chat.parts-resilience.spec.ts new file mode 100644 index 00000000..d0b66432 --- /dev/null +++ b/apps/server/src/core/ai-chat/ai-chat.parts-resilience.spec.ts @@ -0,0 +1,142 @@ +// #489 — client-parts validation + resilient history conversion. +// +// These unit tests exercise the two exported helpers against the REAL +// `convertToModelMessages` from `ai` (NOT a mock): a genuinely malformed part +// (a `null` element inside a parts array) makes the real converter throw +// ("Cannot read properties of null"), which is the actual production +// "bricked chat" mechanism this fix defends against. Asserting against the real +// converter (rather than a mock-shaped error) is the whole point — a mock would +// hide a version change in the converter's throw behaviour. +import { convertToModelMessages, type UIMessage } from 'ai'; +import { + sanitizeUserParts, + convertHistoryResilient, + TOOL_CONTEXT_OMITTED_MARKER, +} from './ai-chat.service'; + +type Row = Omit & { id: string }; + +describe('sanitizeUserParts (#489, branch: validation on receipt)', () => { + it('keeps whitelisted text parts unchanged', () => { + const drops: string[] = []; + const out = sanitizeUserParts( + [ + { type: 'text', text: 'a' }, + { type: 'text', text: 'b' }, + ] as UIMessage['parts'], + (t) => drops.push(t), + ); + expect(out).toEqual([ + { type: 'text', text: 'a' }, + { type: 'text', text: 'b' }, + ]); + expect(drops).toEqual([]); + }); + + it('drops a non-text part (a tool-part in input-available) and reports its type', () => { + const drops: string[] = []; + const out = sanitizeUserParts( + [ + { type: 'text', text: 'hi' }, + { + type: 'tool-getPage', + toolCallId: 't1', + state: 'input-available', + input: { pageId: 'p' }, + }, + ] as unknown as UIMessage['parts'], + (t) => drops.push(t), + ); + expect(out).toEqual([{ type: 'text', text: 'hi' }]); + expect(drops).toEqual(['tool-getPage']); + }); + + it('drops a null part (the shape that would poison convertToModelMessages)', () => { + const drops: string[] = []; + const out = sanitizeUserParts( + [{ type: 'text', text: 'hi' }, null] as unknown as UIMessage['parts'], + (t) => drops.push(t), + ); + expect(out).toEqual([{ type: 'text', text: 'hi' }]); + expect(drops).toEqual(['(unknown)']); + }); + + it('returns undefined when nothing survives (so a null metadata is persisted)', () => { + const out = sanitizeUserParts( + [ + { type: 'tool-x', toolCallId: 't', state: 'input-available' }, + ] as unknown as UIMessage['parts'], + () => undefined, + ); + expect(out).toBeUndefined(); + }); + + it('returns undefined for a non-array input', () => { + expect( + sanitizeUserParts(undefined as unknown as UIMessage['parts'], () => undefined), + ).toBeUndefined(); + }); +}); + +describe('convertHistoryResilient (#489, branches: happy + per-row degradation)', () => { + it('happy path: healthy history converts identically to convertToModelMessages, no degrade', async () => { + const history: Row[] = [ + { id: 'u1', role: 'user', parts: [{ type: 'text', text: 'hi' }] }, + { id: 'a1', role: 'assistant', parts: [{ type: 'text', text: 'hello' }] }, + ]; + const degrades: number[] = []; + const out = await convertHistoryResilient(history, (i) => degrades.push(i)); + const expected = await convertToModelMessages(history as UIMessage[]); + expect(out).toEqual(expected); + expect(degrades).toEqual([]); + }); + + it('REAL poison: a null part throws in the batch converter but is isolated and degraded to a marker', async () => { + // Sanity: the real converter genuinely throws on this shape. + const poisoned: Row = { + id: 'a1', + role: 'assistant', + parts: [ + { type: 'text', text: 'earlier answer' }, + null, + ] as unknown as UIMessage['parts'], + }; + await expect( + convertToModelMessages([poisoned as UIMessage]), + ).rejects.toThrow(); + + const history: Row[] = [ + { id: 'u1', role: 'user', parts: [{ type: 'text', text: 'first' }] }, + poisoned, + { id: 'u2', role: 'user', parts: [{ type: 'text', text: 'second' }] }, + ]; + const degrades: number[] = []; + const out = await convertHistoryResilient(history, (i) => degrades.push(i)); + + // Only the poisoned row (index 1) is degraded. + expect(degrades).toEqual([1]); + // Healthy rows survive verbatim. + const flat = JSON.stringify(out); + expect(flat).toContain('first'); + expect(flat).toContain('second'); + // The degraded row carries its readable text AND the truncation marker so the + // model sees that tool context was omitted (never a silent loss). + expect(flat).toContain('earlier answer'); + expect(flat).toContain(TOOL_CONTEXT_OMITTED_MARKER); + // The whole batch converted (3 model messages, none dropped). + expect(out).toHaveLength(3); + }); + + it('a fully-poisoned row (no readable text) still degrades to just the marker', async () => { + const history: Row[] = [ + { + id: 'a1', + role: 'assistant', + parts: [null] as unknown as UIMessage['parts'], + }, + ]; + const out = await convertHistoryResilient(history, () => undefined); + expect(out).toHaveLength(1); + expect(JSON.stringify(out)).toContain(TOOL_CONTEXT_OMITTED_MARKER); + }); +}); diff --git a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts index ca9511ef..8dd32123 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts @@ -1440,7 +1440,7 @@ describe('AiChatService.stream — resumable pipe options (#184 phase 1.5)', () } // Wire only the deps reached on the way to the pipe call, plus a spy registry. - function makeService(opts: { resumable: boolean }) { + function makeService(opts: { resumable: boolean; history?: unknown[] }) { const aiChatRepo = { findById: jest.fn(async () => ({ id: 'chat-1', workspaceId: 'ws-1' })), insert: jest.fn(), @@ -1448,7 +1448,7 @@ describe('AiChatService.stream — resumable pipe options (#184 phase 1.5)', () const aiChatMessageRepo = { // Both the user insert and the assistant seed return the same row id. insert: jest.fn(async () => ({ id: 'msg-1' })), - findAllByChat: jest.fn(async () => []), + findAllByChat: jest.fn(async () => opts.history ?? []), update: jest.fn(async () => ({ id: 'msg-1' })), // #487: the terminal owner-write + the opportunistic reconcile query. finalizeOwner: jest.fn(async () => ({ id: 'msg-1' })), @@ -1487,7 +1487,7 @@ describe('AiChatService.stream — resumable pipe options (#184 phase 1.5)', () } as never, streamRegistry as never, ); - return { svc, streamRegistry }; + return { svc, streamRegistry, aiChatMessageRepo }; } const body = { @@ -1570,6 +1570,86 @@ describe('AiChatService.stream — resumable pipe options (#184 phase 1.5)', () await expect(drive(svc, makeRunHooks())).rejects.toThrow('boom'); expect(streamRegistry.abortEntry).toHaveBeenCalledWith('chat-1', 'run-1'); }); + + // #489 REGRESSION (against the REAL convertToModelMessages — not mocked here): + // a persisted history row whose parts contain a `null` element makes the real + // convertToModelMessages THROW ("Cannot read properties of null"). Pre-fix that + // 500-ed every turn forever and each retry appended a duplicate user row. The + // fix converts BEFORE the insert and isolates the poisoned row per-row, degrading + // it to text with a "[tool context omitted]" marker. Assert the turn still runs, + // the marker reaches the model, and exactly ONE user row is inserted. + it('#489: a poisoned OLD-history row keeps the chat working; the marker reaches the model; one user insert', async () => { + const { svc, aiChatMessageRepo } = makeService({ + resumable: false, + history: [ + { + id: 'old-1', + role: 'assistant', + content: 'earlier answer', + // A null part is the poison: rowToUiMessage keeps it (the array is + // non-empty) and the real convertToModelMessages throws on it. + metadata: { parts: [{ type: 'text', text: 'earlier answer' }, null] }, + status: 'completed', + }, + ], + }); + // Must NOT throw — the poisoned row is degraded, not fatal. + await drive(svc, makeRunHooks()); + expect(streamTextMock).toHaveBeenCalledTimes(1); + const passedMessages = streamTextMock.mock.calls[0][0].messages; + const serialized = JSON.stringify(passedMessages); + // The model sees the truncation marker (silent tool-context loss is not ok) + // AND the row's readable text is preserved alongside it. + expect(serialized).toContain('[tool context omitted]'); + expect(serialized).toContain('earlier answer'); + // Exactly ONE user row inserted (no duplicate), inserted AFTER conversion. + const userInserts = aiChatMessageRepo.insert.mock.calls + .map((c: unknown[]) => c[0] as { role?: string }) + .filter((r) => r.role === 'user'); + expect(userInserts).toHaveLength(1); + }); + + // #489: client-supplied non-text parts (a tool-part in `input-available`, the + // exact "bricking" payload) are dropped ON RECEIPT — never persisted — so they + // can never poison future turns. Only the text survives into metadata.parts. + it('#489: a non-text client part is stripped before persist (only text survives)', async () => { + const { svc, aiChatMessageRepo } = makeService({ resumable: false }); + await svc.stream({ + user: { id: 'u1' } as never, + workspace: { id: 'ws-1' } as never, + sessionId: 's1', + body: { + chatId: 'chat-1', + messages: [ + { + id: 'm1', + role: 'user', + parts: [ + { type: 'text', text: 'hello' }, + // untrusted tool-part — must be dropped, never persisted + { + type: 'tool-getPage', + toolCallId: 't1', + state: 'input-available', + input: { pageId: 'p' }, + }, + ], + }, + ], + } as never, + res: makeRes() as never, + signal: new AbortController().signal, + model: {} as never, + role: null, + runHooks: makeRunHooks() as never, + }); + const userInsert = aiChatMessageRepo.insert.mock.calls + .map((c: unknown[]) => c[0] as { role?: string; metadata?: unknown }) + .find((r) => r.role === 'user'); + const parts = (userInsert?.metadata as { parts?: Array<{ type: string }> }) + ?.parts; + expect(parts).toEqual([{ type: 'text', text: 'hello' }]); + }); }); /** diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index 02ecc9d1..66d21473 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -14,6 +14,7 @@ import { convertToModelMessages, stepCountIs, type UIMessage, + type ModelMessage, type LanguageModel, } from 'ai'; import { AiService } from '../../integrations/ai/ai.service'; @@ -1042,7 +1043,58 @@ export class AiChatService implements OnModuleInit, OnModuleDestroy { const incoming = lastUserMessage(body.messages); const incomingText = uiMessageText(incoming); - // Persist the user message before contacting the model. + // #489: sanitize client-supplied parts ON RECEIPT. The client only ever + // sends `sendMessage({ text })` (a single text part); there is no + // file/attachment path. Any other part — most dangerously a tool-part in + // `input-available` state — is untrusted data that, once persisted to + // `metadata.parts` verbatim, is REPLAYED through convertToModelMessages on + // every later turn. A malformed tool-part makes that conversion throw, + // 500-ing every future turn of the chat forever ("bricked"). Drop any + // non-whitelisted part with a warn. + const sanitizedParts = sanitizeUserParts(incoming?.parts, (type) => + this.logger.warn( + `Dropping unsupported user message part '${type}' on chat ${chatId}`, + ), + ); + + // #489: rebuild the conversation from persisted history (not the client + // payload) and CONVERT it to model messages BEFORE persisting the user row. + // Load the OLD history (WITHOUT the new row) and append the incoming turn in + // memory for the conversion. This makes the insert happen only after a + // successful conversion, so a conversion failure cannot leave a DUPLICATE + // user row behind on the client's retry (the "bricked chat" that accreted a + // dup on every 500). `findAllByChat` returns chronological order (oldest -> + // newest) and keeps a 5000-row memory-safety backstop (on overflow it keeps + // the NEWEST rows and logs a warning); that is a safety net far above any + // realistic chat, not a conversational limit. + const oldHistory = await this.aiChatMessageRepo.findAllByChat( + chatId, + workspace.id, + ); + const uiMessages: Array & { id: string }> = [ + ...oldHistory.map(rowToUiMessage), + { + id: 'pending-user', + role: 'user', + parts: (sanitizedParts && sanitizedParts.length > 0 + ? sanitizedParts + : textPart(incomingText)) as UIMessage['parts'], + }, + ]; + // convertToModelMessages is async in ai@6.0.134 (returns Promise). + // Resilient (#489): a single poisoned row in the OLD history is isolated via + // per-row conversion and degraded to plain text with a "[tool context + // omitted]" marker rather than 500-ing the whole turn (silent loss of tool + // context is not acceptable — the model must see the truncation). + const messages = await convertHistoryResilient(uiMessages, (index, err) => + this.logger.warn( + `Degraded unconvertible history row ${index} on chat ${chatId} to text: ${ + err instanceof Error ? err.message : 'unknown error' + }`, + ), + ); + + // Persist the user message only AFTER a successful conversion (#489). await this.aiChatMessageRepo.insert({ chatId, workspaceId: workspace.id, @@ -1050,31 +1102,21 @@ export class AiChatService implements OnModuleInit, OnModuleDestroy { role: 'user', content: incomingText, // jsonb column: UIMessage parts are JSON-serializable at runtime but not - // structurally `JsonValue`, so cast through unknown. - metadata: (incoming?.parts ? { parts: incoming.parts } : null) as never, + // structurally `JsonValue`, so cast through unknown. Persist the SANITIZED + // parts (never the raw client parts) so the row is always convertible. + metadata: (sanitizedParts ? { parts: sanitizedParts } : null) as never, }); - // Rebuild the conversation from persisted history (not the client payload), - // so the model always sees the authoritative server-side transcript. Load - // the FULL history in chronological order (oldest -> newest, incl. the user - // message just inserted above) so NO turns are dropped — there is no - // recent-tail window anymore. `findAllByChat` keeps a 5000-row memory-safety - // backstop (on overflow it keeps the NEWEST rows and logs a warning); that - // is a safety net far above any realistic chat, not a conversational limit. - const history = await this.aiChatMessageRepo.findAllByChat( - chatId, - workspace.id, - ); - const uiMessages = history.map(rowToUiMessage); - // convertToModelMessages is async in ai@6.0.134 (returns Promise). - const messages = await convertToModelMessages(uiMessages); - // Interrupt-resume detection (#198): the client "send now" flag is only a // hint — confirm it against the persisted history (the preceding assistant // turn must really be aborted/streaming) so a spoofed flag cannot inject the // interrupt note onto an ordinary turn. The partial output the model needs is // already in `messages` (the aborted assistant row replays via findRecent). - const interrupted = isInterruptResume(history, body.interrupted); + // Append the new user turn (shape-only) so index -2 is the prior assistant. + const interrupted = isInterruptResume( + [...oldHistory, { role: 'user', status: null, metadata: null }], + body.interrupted, + ); // Per-turn page-change detection (#274): if the open page was hand-edited by // the user since the agent's last turn ended, compute the unified diff so the @@ -2074,6 +2116,82 @@ function textPart(text: string): Array<{ type: 'text'; text: string }> { return text ? [{ type: 'text', text }] : []; } +/** + * Part types accepted on an INCOMING user turn (#489). The client only ever + * sends `sendMessage({ text })` (a single text part); there is no file/attachment + * path. Everything else on a client-supplied user message — most dangerously a + * tool-part in `input-available` state — is untrusted data that would be + * persisted to `metadata.parts` verbatim and replayed through + * `convertToModelMessages` on every later turn, potentially bricking the chat. + */ +const ALLOWED_USER_PART_TYPES: ReadonlySet = new Set(['text']); + +/** + * Keep only whitelisted parts on a client-supplied user message; report each + * dropped part's type via `onDrop` (the caller warns). Returns `undefined` when + * nothing survives (no parts / none whitelisted), so the caller persists a null + * metadata rather than an empty-parts object. Never throws. + */ +export function sanitizeUserParts( + parts: UIMessage['parts'] | undefined, + onDrop: (type: string) => void, +): UIMessage['parts'] | undefined { + if (!Array.isArray(parts)) return undefined; + const kept = parts.filter((p) => { + const type = + typeof (p as { type?: unknown })?.type === 'string' + ? (p as { type: string }).type + : ''; + if (ALLOWED_USER_PART_TYPES.has(type)) return true; + onDrop(type || '(unknown)'); + return false; + }); + return kept.length > 0 ? (kept as UIMessage['parts']) : undefined; +} + +/** Marker for a history row whose tool parts could not be replayed (#489). */ +export const TOOL_CONTEXT_OMITTED_MARKER = '[tool context omitted]'; + +/** + * Convert persisted UI history to model messages, tolerating a single poisoned + * row (#489). `convertToModelMessages` over the WHOLE array throws if ANY row is + * malformed (e.g. a tool-part left unbalanced / in `input-available` state), + * which would otherwise 500 every turn of the chat forever. On a batch failure we + * fall back to per-row conversion so the bad row is isolated: it is degraded to + * plain text carrying its readable text plus a `[tool context omitted]` marker + * (the model MUST see that its tool context was truncated — silent loss is not + * acceptable), while every healthy row converts normally. Because AI SDK v6 + * carries a tool call and its result inside the SAME assistant UIMessage's parts, + * per-row conversion preserves call/result pairing. + */ +export async function convertHistoryResilient( + uiMessages: Array & { id: string }>, + onDegrade: (index: number, err: unknown) => void, +): Promise { + try { + return await convertToModelMessages(uiMessages as UIMessage[]); + } catch { + const out: ModelMessage[] = []; + for (let i = 0; i < uiMessages.length; i++) { + const m = uiMessages[i]; + try { + out.push(...(await convertToModelMessages([m as UIMessage]))); + } catch (err) { + onDegrade(i, err); + const text = uiMessageText(m as UIMessage); + const degraded = text + ? `${text}\n\n${TOOL_CONTEXT_OMITTED_MARKER}` + : TOOL_CONTEXT_OMITTED_MARKER; + out.push({ + role: m.role === 'assistant' ? 'assistant' : 'user', + content: degraded, + } as ModelMessage); + } + } + return out; + } +} + /** * Minimal shapes of the AI SDK v6 step objects we read to rebuild UIMessage * parts (see ai@6.0.134 `StepResult`: `text`, `toolCalls` -> TypedToolCall,