fix(ai-chat): валидация клиентских parts + insert после конвертации (#489)
Юзер-сообщение персистилось в metadata БЕЗ валидации и реплеилось через
convertToModelMessages на каждом ходе; insert user-строки шёл ДО конвертации.
Битая структура parts (например, null-элемент в массиве) → конвертация кидает
→ 500 на КАЖДОМ ходе навсегда, а каждый ретрай добавлял дубль user-строки.
- Санитизация parts при приёме: whitelist { text }, прочее (в т.ч. tool-part
в input-available) отбрасывается с warn — не попадает в metadata.
- convertToModelMessages прогоняется ДО insert'а user-строки (ретрай не плодит
дубли); при падении на СТАРОЙ истории — per-row конвертация изолирует битую
строку и деградирует её до plain-text с маркером «[tool context omitted]»
(молчаливая потеря tool-контекста недопустима).
Тесты против РЕАЛЬНОГО convertToModelMessages (null-part реально кидает):
unit трёх веток + сервис-регресс «чат с битым сообщением в истории работает,
маркер доходит до модели, одна user-строка».
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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<UIMessage, 'id'> & { 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);
|
||||
});
|
||||
});
|
||||
@@ -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' }]);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
|
||||
@@ -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<Omit<UIMessage, 'id'> & { 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<ModelMessage[]>).
|
||||
// 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<ModelMessage[]>).
|
||||
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<string> = 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<Omit<UIMessage, 'id'> & { id: string }>,
|
||||
onDegrade: (index: number, err: unknown) => void,
|
||||
): Promise<ModelMessage[]> {
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user