From 4a02791c9a72e89c8c442a0a0972d06ddfc89361 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Wed, 19 Aug 2026 12:47:24 +0800 Subject: [PATCH] fix(llm): address multimodal review findings --- ...-19-direct-deepseek-vision-input.i18n.yaml | 4 +- ...2026-08-19-direct-deepseek-vision-input.md | 2 +- ...6-08-19-direct-deepseek-vision-input.zh.md | 2 +- docs/config-catalog.i18n.yaml | 2 +- docs/config-catalog.zh.md | 2 +- docs/module-graph.i18n.yaml | 4 +- docs/module-graph.md | 3 +- docs/module-graph.zh.md | 3 +- packages/llm/llm-deepseek/src/serialize.ts | 14 +- .../llm/llm-deepseek/tests/adapter.e2e.ts | 61 --------- .../llm/llm-deepseek/tests/adapter.spec.ts | 32 ++++- .../llm/llm-deepseek/tests/serialize.spec.ts | 129 ++++++++++++++++++ packages/llm/llm-pi-ai/src/context.ts | 16 ++- packages/llm/llm-pi-ai/tests/context.spec.ts | 64 ++++++++- packages/llm/llm/tests/content.spec.ts | 13 ++ 15 files changed, 268 insertions(+), 83 deletions(-) diff --git a/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.i18n.yaml b/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.i18n.yaml index a2caf36d87..d46a50b6d3 100644 --- a/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.i18n.yaml +++ b/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write .agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.md -2026-08-19-direct-deepseek-vision-input.md: 60f55f10e108a29d465e388fa385b1955b7f2ce5 -2026-08-19-direct-deepseek-vision-input.zh.md: 7f41d89ffd13da1b1b170f114dfc481e66075ef2 +2026-08-19-direct-deepseek-vision-input.md: 5c2f2ea3ff87b58906ce76caf9f611e07f3e2d04 +2026-08-19-direct-deepseek-vision-input.zh.md: 3aea6b6a4f27f7116c1cb935b1719dfbcf7ed8fc diff --git a/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.md b/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.md index 60f55f10e1..5c2f2ea3ff 100644 --- a/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.md +++ b/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.md @@ -27,7 +27,7 @@ Canonical messages continue to store only `ImageAttachmentRef`. Data URLs exist ## Verification -Package tests pin model discovery and fallback capabilities, configuration validation and live settings updates, user and tool-result wire messages, all admitted MIME types, cancellation, attachment failures, 413 classification, exact image-bound behavior, and pi-ai equivalence. A keyless assembled ACP request records the native adapter's tool-result data URL and oldest-image placeholder. A key-gated real-API e2e sends a deterministic image to the official vision model. +Package tests pin model discovery and fallback capabilities, configuration validation and live settings updates, user and tool-result wire messages, all admitted MIME types, cancellation, attachment failures, 413 classification, exact image-bound behavior, and pi-ai equivalence. A keyless assembled ACP request records the native adapter's tool-result data URL and oldest-image placeholder. ## Consequences diff --git a/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.zh.md b/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.zh.md index 7f41d89ffd..3aea6b6a4f 100644 --- a/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.zh.md +++ b/.agents/notes/implemented/feature/2026-08-19-direct-deepseek-vision-input.zh.md @@ -27,7 +27,7 @@ DeepSeek 视觉部署使用 chat-completions 图片协议，但直接 `deepseek- ## Verification -包测试固定模型发现与回退能力、配置校验与存活 settings 更新、user 和工具结果协议消息、所有已准入 MIME 类型、取消、附件失败、413 分类、确切图片上限行为和 pi-ai 等价性。无需密钥的组装 ACP 请求会记录原生适配器的工具结果 data URL 与最旧图片占位文本。受密钥控制的真实 API e2e 会向官方视觉模型发送一张确定性图片。 +包测试固定模型发现与回退能力、配置校验与存活 settings 更新、user 和工具结果协议消息、所有已准入 MIME 类型、取消、附件失败、413 分类、确切图片上限行为和 pi-ai 等价性。无需密钥的组装 ACP 请求会记录原生适配器的工具结果 data URL 与最旧图片占位文本。 ## Consequences diff --git a/docs/config-catalog.i18n.yaml b/docs/config-catalog.i18n.yaml index 7f1120a1df..6a9a3acf1f 100644 --- a/docs/config-catalog.i18n.yaml +++ b/docs/config-catalog.i18n.yaml @@ -3,4 +3,4 @@ # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write docs/config-catalog.md config-catalog.md: 7d42923c1bc4c73636b52713694ff8f6eeb9146a -config-catalog.zh.md: b9327fa127ba8e341a15954685942f2d15d6b8d3 +config-catalog.zh.md: 327a4951c513e7cd2360488026e7578e95458f47 diff --git a/docs/config-catalog.zh.md b/docs/config-catalog.zh.md index b9327fa127..327a4951c5 100644 --- a/docs/config-catalog.zh.md +++ b/docs/config-catalog.zh.md @@ -897,7 +897,7 @@ export interface DeepSeekCatalogModel { 依赖：[`ModelModality`](../packages/llm/llm/src/index.ts) · [`RetryPolicyConfig`](../packages/llm/llm/src/index.ts) -来源：[`packages/llm/llm-deepseek/src/index.ts:72`](../packages/llm/llm-deepseek/src/index.ts) +来源：[`packages/llm/llm-deepseek/src/index.ts:66`](../packages/llm/llm-deepseek/src/index.ts) diff --git a/docs/module-graph.i18n.yaml b/docs/module-graph.i18n.yaml index 1b0e8fad9c..eea3a0a930 100644 --- a/docs/module-graph.i18n.yaml +++ b/docs/module-graph.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write docs/module-graph.md -module-graph.md: 0bd5f80534ba65e0d483bd04228cdac321e082bc -module-graph.zh.md: 6239520b7d819eb14c2b859afa44a0c987545200 +module-graph.md: 5398515912c98962152d2b121b1b02d7a06097b9 +module-graph.zh.md: 7a398374603d66f8a3661a1d927b6418a99235a6 diff --git a/docs/module-graph.md b/docs/module-graph.md index 0bd5f80534..5398515912 100644 --- a/docs/module-graph.md +++ b/docs/module-graph.md @@ -407,6 +407,7 @@ flowchart TD pkg_settings_file --> pkg_invariants pkg_settings_file --> pkg_settings pkg_llm_deepseek --> pkg_anonymous_user_id + pkg_llm_deepseek --> pkg_attachment pkg_llm_deepseek --> pkg_credentials pkg_llm_deepseek --> pkg_invariants pkg_llm_deepseek --> pkg_launch_environment @@ -1463,7 +1464,7 @@ flowchart TD | [`client-hmr`](../packages/client/hmr) | `client` | [`client-modules`](../packages/client/modules), [`host-webserver`](../packages/host/webserver), [`invariants`](../packages/runtime-diagnostics/invariants) | | [`credentials-local`](../packages/credentials/credentials-local) | `credentials` | [`atomic-write`](../packages/util/atomic-write), [`credentials`](../packages/credentials/credentials), [`home-paths`](../packages/util/home-paths), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment) | | [`settings-file`](../packages/settings/settings-file) | `settings` | [`atomic-write`](../packages/util/atomic-write), [`home-paths`](../packages/util/home-paths), [`invariants`](../packages/runtime-diagnostics/invariants), [`settings`](../packages/settings/settings) | -| [`llm-deepseek`](../packages/llm/llm-deepseek) | `llm` | [`anonymous-user-id`](../packages/identity/anonymous-user-id), [`credentials`](../packages/credentials/credentials), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment), [`llm`](../packages/llm/llm), [`settings`](../packages/settings/settings), [`timeout`](../packages/util/timeout) | +| [`llm-deepseek`](../packages/llm/llm-deepseek) | `llm` | [`anonymous-user-id`](../packages/identity/anonymous-user-id), [`attachment`](../packages/attachment/attachment), [`credentials`](../packages/credentials/credentials), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment), [`llm`](../packages/llm/llm), [`settings`](../packages/settings/settings), [`timeout`](../packages/util/timeout) | | [`llm-pi-ai`](../packages/llm/llm-pi-ai) | `llm` | [`attachment`](../packages/attachment/attachment), [`credentials`](../packages/credentials/credentials), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment), [`llm`](../packages/llm/llm), [`settings`](../packages/settings/settings), [`timeout`](../packages/util/timeout) | | [`session`](../packages/core/session) | `core` | [`brand`](../packages/util/brand), [`invariants`](../packages/runtime-diagnostics/invariants), [`llm`](../packages/llm/llm), [`scope`](../packages/core/scope), [`typert-protocol`](../packages/typert/protocol) | | [`system-prompt`](../packages/core/system-prompt) | `core` | [`invariants`](../packages/runtime-diagnostics/invariants), [`llm`](../packages/llm/llm), [`scope`](../packages/core/scope) | diff --git a/docs/module-graph.zh.md b/docs/module-graph.zh.md index 6239520b7d..7a39837460 100644 --- a/docs/module-graph.zh.md +++ b/docs/module-graph.zh.md @@ -409,6 +409,7 @@ flowchart TD pkg_settings_file --> pkg_invariants pkg_settings_file --> pkg_settings pkg_llm_deepseek --> pkg_anonymous_user_id + pkg_llm_deepseek --> pkg_attachment pkg_llm_deepseek --> pkg_credentials pkg_llm_deepseek --> pkg_invariants pkg_llm_deepseek --> pkg_launch_environment @@ -1465,7 +1466,7 @@ flowchart TD | [`client-hmr`](../packages/client/hmr) | `client` | [`client-modules`](../packages/client/modules), [`host-webserver`](../packages/host/webserver), [`invariants`](../packages/runtime-diagnostics/invariants) | | [`credentials-local`](../packages/credentials/credentials-local) | `credentials` | [`atomic-write`](../packages/util/atomic-write), [`credentials`](../packages/credentials/credentials), [`home-paths`](../packages/util/home-paths), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment) | | [`settings-file`](../packages/settings/settings-file) | `settings` | [`atomic-write`](../packages/util/atomic-write), [`home-paths`](../packages/util/home-paths), [`invariants`](../packages/runtime-diagnostics/invariants), [`settings`](../packages/settings/settings) | -| [`llm-deepseek`](../packages/llm/llm-deepseek) | `llm` | [`anonymous-user-id`](../packages/identity/anonymous-user-id), [`credentials`](../packages/credentials/credentials), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment), [`llm`](../packages/llm/llm), [`settings`](../packages/settings/settings), [`timeout`](../packages/util/timeout) | +| [`llm-deepseek`](../packages/llm/llm-deepseek) | `llm` | [`anonymous-user-id`](../packages/identity/anonymous-user-id), [`attachment`](../packages/attachment/attachment), [`credentials`](../packages/credentials/credentials), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment), [`llm`](../packages/llm/llm), [`settings`](../packages/settings/settings), [`timeout`](../packages/util/timeout) | | [`llm-pi-ai`](../packages/llm/llm-pi-ai) | `llm` | [`attachment`](../packages/attachment/attachment), [`credentials`](../packages/credentials/credentials), [`invariants`](../packages/runtime-diagnostics/invariants), [`launch-environment`](../packages/util/launch-environment), [`llm`](../packages/llm/llm), [`settings`](../packages/settings/settings), [`timeout`](../packages/util/timeout) | | [`session`](../packages/core/session) | `core` | [`brand`](../packages/util/brand), [`invariants`](../packages/runtime-diagnostics/invariants), [`llm`](../packages/llm/llm), [`scope`](../packages/core/scope), [`typert-protocol`](../packages/typert/protocol) | | [`system-prompt`](../packages/core/system-prompt) | `core` | [`invariants`](../packages/runtime-diagnostics/invariants), [`llm`](../packages/llm/llm), [`scope`](../packages/core/scope) | diff --git a/packages/llm/llm-deepseek/src/serialize.ts b/packages/llm/llm-deepseek/src/serialize.ts index bdf9869544..da643aa599 100644 --- a/packages/llm/llm-deepseek/src/serialize.ts +++ b/packages/llm/llm-deepseek/src/serialize.ts @@ -148,9 +148,12 @@ async function contentParts( /** Keep text-only user messages on the compact string wire form. */ function userContent(parts: readonly WireUserContentPart[]): string | WireUserContentPart[] { - return parts.some(part => part.type === 'image_url') - ? [...parts] - : parts.map(part => part.type === 'text' ? part.text : '').join('') + const text: string[] = [] + for (const part of parts) { + if (part.type === 'image_url') return [...parts] + text.push(part.text) + } + return text.join('') } /** Serialize one assistant message (text + reasoning + tool calls). */ @@ -268,11 +271,12 @@ export async function serializeMessagesWithImages( const toolResults = message.content.filter((block): block is Extract => ( block.type === 'tool-result' )) - if (regular.length > 0 || toolResults.length === 0) { + const content = userContent(await contentParts(regular, attachments, signal)) + if (content.length > 0 || toolResults.length === 0) { flushToolImages() wire.push({ role: 'user', - content: userContent(await contentParts(regular, attachments, signal)), + content, }) } for (const result of toolResults) { diff --git a/packages/llm/llm-deepseek/tests/adapter.e2e.ts b/packages/llm/llm-deepseek/tests/adapter.e2e.ts index 7845157fab..19ce411406 100644 --- a/packages/llm/llm-deepseek/tests/adapter.e2e.ts +++ b/packages/llm/llm-deepseek/tests/adapter.e2e.ts @@ -1,18 +1,10 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { createHash } from 'node:crypto' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { Context } from '@deepseek-ai/cordis' import LlmRuntime, { createUserMessage, CallId, ReasoningEffortId , createMessage } from '@deepseek-ai/dsh-llm' import type { Message, ToolSchema } from '@deepseek-ai/dsh-llm' -import AttachmentStore, { AttachmentId } from '@deepseek-ai/dsh-attachment' -import type { - ImageAttachmentLimits, - ImageAttachmentRef, - SaveImageAttachment, - StoredImageAttachment, -} from '@deepseek-ai/dsh-attachment' import { LocalCredentialProvider } from '@deepseek-ai/dsh-credentials-local' import * as LlmDeepSeek from '@deepseek-ai/dsh-llm-deepseek' import type { Config } from '@deepseek-ai/dsh-llm-deepseek' @@ -26,41 +18,6 @@ import { assemble, type AssembledResult } from './assemble.ts' const FLASH = 'deepseek-v4-flash' const PRO = 'deepseek-v4-pro' -const VISION = 'deepseek-v4-flash-vision-exp' -const RED_IMAGE = Buffer.from( - 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1PeAAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC', - 'base64', -) -const RED_IMAGE_REF: ImageAttachmentRef = { - attachmentId: AttachmentId(`sha256:${createHash('sha256').update(RED_IMAGE).digest('hex')}`), - mediaType: 'image/png', - bytes: RED_IMAGE.byteLength, - width: 1, - height: 1, -} - -class E2eAttachmentStore extends AttachmentStore { - readonly imageLimits: ImageAttachmentLimits = { - maxImageBytes: 1024, - maxImagesPerMessage: 1, - maxMessageImageBytes: 1024, - maxImagePixels: 1, - maxImageDimension: 1, - mediaTypes: ['image/png'], - } - - validateImage(_input: SaveImageAttachment): Promise { - return Promise.resolve() - } - - saveImage(_input: SaveImageAttachment): Promise { - return Promise.resolve(RED_IMAGE_REF) - } - - readImage(_ref: ImageAttachmentRef, _signal?: AbortSignal): Promise { - return Promise.resolve({ ref: RED_IMAGE_REF, data: RED_IMAGE }) - } -} const contexts: Context[] = [] let identityHome: string @@ -73,7 +30,6 @@ async function harness(_model: string, config: Partial = {}) { const ctx = new Context() contexts.push(ctx) await ctx.plugin(LlmRuntime) - await ctx.plugin(E2eAttachmentStore) await ctx.plugin(LlmDeepSeek, config) return ctx } @@ -109,23 +65,6 @@ const weatherTool: ToolSchema = { } describe.skipIf(!process.env.DEEPSEEK_API_KEY)('llm-deepseek e2e (real API)', () => { - it('recognizes a deterministic image with the official vision model', async () => { - const ctx = await harness(VISION, { thinking: 'disabled' }) - const result = await assemble(ctx, { - model: VISION, - messages: [createUserMessage({ - content: [ - { type: 'text', text: 'This image is one solid color. Reply with only its English color name.' }, - { type: 'image', attachment: RED_IMAGE_REF }, - ], - source: { kind: 'plugin', plugin: 'test' }, - })], - maxTokens: 50, - }) - expect(result.finish.kind).toBe('stop') - expect(textOf(result).toLowerCase()).toContain('red') - }) - it('serves a real request with the key held only by a credentials-local document', async () => { const key = process.env.DEEPSEEK_API_KEY if (key === undefined) throw new Error('e2e ran without DEEPSEEK_API_KEY') diff --git a/packages/llm/llm-deepseek/tests/adapter.spec.ts b/packages/llm/llm-deepseek/tests/adapter.spec.ts index cecb37bdff..6d86691e5c 100644 --- a/packages/llm/llm-deepseek/tests/adapter.spec.ts +++ b/packages/llm/llm-deepseek/tests/adapter.spec.ts @@ -178,7 +178,10 @@ describe('DeepSeekAdapter against a mock server', () => { const server = await mockServer([]) const resolveApiKey = vi.fn(() => Promise.resolve('k')) const adapter = new DeepSeekAdapter({ - options: () => resolveAdapterOptions({ baseURL: server.url }), + options: () => resolveAdapterOptions({ + baseURL: server.url, + models: [{ id: 'deepseek-v4-flash-vision-exp', inputModalities: ['text', 'image'] }], + }), resolveApiKey, resolveUserId: () => TEST_USER_ID, }) @@ -865,6 +868,21 @@ describe('plugin registration and config', () => { ]) }) + it('defaults an adapter-supplied catalog entry to text input', async () => { + const connection = resolveAdapterOptions({ models: [] }) + const adapter = new DeepSeekAdapter({ + options: () => ({ ...connection, models: [{ id: 'adapter-model' }] }), + resolveApiKey: () => Promise.resolve('k'), + resolveUserId: () => TEST_USER_ID, + }) + await expect(adapter.listModels('deepseek-official')).resolves.toEqual([{ + provider: 'deepseek-official', + id: 'adapter-model', + name: 'adapter-model', + inputModalities: ['text'], + }]) + }) + it('advertises configured models without restricting arbitrary request ids', async () => { const ctx = new Context() await ctx.plugin(LlmRuntime) @@ -954,6 +972,18 @@ describe('plugin registration and config', () => { expect(ctx.llm.listProviders()).toEqual([]) }) + const invalidProgrammaticModalities: Array<[LlmDeepSeek.DeepSeekCatalogModel[], RegExp]> = [ + [[{ id: 'm', inputModalities: [] }], /inputModalities must not be empty/], + [[{ + id: 'm', + inputModalities: ['audio'] as unknown as NonNullable, + }], /inputModalities must contain only "text" and "image"/], + ] + + it.each(invalidProgrammaticModalities)('rejects programmatic modality config that bypasses the schema', (models, message) => { + expect(() => resolveAdapterOptions({ models: [...models] })).toThrow(message) + }) + it.each([0, 1.5])('rejects a per-model output cap of %s', (maxTokens) => { expect(() => resolveAdapterOptions({ models: [{ id: 'bad-cap', maxTokens }] })) .toThrow(/maxTokens must be a positive integer/) diff --git a/packages/llm/llm-deepseek/tests/serialize.spec.ts b/packages/llm/llm-deepseek/tests/serialize.spec.ts index 059d6c0a54..d503a8b5c7 100644 --- a/packages/llm/llm-deepseek/tests/serialize.spec.ts +++ b/packages/llm/llm-deepseek/tests/serialize.spec.ts @@ -393,6 +393,95 @@ describe('image serialization', () => { ]) }) + it('does not emit an empty user message for ignored content beside a tool result', async () => { + const messages = [createUserMessage({ + content: [ + { type: 'text', text: '' }, + { type: 'chart', data: 'ignored' } as unknown as ContentBlock, + { + type: 'tool-result', + toolCallId: CallId('result'), + content: [{ type: 'text', text: 'ok' }], + }, + ], + source: { kind: 'plugin', plugin: 'test' }, + })] + + await expect(serializeMessagesWithImages( + messages, + attachmentStore(), + new AbortController().signal, + )).resolves.toEqual([ + { role: 'tool', tool_call_id: 'result', content: 'ok' }, + ]) + }) + + it('recursively converts nested tool-result content and preserves the empty fallback', async () => { + const messages = [createUserMessage({ + content: [ + { + type: 'tool-result', + toolCallId: CallId('nested'), + content: [{ + type: 'tool-result', + toolCallId: CallId('inner'), + content: [{ type: 'text', text: 'inside' }], + }], + }, + { type: 'tool-result', toolCallId: CallId('empty'), content: [] }, + ], + source: { kind: 'plugin', plugin: 'test' }, + })] + + await expect(serializeMessagesWithImages( + messages, + attachmentStore(), + new AbortController().signal, + )).resolves.toEqual([ + { role: 'tool', tool_call_id: 'nested', content: 'inside' }, + { role: 'tool', tool_call_id: 'empty', content: '(no output)' }, + ]) + }) + + it('flushes tool-result images before system and assistant history', async () => { + const imageResult = (id: string) => createUserMessage({ + content: [{ + type: 'tool-result', + toolCallId: CallId(id), + content: [{ type: 'image', attachment: imageRef() }], + }], + source: { kind: 'plugin' as const, plugin: 'test' }, + }) + const messages = [ + imageResult('before-system'), + createMessage({ + role: 'system', + content: [{ type: 'text', text: 'system history' }], + source: { kind: 'plugin', plugin: 'test' }, + }), + imageResult('before-assistant'), + createMessage({ + role: 'assistant', + content: [{ type: 'text', text: 'assistant history' }], + source: { kind: 'plugin', plugin: 'test' }, + }), + ] + + const wire = await serializeMessagesWithImages( + messages, + attachmentStore(), + new AbortController().signal, + ) + expect(wire).toEqual([ + { role: 'tool', tool_call_id: 'before-system', content: '(see attached image)' }, + expect.objectContaining({ role: 'user' }), + { role: 'system', content: 'system history' }, + { role: 'tool', tool_call_id: 'before-assistant', content: '(see attached image)' }, + expect.objectContaining({ role: 'user' }), + { role: 'assistant', content: 'assistant history' }, + ]) + }) + it('offloads oldest images before reads and keeps the newest image', async () => { const readImage = vi.fn((ref: ImageAttachmentRef) => Promise.resolve({ ref, @@ -435,6 +524,37 @@ describe('image serialization', () => { expect(readImage).not.toHaveBeenCalled() }) + it('rejects unsupported image history before request offloading can replace it', async () => { + const readImage = vi.fn() + await expect(serializeRequestWithImages(request({ + messages: [createMessage({ + role: 'system', + content: [{ type: 'image', attachment: imageRef('image/png', 300) }], + source: { kind: 'plugin', plugin: 'test' }, + })], + }), { + attachments: attachmentStore(readImage), + maxRequestImageBytes: 1, + signal: new AbortController().signal, + })).rejects.toMatchObject({ code: 'UNSUPPORTED_CONTENT' }) + expect(readImage).not.toHaveBeenCalled() + }) + + it('prepends the request system prompt on the image path', async () => { + const wire = await serializeRequestWithImages(request({ + system: 'system prompt', + messages: [createUserMessage({ + content: [{ type: 'image', attachment: imageRef() }], + source: { kind: 'plugin', plugin: 'test' }, + })], + }), { + attachments: attachmentStore(), + maxRequestImageBytes: 20 * 1024 * 1024, + signal: new AbortController().signal, + }) + expect(wire.messages[0]).toEqual({ role: 'system', content: 'system prompt' }) + }) + it('preserves stable attachment failure codes', async () => { const readImage = vi.fn(() => Promise.reject(new AttachmentError( 'Stored attachment bytes are corrupt.', @@ -446,6 +566,15 @@ describe('image serialization', () => { })], attachmentStore(readImage), new AbortController().signal)) .rejects.toMatchObject({ code: 'ATTACHMENT_CORRUPT' }) }) + + it('preserves non-attachment resolver failures', async () => { + const failure = new Error('resolver failed') + const readImage = vi.fn(() => Promise.reject(failure)) + await expect(serializeMessagesWithImages([createUserMessage({ + content: [{ type: 'image', attachment: imageRef() }], + source: { kind: 'plugin', plugin: 'test' }, + })], attachmentStore(readImage), new AbortController().signal)).rejects.toBe(failure) + }) }) describe('review fixes: assistant content shapes', () => { diff --git a/packages/llm/llm-pi-ai/src/context.ts b/packages/llm/llm-pi-ai/src/context.ts index c0391b7bad..d66a48115d 100644 --- a/packages/llm/llm-pi-ai/src/context.ts +++ b/packages/llm/llm-pi-ai/src/context.ts @@ -26,6 +26,18 @@ function toolResultText(blocks: readonly ContentBlock[]): string { : block.type === 'tool-result' ? toolResultText(block.content) : '').join('') } +/** Reject image roles that pi-ai cannot replay before request-size offloading can replace them. */ +function assertSupportedImageRoles(messages: readonly Message[]): void { + for (const message of messages) { + if (message.role !== 'user' && contentHasImage(message.content)) { + throw new LlmError( + `pi-ai cannot represent an image in an in-history ${message.role} message`, + 'UNSUPPORTED_CONTENT', + ) + } + } +} + async function userContent( blocks: readonly ContentBlock[], attachments: AttachmentStore, @@ -169,15 +181,13 @@ async function toPiContextWithImages( onReplayDegrade?: (reason: string) => void, maxRequestImageBytes?: number, ): Promise { + assertSupportedImageRoles(options.messages) const requestMessages = offloadRequestImages(options.messages, maxRequestImageBytes) const toolNames = new Map() const messages: PiMessage[] = [] for (const message of requestMessages) { if (message.role === 'system') { - if (contentHasImage(message.content)) { - throw new LlmError('pi-ai cannot represent an image in an in-history system message', 'UNSUPPORTED_CONTENT') - } // pi-ai has a single systemPrompt slot; in-history system messages are // folded into user messages to preserve order (rare in practice — the // harness sends the system prompt via options.system). diff --git a/packages/llm/llm-pi-ai/tests/context.spec.ts b/packages/llm/llm-pi-ai/tests/context.spec.ts index 2ab90d0c21..b82c6b63f5 100644 --- a/packages/llm/llm-pi-ai/tests/context.spec.ts +++ b/packages/llm/llm-pi-ai/tests/context.spec.ts @@ -140,6 +140,59 @@ describe('pi-ai request context conversion', () => { ]) }) + it('recursively converts nested tool-result text and images', async () => { + const callId = CallId('nested-call') + const context = await toPiContext(request([user([{ + type: 'tool-result', + toolCallId: callId, + content: [ + { + type: 'tool-result', + toolCallId: callId, + content: [{ type: 'text', text: 'nested text' }], + }, + { + type: 'tool-result', + toolCallId: callId, + content: [{ type: 'image', attachment: ref }], + }, + ], + }])]), attachments) + + expect(context.messages).toEqual([{ + role: 'toolResult', + toolCallId: 'nested-call', + toolName: 'unknown', + content: [ + { type: 'text', text: 'nested text' }, + { type: 'image', data: 'AQ==', mimeType: 'image/png' }, + ], + isError: false, + timestamp: 0, + }]) + }) + + it('flattens nested text-only tool results and ignores other block types without storage', () => { + const callId = CallId('nested-text') + expect(toPiContext(request([user([{ + type: 'tool-result', + toolCallId: callId, + content: [ + { type: 'chart', data: 'ignored' } as unknown as ContentBlock, + { + type: 'tool-result', + toolCallId: callId, + content: [{ type: 'text', text: 'nested' }], + }, + ], + }])]))).toMatchObject({ + messages: [{ + role: 'toolResult', + content: [{ type: 'text', text: 'nested' }], + }], + }) + }) + it('replaces the oldest images with placeholders once the request payload bound is exceeded', async () => { const readImage = vi.fn(() => Promise.resolve({ ref: { ...ref, bytes: 3 }, data: Uint8Array.of(1, 2, 3) })) const store = { readImage } as unknown as AttachmentStore @@ -249,9 +302,14 @@ describe('pi-ai request context conversion', () => { }) it('handles in-history system and assistant messages explicitly on the image path', async () => { - await expect(toPiContext(request([ - history('system', [{ type: 'image', attachment: ref }]), - ]), attachments)).rejects.toMatchObject({ code: 'UNSUPPORTED_CONTENT' }) + for (const role of ['system', 'assistant'] as const) { + const readImage = vi.fn() + const store = { readImage } as unknown as AttachmentStore + await expect(toPiContext(request([ + history(role, [{ type: 'image', attachment: ref }]), + ]), store, undefined, 1)).rejects.toMatchObject({ code: 'UNSUPPORTED_CONTENT' }) + expect(readImage).not.toHaveBeenCalled() + } await expect(toPiContext(request([ history('system', [{ type: 'text', text: 'history system' }]), diff --git a/packages/llm/llm/tests/content.spec.ts b/packages/llm/llm/tests/content.spec.ts index e61a585230..ffb5a586bf 100644 --- a/packages/llm/llm/tests/content.spec.ts +++ b/packages/llm/llm/tests/content.spec.ts @@ -73,4 +73,17 @@ describe('offloadRequestImages', () => { expect(offloadRequestImages(messages, 8)[0]?.content) .toEqual([{ type: 'text', text: OFFLOADED_IMAGE_TEXT }]) }) + + it('keeps unchanged nested content while replacing a later image', () => { + const nested = { + type: 'tool-result' as const, + toolCallId: CallId('text-only'), + content: [{ type: 'text' as const, text: 'kept' }], + } + const messages = [createUserMessage({ content: [nested, image(3)], source })] + expect(offloadRequestImages(messages, 1)[0]?.content).toEqual([ + nested, + { type: 'text', text: OFFLOADED_IMAGE_TEXT }, + ]) + }) })