From b0577d33f898d422c7964247080479730f9f0eed Mon Sep 17 00:00:00 2001 From: CyberWolf Date: Wed, 4 Nov 2026 12:34:45 +0800 Subject: [PATCH] fix(ollama): drop /api/chat suffix option and surface stream errors - Provider dropdown offers only /chat; /api/chat built /api/api/chat on the default /api base - Migration moves saved /api/chat suffixes to base .../api + /chat (same URL, never /api/api/chat) - OllamaClient throws on data.error and when no message is returned (stream and non-stream) - Tests for dropdown, migration and client error paths Follow-up to #78 review. --- .../services/__tests__/OllamaProtocol.test.ts | 3 +- .../adapters/llm/OllamaClient.ts | 90 +++++++++++----- .../llm/__tests__/OllamaClient.test.ts | 79 ++++++++++++++ .../OllamaChatSuffixMigration.test.ts | 100 ++++++++++++++---- .../migration.sql | 9 ++ src/renderer/components/Provider.tsx | 2 +- .../components/__tests__/Provider.test.tsx | 4 +- 7 files changed, 235 insertions(+), 52 deletions(-) create mode 100644 src/core/infrastructure/db/migrations/20261104000001_ollama_api_chat_suffix/migration.sql diff --git a/src/core/application/services/__tests__/OllamaProtocol.test.ts b/src/core/application/services/__tests__/OllamaProtocol.test.ts index 727a22e..e324d38 100644 --- a/src/core/application/services/__tests__/OllamaProtocol.test.ts +++ b/src/core/application/services/__tests__/OllamaProtocol.test.ts @@ -126,8 +126,7 @@ describe('Ollama protocol consistency', () => { it.each([ { base: `${HOST}/api`, suffix: '' }, { base: `${HOST}/api`, suffix: null }, - { base: `${HOST}/api`, suffix: '/chat' }, - { base: HOST, suffix: '/api/chat' } + { base: `${HOST}/api`, suffix: '/chat' } ])('reaches /api/chat with base "$base" and suffix "$suffix"', async ({ base, suffix }) => { useProvider(base, suffix) diff --git a/src/core/infrastructure/adapters/llm/OllamaClient.ts b/src/core/infrastructure/adapters/llm/OllamaClient.ts index befcfa9..29408f9 100644 --- a/src/core/infrastructure/adapters/llm/OllamaClient.ts +++ b/src/core/infrastructure/adapters/llm/OllamaClient.ts @@ -82,11 +82,16 @@ export class OllamaClient extends LLMClient { const decoder = new TextDecoder('utf-8'); let content = ''; let model = ''; + let hasMessage = false; const processStream = async (): Promise => { const { done, value } = await reader.read(); if (done) { + if (!hasMessage) { + throw this.missingMessageError(); + } + return { content, model: model || normalizedOptions.model || 'llama3', @@ -100,53 +105,67 @@ export class OllamaClient extends LLMClient { const lines = chunk.split('\n').filter(line => line.trim() !== ''); for (const line of lines) { + let data: any; try { - const data = JSON.parse(line); + data = JSON.parse(line); + } catch { + // 忽略解析错误 + continue; + } - // 存储模型名称 - if (data.model && !model) { - model = data.model; - } + this.assertNoResponseError(data); - // 提取消息内容 - if (data.message && data.message.content) { - if (!data.done) { - // 累加流式内容 - content += data.message.content; - if (normalizedOptions.onUpdate) { - normalizedOptions.onUpdate(content); - } + // 存储模型名称 + if (data.model && !model) { + model = data.model; + } + + if (data.message) { + hasMessage = true; + } + + // 提取消息内容 + if (data.message && data.message.content) { + if (!data.done) { + // 累加流式内容 + content += data.message.content; + if (normalizedOptions.onUpdate) { + normalizedOptions.onUpdate(content); } } + } - // 如果是最后一条消息，可以直接返回结果 - if (data.done === true) { - return { - content, - model: data.model || normalizedOptions.model || 'llama3', - finishReason: 'stop', - responseFormat: normalizedOptions.response_format?.type, - rawResponse: data - }; + // 如果是最后一条消息，可以直接返回结果 + if (data.done === true) { + if (!hasMessage) { + throw this.missingMessageError(); } - } catch { - // 忽略解析错误 + + return { + content, + model: data.model || normalizedOptions.model || 'llama3', + finishReason: 'stop', + responseFormat: normalizedOptions.response_format?.type, + rawResponse: data + }; } } return processStream(); }; - return processStream(); + // 需要 await：流中抛出的错误要经过下方 catch 统一包装 + return await processStream(); } else { // 处理非流式响应 const data = await response.json().catch((error: Error) => { throw new Error(`Ollama 返回了非 JSON 响应 (HTTP ${response.status}): ${error.message}`); }); - // 没有 message 说明响应不是 /api/chat 协议（如 /api/generate），不能当作模型返回了空内容 + this.assertNoResponseError(data); + if (!data.message) { - throw new Error(`Ollama 响应缺少 message 字段，请确认 API 地址指向 /api/chat: ${this.baseUrl}`); + throw this.missingMessageError(); } return { @@ -163,6 +182,23 @@ export class OllamaClient extends LLMClient { } } + /** + * HTTP 200 的响应体也可能带 error（如流式响应开始后模型崩溃） + */ + private assertNoResponseError(data: any): void { + if (data.error) { + const detail = typeof data.error === 'string' ? data.error : JSON.stringify(data.error); + throw new Error(`Ollama 返回错误: ${detail}`); + } + } + + /** + * 没有 message 说明响应不是 /api/chat 协议（如 /api/generate），不能当作模型返回了空内容 + */ + private missingMessageError(): Error { + return new Error(`Ollama 响应缺少 message 字段，请确认 API 地址指向 /api/chat: ${this.baseUrl}`); + } + /** * 读取非 2xx 响应的错误详情 * 响应体不一定是 JSON（如路由不存在时是纯文本 404），解析失败时使用原文 diff --git a/src/core/infrastructure/adapters/llm/__tests__/OllamaClient.test.ts b/src/core/infrastructure/adapters/llm/__tests__/OllamaClient.test.ts index 15879f9..fec5337 100644 --- a/src/core/infrastructure/adapters/llm/__tests__/OllamaClient.test.ts +++ b/src/core/infrastructure/adapters/llm/__tests__/OllamaClient.test.ts @@ -201,6 +201,79 @@ describe('OllamaClient', () => { expect(result.content).toBe('Streaming response') }) + + describe('responses that are not a chat stream', () => { + const stubStream = (...lines: unknown[]) => { + const ndjson = lines.map((line) => JSON.stringify(line)).join('\n') + global.fetch = vi.fn().mockResolvedValue(new Response(ndjson)) + } + + const completeStream = (onUpdate: (content: string) => void = vi.fn()) => client.completion({ + messages: [ + { role: 'user', content: { type: 'text', text: 'Test' } } + ], + model: 'llama2', + stream: true, + onUpdate + }) + + it('should report an error sent as the first line', async () => { + stubStream({ error: "model 'x' not found" }) + + await expect(completeStream()).rejects.toThrow("Ollama补全请求失败: Ollama 返回错误: model 'x' not found") + }) + + it('should report an error sent after partial content instead of returning it', async () => { + const onUpdate = vi.fn() + stubStream( + { model: 'llama2', message: { content: 'Partial' }, done: false }, + { error: 'llama runner process has terminated' } + ) + + await expect(completeStream(onUpdate)).rejects.toThrow('llama runner process has terminated') + expect(onUpdate).toHaveBeenCalledWith('Partial') + }) + + it('should report a /api/generate stream instead of returning empty content', async () => { + stubStream( + { model: 'llama2', response: 'Hello', done: false }, + { model: 'llama2', response: '', done: true } + ) + + await expect(completeStream()).rejects.toThrow(/Ollama补全请求失败.*缺少 message.*\/api\/chat/) + }) + + it('should report a stream that ends without any message', async () => { + stubStream({ model: 'llama2', done: false }) + + await expect(completeStream()).rejects.toThrow(/缺少 message/) + }) + + it('should report an empty stream', async () => { + stubStream() + + await expect(completeStream()).rejects.toThrow(/缺少 message/) + }) + + it('should accept a chat stream whose message is empty, like the non-streaming branch', async () => { + stubStream({ model: 'llama2', message: { role: 'assistant', content: '' }, done: true }) + + const result = await completeStream() + + expect(result.content).toBe('') + expect(result.finishReason).toBe('stop') + }) + + it('should still skip lines that are not JSON', async () => { + global.fetch = vi.fn().mockResolvedValue(new Response( + 'not json\n{"model":"llama2","message":{"content":"Hello"},"done":false}\n{"model":"llama2","done":true}\n' + )) + + const result = await completeStream() + + expect(result.content).toBe('Hello') + }) + }) }) describe('Tool Support', () => { @@ -335,6 +408,12 @@ describe('OllamaClient', () => { await expect(completeText()).rejects.toThrow(/\/api\/chat/) }) + it('should report an error in a successful response instead of blaming the endpoint', async () => { + mockFetchSuccess({ error: 'llama runner process has terminated' }) + + await expect(completeText()).rejects.toThrow('Ollama 返回错误: llama runner process has terminated') + }) + it('should handle network errors', async () => { global.fetch = vi.fn().mockRejectedValue(new Error('Connection refused')) diff --git a/src/core/infrastructure/db/__tests__/OllamaChatSuffixMigration.test.ts b/src/core/infrastructure/db/__tests__/OllamaChatSuffixMigration.test.ts index 19d0ff7..7134c72 100644 --- a/src/core/infrastructure/db/__tests__/OllamaChatSuffixMigration.test.ts +++ b/src/core/infrastructure/db/__tests__/OllamaChatSuffixMigration.test.ts @@ -6,20 +6,33 @@ import { PrismaClient } from '@prisma/client' import { runMigrations } from '../Migration.js' /** - * Runs the migration through the real runner against a real SQLite file, so the + * Runs the migrations through the real runner against a real SQLite file, so the * runner's statement splitting and the table/column names are exercised too. */ -const MIGRATION_NAME = '20261104000000_ollama_chat_suffix' +const GENERATE_MIGRATION = '20261104000000_ollama_chat_suffix' +const API_CHAT_MIGRATION = '20261104000001_ollama_api_chat_suffix' -describe(`migration ${MIGRATION_NAME}`, () => { +const HOST = 'http://localhost:11434' +const API_BASE = `${HOST}/api` + +describe('Ollama suffix migrations', () => { let dir: string let prisma: PrismaClient - const seed = (name: string, type: string, suffix: string | null) => - prisma.provider.create({ data: { name, type, suffix, base_url: 'http://localhost:11434/api' } }) + const seed = (name: string, type: string, suffix: string | null, base_url: string | null = API_BASE) => + prisma.provider.create({ data: { name, type, suffix, base_url } }) + + const configOf = async (name: string) => { + const { base_url, suffix } = await prisma.provider.findFirstOrThrow({ where: { name } }) + return { base_url, suffix } + } - const suffixOf = async (name: string) => - (await prisma.provider.findFirstOrThrow({ where: { name } })).suffix + const replay = async (...migrations: string[]) => { + for (const migration of migrations) { + await prisma.$executeRaw`DELETE FROM _prisma_migrations WHERE migration_name = ${migration}` + } + await runMigrations(prisma) + } beforeAll(async () => { vi.spyOn(console, 'log').mockImplementation(() => {}) @@ -29,19 +42,26 @@ describe(`migration ${MIGRATION_NAME}`, () => { datasources: { db: { url: `file:${path.join(dir, 'test.db')}` } } }) - // Bring the schema up to date, seed rows as an older version saved them, - // then replay this migration the way an upgrade would. + // Bring the schema up to date, seed rows as older versions saved them, + // then replay the migrations the way an upgrade would. await runMigrations(prisma) await seed('legacy-default', 'ollama', '/generate') await seed('legacy-api-prefixed', 'ollama', '/api/generate') + await seed('legacy-api-prefixed-on-host', 'ollama', '/api/generate', HOST) await seed('already-chat', 'ollama', '/chat') - await seed('already-api-chat', 'ollama', '/api/chat') + await seed('api-chat', 'ollama', '/api/chat') + await seed('api-chat-trailing-slash', 'ollama', '/api/chat', `${API_BASE}/`) + await seed('api-chat-null-base', 'ollama', '/api/chat', null) + await seed('api-chat-empty-base', 'ollama', '/api/chat', '') + await seed('api-chat-on-host', 'ollama', '/api/chat', HOST) + await seed('api-chat-on-host-trailing-slash', 'ollama', '/api/chat', `${HOST}/`) + await seed('api-chat-behind-proxy', 'ollama', '/api/chat', 'https://proxy.test/ollama') await seed('unset-empty', 'ollama', '') await seed('unset-null', 'ollama', null) await seed('custom', 'ollama', '/custom/generate') await seed('other-provider', 'openai', '/generate') - await prisma.$executeRaw`DELETE FROM _prisma_migrations WHERE migration_name = ${MIGRATION_NAME}` - await runMigrations(prisma) + await seed('other-provider-api-chat', 'openai', '/api/chat', HOST) + await replay(GENERATE_MIGRATION, API_CHAT_MIGRATION) }) afterAll(async () => { @@ -50,19 +70,59 @@ describe(`migration ${MIGRATION_NAME}`, () => { }) it('moves saved /generate suffixes of Ollama providers to the chat endpoint', async () => { - expect(await suffixOf('legacy-default')).toBe('/chat') - expect(await suffixOf('legacy-api-prefixed')).toBe('/api/chat') + const expected = { base_url: API_BASE, suffix: '/chat' } + + expect(await configOf('legacy-default')).toEqual(expected) + expect(await configOf('legacy-api-prefixed')).toEqual(expected) + expect(await configOf('legacy-api-prefixed-on-host')).toEqual(expected) + }) + + it.each([ + { name: 'api-chat', expected: { base_url: API_BASE, suffix: '/chat' } }, + { name: 'api-chat-trailing-slash', expected: { base_url: API_BASE, suffix: '/chat' } }, + { name: 'api-chat-null-base', expected: { base_url: null, suffix: '/chat' } }, + { name: 'api-chat-empty-base', expected: { base_url: '', suffix: '/chat' } } + ])('drops the duplicate /api from $name', async ({ name, expected }) => { + expect(await configOf(name)).toEqual(expected) + }) + + it.each([ + { name: 'api-chat-on-host', expected: { base_url: API_BASE, suffix: '/chat' } }, + { name: 'api-chat-on-host-trailing-slash', expected: { base_url: API_BASE, suffix: '/chat' } }, + { name: 'api-chat-behind-proxy', expected: { base_url: 'https://proxy.test/ollama/api', suffix: '/chat' } } + ])('moves /api into the base for $name, which keeps the endpoint and fixes /tags', async ({ name, expected }) => { + expect(await configOf(name)).toEqual(expected) + }) + + it('leaves no Ollama provider on a /api/api endpoint', async () => { + const providers = await prisma.provider.findMany({ where: { type: 'ollama' } }) + + // The same fallbacks as ModelService.getLLMClient + const endpoints = providers.map(({ base_url, suffix }) => `${base_url || API_BASE}${suffix || '/chat'}`) + + expect(endpoints).toContain(`${API_BASE}/chat`) + expect(endpoints.filter((endpoint) => endpoint.includes('/api/api'))).toEqual([]) + expect(providers.filter(({ suffix }) => suffix === '/api/chat')).toEqual([]) }) it('leaves working and unset Ollama suffixes alone', async () => { - expect(await suffixOf('already-chat')).toBe('/chat') - expect(await suffixOf('already-api-chat')).toBe('/api/chat') - expect(await suffixOf('unset-empty')).toBe('') - expect(await suffixOf('unset-null')).toBeNull() + expect(await configOf('already-chat')).toEqual({ base_url: API_BASE, suffix: '/chat' }) + expect(await configOf('unset-empty')).toEqual({ base_url: API_BASE, suffix: '' }) + expect(await configOf('unset-null')).toEqual({ base_url: API_BASE, suffix: null }) }) it('leaves custom suffixes and other providers alone', async () => { - expect(await suffixOf('custom')).toBe('/custom/generate') - expect(await suffixOf('other-provider')).toBe('/generate') + expect(await configOf('custom')).toEqual({ base_url: API_BASE, suffix: '/custom/generate' }) + expect(await configOf('other-provider')).toEqual({ base_url: API_BASE, suffix: '/generate' }) + expect(await configOf('other-provider-api-chat')).toEqual({ base_url: HOST, suffix: '/api/chat' }) + }) + + // The runner has no transaction: a failed migration runs again from its first statement + it('gives the same result when the /api/chat migration runs again', async () => { + const before = await prisma.provider.findMany({ orderBy: { id: 'asc' } }) + + await replay(API_CHAT_MIGRATION) + + expect(await prisma.provider.findMany({ orderBy: { id: 'asc' } })).toEqual(before) }) }) diff --git a/src/core/infrastructure/db/migrations/20261104000001_ollama_api_chat_suffix/migration.sql b/src/core/infrastructure/db/migrations/20261104000001_ollama_api_chat_suffix/migration.sql new file mode 100644 index 0000000..5fccbd7 --- /dev/null +++ b/src/core/infrastructure/db/migrations/20261104000001_ollama_api_chat_suffix/migration.sql @@ -0,0 +1,9 @@ +-- The dropdown no longer offers /api/chat: on a base ending in /api it requests /api/api/chat, and on any other base the model list (base + /tags) fails. +-- Move every saved /api/chat to the one shape where both work: a base ending in /api with the /chat suffix. +-- A base that already ends in /api, or is unset (the runtime default ends in /api), only needs the suffix changed. +UPDATE "Provider" SET "base_url" = RTRIM("base_url", '/'), "suffix" = '/chat' +WHERE "type" = 'ollama' AND "suffix" = '/api/chat' + AND (COALESCE("base_url", '') = '' OR RTRIM("base_url", '/') GLOB '*/api'); +-- Any other base moves /api out of the suffix, so the chat endpoint stays the same URL. +UPDATE "Provider" SET "base_url" = RTRIM("base_url", '/') || '/api', "suffix" = '/chat' +WHERE "type" = 'ollama' AND "suffix" = '/api/chat'; diff --git a/src/renderer/components/Provider.tsx b/src/renderer/components/Provider.tsx index 8e74469..29e5714 100644 --- a/src/renderer/components/Provider.tsx +++ b/src/renderer/components/Provider.tsx @@ -469,9 +469,9 @@ const Provider: React.FC = ({ ]; case "ollama": // 空后缀运行时会回退到 /chat，所以不提供“无后缀” + // base 约定以 /api 结尾（模型列表依赖 base + /tags），所以不提供 /api/chat return [ { label: "/chat", value: "/chat" }, - { label: "/api/chat", value: "/api/chat" }, ]; default: return [noSuffix]; diff --git a/src/renderer/components/__tests__/Provider.test.tsx b/src/renderer/components/__tests__/Provider.test.tsx index 1236938..36c56a4 100644 --- a/src/renderer/components/__tests__/Provider.test.tsx +++ b/src/renderer/components/__tests__/Provider.test.tsx @@ -564,7 +564,7 @@ describe('Provider', () => { }) }) - it('should only offer suffixes that reach the chat endpoint', async () => { + it('should only offer the suffix that reaches the chat endpoint from the default base', async () => { mockOllama('/chat') render( @@ -585,7 +585,7 @@ describe('Provider', () => { const options = Array.from(document.querySelectorAll('.ant-select-item-option-content')) .map(el => el.textContent) - expect(options).toEqual(['/chat', '/api/chat']) + expect(options).toEqual(['/chat']) }) }) })