From 61610d08cdcf1c44cccba4d6759985b936bb72e3 Mon Sep 17 00:00:00 2001 From: IronRookieCoder Date: Mon, 18 May 2026 17:59:13 +0800 Subject: [PATCH] fix(openai): reject invalid streamed tool arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Bug 详情 OpenAI 兼容流返回工具调用时，模型可能输出无法解析为 JSON object 的 arguments，例如 TaskUpdate 参数片段最终拼成非法 JSON。此前这类返回会被任务工具宽松修复或降级为 {}，用户看不到模型返回不合法，工具也可能进入错误执行路径。 ## 根因 normalizeContentFromAPI 在解析 tool_use.input 失败后会尝试 repairTaskToolInput，并在无法修复时回退为空对象；OpenAI 组装层没有把非法工具调用转换成用户可见提示。 ## 修复方案 将工具调用参数作为模型边界输入严格校验：最终工具名必须存在，工具参数必须解析为 JSON object。非法响应抛出 InvalidToolCallResponseError，并在 OpenAI 组装层转换为普通英文 assistant 提示，工具不再执行。 ## 变更要点 - 移除任务工具参数的宽松修复和 {} 降级逻辑 - 新增非法工具调用参数和缺失工具名的显式错误 - OpenAI 组装层捕获非法工具调用并显示普通英文提示 - 增加流式分片省略 name 时保留初始工具名的测试 ## 自测 - bun test src/utils/__tests__/messages.test.ts - bun test packages/@ant/model-provider/src/shared/__tests__/openaiStreamAdapter.test.ts - git diff --check - bun run typecheck 仍失败，失败项为仓库既有类型问题，未涉及本次改动文件 --- .../__tests__/openaiStreamAdapter.test.ts | 74 +++++++ src/services/api/openai/index.ts | 45 ++-- src/utils/__tests__/messages.test.ts | 202 +++++++----------- src/utils/messages.ts | 179 +++------------- 4 files changed, 204 insertions(+), 296 deletions(-) diff --git a/packages/@ant/model-provider/src/shared/__tests__/openaiStreamAdapter.test.ts b/packages/@ant/model-provider/src/shared/__tests__/openaiStreamAdapter.test.ts index d44bc16aa..a2745faa8 100644 --- a/packages/@ant/model-provider/src/shared/__tests__/openaiStreamAdapter.test.ts +++ b/packages/@ant/model-provider/src/shared/__tests__/openaiStreamAdapter.test.ts @@ -390,6 +390,80 @@ describe('adaptOpenAIStreamToAnthropic', () => { ) }) + test('keeps the initial tool name when later argument chunks omit it', async () => { + const events = await collectEvents([ + makeChunk({ + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { + index: 0, + id: 'call_task_update', + type: 'function', + function: { name: 'TaskUpdate', arguments: '' }, + }, + ], + }, + finish_reason: null, + }, + ], + }), + makeChunk({ + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { + index: 0, + type: 'function', + function: { arguments: '{"status": ' }, + }, + ], + }, + finish_reason: null, + }, + ], + }), + makeChunk({ + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { + index: 0, + type: 'function', + function: { arguments: 'in_progress' }, + }, + ], + }, + finish_reason: null, + }, + ], + }), + makeChunk({ + choices: [{ index: 0, delta: {}, finish_reason: 'tool_calls' }], + }), + ]) + + const blockStarts = events.filter( + e => e.type === 'content_block_start', + ) as any[] + expect(blockStarts).toHaveLength(1) + expect(blockStarts[0].content_block.name).toBe('TaskUpdate') + + const jsonDeltas = events.filter( + e => + e.type === 'content_block_delta' && e.delta.type === 'input_json_delta', + ) as any[] + expect(jsonDeltas.map(e => e.delta.partial_json).join('')).toBe( + '{"status": in_progress', + ) + }) + test('maps finish_reason stop to end_turn', async () => { const events = await collectEvents([ makeChunk({ diff --git a/src/services/api/openai/index.ts b/src/services/api/openai/index.ts index 520290b18..4f0dde5e4 100644 --- a/src/services/api/openai/index.ts +++ b/src/services/api/openai/index.ts @@ -54,6 +54,8 @@ import type { Options } from '../claude.js' import { randomUUID } from 'crypto' import { createAssistantAPIErrorMessage, + createAssistantMessage, + InvalidToolCallResponseError, createUserMessage, normalizeContentFromAPI, } from '../../../utils/messages.js' @@ -166,23 +168,32 @@ function assembleFinalAssistantOutputs(params: { .filter(Boolean) if (allBlocks.length > 0) { - outputs.push({ - message: { - ...partialMessage, - content: normalizeContentFromAPI( - allBlocks, - tools, - agentId as AgentId | undefined, - ), - usage, - stop_reason: stopReason, - stop_sequence: null, - }, - requestId: undefined, - type: 'assistant', - uuid: randomUUID(), - timestamp: new Date().toISOString(), - } as AssistantMessage) + try { + outputs.push({ + message: { + ...partialMessage, + content: normalizeContentFromAPI( + allBlocks, + tools, + agentId as AgentId | undefined, + ), + usage, + stop_reason: stopReason, + stop_sequence: null, + }, + requestId: undefined, + type: 'assistant', + uuid: randomUUID(), + timestamp: new Date().toISOString(), + } as AssistantMessage) + } catch (error) { + if (!(error instanceof InvalidToolCallResponseError)) throw error + outputs.push( + createAssistantMessage({ + content: error.message, + }), + ) + } } if (stopReason === 'max_tokens') { diff --git a/src/utils/__tests__/messages.test.ts b/src/utils/__tests__/messages.test.ts index 454a2806f..6b833b679 100644 --- a/src/utils/__tests__/messages.test.ts +++ b/src/utils/__tests__/messages.test.ts @@ -206,87 +206,8 @@ describe('createToolResultStopMessage', () => { }) }) -describe('normalizeContentFromAPI task tool input repair', () => { - test('repairs unquoted TaskUpdate status and preserves taskId', () => { - const content = normalizeContentFromAPI( - [ - { - type: 'tool_use', - id: 'toolu_1', - name: 'TaskUpdate', - input: '{"status": in_progresss", "taskId": "1"}', - } as any, - ], - [TaskUpdateTool] as any, - ) - - expect((content[0] as any).input).toEqual({ - status: 'in_progress', - taskId: '1', - }) - }) - - test('repairs misspelled completed TaskUpdate status', () => { - const content = normalizeContentFromAPI( - [ - { - type: 'tool_use', - id: 'toolu_1', - name: 'TaskUpdate', - input: '{"status": completedd", "taskId": "2"}', - } as any, - ], - [TaskUpdateTool] as any, - ) - - expect((content[0] as any).input).toEqual({ - status: 'completed', - taskId: '2', - }) - }) - - test('repairs common TaskUpdate aliases and loose scalar values', () => { - const content = normalizeContentFromAPI( - [ - { - type: 'tool_use', - id: 'toolu_1', - name: 'TaskUpdate', - input: - "{'status': done, 'task_id': 2, 'addBlockedBy': [1, '3'], 'active_form': 'Running checks'}", - } as any, - ], - [TaskUpdateTool] as any, - ) - - expect((content[0] as any).input).toEqual({ - status: 'completed', - taskId: '2', - addBlockedBy: ['1', '3'], - activeForm: 'Running checks', - }) - }) - - test('repairs unterminated TaskUpdate string values', () => { - const content = normalizeContentFromAPI( - [ - { - type: 'tool_use', - id: 'toolu_1', - name: 'TaskUpdate', - input: '{"taskId": "4", "subject": "Run final verification', - } as any, - ], - [TaskUpdateTool] as any, - ) - - expect((content[0] as any).input).toEqual({ - taskId: '4', - subject: 'Run final verification', - }) - }) - - test('does not repair malformed non-task tool input', () => { +describe('normalizeContentFromAPI tool input validation', () => { + test('normalizes valid tool input objects', () => { const content = normalizeContentFromAPI( [ { @@ -298,69 +219,96 @@ describe('normalizeContentFromAPI task tool input repair', () => { { type: 'tool_use', id: 'toolu_2', - name: 'Bash', - input: '{"command": ls"}', + name: 'TaskUpdate', + input: '{"status":"completed","taskId":"1"}', } as any, ], - [TaskGetTool] as any, + [TaskGetTool, TaskUpdateTool] as any, ) expect((content[0] as any).input).toEqual({ taskId: '1' }) - expect((content[1] as any).input).toEqual({}) + expect((content[1] as any).input).toEqual({ + status: 'completed', + taskId: '1', + }) }) - test('does not repair task inputs missing required fields', () => { + test('rejects malformed JSON tool inputs', () => { + for (const input of [ + '{"status": in_progresss", "taskId": "1"}', + '{"status": completedd", "taskId": "2"}', + 'in_progress', + '', + ]) { + expect(() => + normalizeContentFromAPI( + [ + { + type: 'tool_use', + id: 'toolu_1', + name: 'TaskUpdate', + input, + } as any, + ], + [TaskUpdateTool] as any, + ), + ).toThrow( + 'Model response error: invalid tool call arguments for TaskUpdate. The tool call was not executed.', + ) + } + }) + + test('rejects non-object JSON tool inputs', () => { + for (const input of ['null', '[]', '"in_progress"', '1']) { + expect(() => + normalizeContentFromAPI( + [ + { + type: 'tool_use', + id: 'toolu_1', + name: 'TaskUpdate', + input, + } as any, + ], + [TaskUpdateTool] as any, + ), + ).toThrow( + 'Model response error: invalid tool call arguments for TaskUpdate. The tool call was not executed.', + ) + } + }) + + test('rejects tool calls without a final name', () => { + expect(() => + normalizeContentFromAPI( + [ + { + type: 'tool_use', + id: 'toolu_1', + name: '', + input: '{"taskId":"1"}', + } as any, + ], + [TaskGetTool] as any, + ), + ).toThrow( + 'Model response error: invalid tool call without a tool name. The tool call was not executed.', + ) + }) + + test('preserves valid empty object input', () => { const content = normalizeContentFromAPI( [ { type: 'tool_use', id: 'toolu_1', - name: 'TaskCreate', - input: '{"subject": "missing description"', - } as any, - { - type: 'tool_use', - id: 'toolu_2', - name: 'TaskUpdate', - input: '{"status": completedd"}', + name: 'TaskList', + input: '{}', } as any, ], [] as any, ) - expect((content[0] as any).input).toEqual({}) - expect((content[1] as any).input).toEqual({}) - }) - - test('does not repair TaskUpdate into a no-op when status is invalid', () => { - const content = normalizeContentFromAPI( - [ - { - type: 'tool_use', - id: 'toolu_1', - name: 'TaskUpdate', - input: '{"status": unknown_state", "taskId": "2"}', - } as any, - ], - [TaskUpdateTool] as any, - ) - - expect((content[0] as any).input).toEqual({}) - }) - - test('does not repair TaskUpdate with only taskId', () => { - const content = normalizeContentFromAPI( - [ - { - type: 'tool_use', - id: 'toolu_1', - name: 'TaskUpdate', - input: '{"taskId": "2"', - } as any, - ], - [TaskUpdateTool] as any, - ) - expect((content[0] as any).input).toEqual({}) }) }) diff --git a/src/utils/messages.ts b/src/utils/messages.ts index aab561801..6b0429934 100644 --- a/src/utils/messages.ts +++ b/src/utils/messages.ts @@ -142,8 +142,6 @@ import { } from '@claude-code-best/builtin-tools/tools/FileReadTool/FileReadTool.js' import { SEND_MESSAGE_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/SendMessageTool/constants.js' import { TASK_CREATE_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/TaskCreateTool/constants.js' -import { TASK_GET_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/TaskGetTool/constants.js' -import { TASK_LIST_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/TaskListTool/constants.js' import { TASK_OUTPUT_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/TaskOutputTool/constants.js' import { TASK_UPDATE_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/TaskUpdateTool/constants.js' import type { PermissionMode } from '../types/permissions.js' @@ -180,153 +178,21 @@ const MEMORY_CORRECTION_HINT = "\n\nNote: The user's next message may contain a correction or preference. Pay close attention — if they explain what went wrong or how they'd prefer you to work, consider saving that to memory for future sessions." const TOOL_REFERENCE_TURN_BOUNDARY = 'Tool loaded.' -const TASK_TOOL_NAMES = new Set([ - TASK_CREATE_TOOL_NAME, - TASK_GET_TOOL_NAME, - TASK_LIST_TOOL_NAME, - TASK_UPDATE_TOOL_NAME, -]) -const TASK_STATUS_VALUES = ['pending', 'in_progress', 'completed', 'deleted'] - -function normalizeLooseTaskStatus(value: string): string { - const key = value.trim().toLowerCase().replace(/[\s-]+/g, '_') - if (key === 'open' || key === 'todo') return 'pending' - if ( - key === 'started' || - key === 'active' || - key === 'inprogress' || - key.startsWith('in_progress') - ) { - return 'in_progress' +export class InvalidToolCallResponseError extends Error { + constructor(message: string) { + super(message) + this.name = 'InvalidToolCallResponseError' } - if ( - key === 'complete' || - key === 'done' || - key === 'resolved' || - key === 'closed' || - key.startsWith('completed') || - key.startsWith('complete') - ) { - return 'completed' - } - if (key === 'delete' || key === 'removed' || key.startsWith('deleted')) { - return 'deleted' - } - return value } -function extractLooseTaskStringField(raw: string, key: string): string | null { - const quoted = new RegExp(`["']${key}["']\\s*:\\s*["']([^"']*)`).exec(raw) - if (quoted) return quoted[1] ?? '' - - const bare = new RegExp(`["']${key}["']\\s*:\\s*([^,}\\n\\r]+)`).exec(raw) - if (!bare) return null - - return (bare[1] ?? '') - .trim() - .replace(/^["']/, '') - .replace(/["']$/, '') +function isPlainToolInput(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) } -function extractLooseTaskStringArray(raw: string, key: string): string[] | null { - const match = new RegExp(`["']${key}["']\\s*:\\s*\\[([^\\]]*)\\]`).exec(raw) - if (!match) return null - - return (match[1] ?? '') - .split(',') - .map(item => - item - .trim() - .replace(/^["']/, '') - .replace(/["']$/, ''), - ) - .filter(Boolean) -} - -function repairTaskToolInput(toolName: string, rawInput: string): unknown { - if (!TASK_TOOL_NAMES.has(toolName)) return null - - const repaired: Record = {} - - if (toolName === TASK_LIST_TOOL_NAME) { - return rawInput.trim() === '' || rawInput.trim() === '{' ? {} : null - } - - for (const key of ['taskId', 'task_id', 'id']) { - const value = extractLooseTaskStringField(rawInput, key) - if (value !== null) { - repaired.taskId = value - break - } - } - - if (toolName === TASK_UPDATE_TOOL_NAME) { - const status = extractLooseTaskStringField(rawInput, 'status') - if (status !== null) { - const normalizedStatus = normalizeLooseTaskStatus(status) - if (TASK_STATUS_VALUES.includes(normalizedStatus)) { - repaired.status = normalizedStatus - } else { - return null - } - } - - for (const key of ['subject', 'description', 'activeForm', 'owner']) { - const value = extractLooseTaskStringField(rawInput, key) - if (value !== null) repaired[key] = value - } - - const activeForm = extractLooseTaskStringField(rawInput, 'active_form') - if (activeForm !== null && repaired.activeForm === undefined) { - repaired.activeForm = activeForm - } - - for (const [sourceKey, targetKey] of [ - ['addBlocks', 'addBlocks'], - ['add_blocks', 'addBlocks'], - ['blocks', 'addBlocks'], - ['addBlockedBy', 'addBlockedBy'], - ['add_blocked_by', 'addBlockedBy'], - ['blockedBy', 'addBlockedBy'], - ['blocked_by', 'addBlockedBy'], - ] as const) { - const value = extractLooseTaskStringArray(rawInput, sourceKey) - if (value !== null && repaired[targetKey] === undefined) { - repaired[targetKey] = value - } - } - } else if (toolName === TASK_CREATE_TOOL_NAME) { - for (const key of ['subject', 'description', 'activeForm']) { - const value = extractLooseTaskStringField(rawInput, key) - if (value !== null) repaired[key] = value - } - const activeForm = extractLooseTaskStringField(rawInput, 'active_form') - if (activeForm !== null && repaired.activeForm === undefined) { - repaired.activeForm = activeForm - } - } - - if ( - (toolName === TASK_GET_TOOL_NAME || toolName === TASK_UPDATE_TOOL_NAME) && - typeof repaired.taskId !== 'string' - ) { - return null - } - if ( - toolName === TASK_UPDATE_TOOL_NAME && - !Object.keys(repaired).some(key => key !== 'taskId') - ) { - return null - } - if ( - toolName === TASK_CREATE_TOOL_NAME && - (typeof repaired.subject !== 'string' || - typeof repaired.description !== 'string') - ) { - return null - } - - return Object.keys(repaired).length > 0 ? repaired : null +function createInvalidToolArgumentsError(toolName: string): InvalidToolCallResponseError { + return new InvalidToolCallResponseError( + `Model response error: invalid tool call arguments for ${toolName}. The tool call was not executed.`, + ) } /** @@ -3118,6 +2984,12 @@ export function normalizeContentFromAPI( return contentBlocks.map(contentBlock => { switch (contentBlock.type) { case 'tool_use': { + if (typeof contentBlock.name !== 'string' || contentBlock.name === '') { + throw new InvalidToolCallResponseError( + 'Model response error: invalid tool call without a tool name. The tool call was not executed.', + ) + } + if ( typeof contentBlock.input !== 'string' && !isObject(contentBlock.input) @@ -3134,11 +3006,10 @@ export function normalizeContentFromAPI( let normalizedInput: unknown if (typeof contentBlock.input === 'string') { const parsed = safeParseJSON(contentBlock.input) - if (parsed === null && contentBlock.input.length > 0) { + if (parsed === null) { // TET/FC-v3 diagnostic: the streamed tool input JSON failed to - // parse. We fall back to {} which means downstream validation - // sees empty input. The raw prefix goes to debug log only — no - // PII-tagged proto column exists for it yet. + // parse. The raw prefix goes to debug log only — no PII-tagged + // proto column exists for it yet. logEvent('tengu_tool_input_json_parse_fail', { toolName: sanitizeToolNameForAnalytics(contentBlock.name), inputLen: contentBlock.input.length, @@ -3149,12 +3020,16 @@ export function normalizeContentFromAPI( { level: 'warn' }, ) } + throw createInvalidToolArgumentsError(contentBlock.name) } - normalizedInput = - parsed ?? - repairTaskToolInput(contentBlock.name, contentBlock.input) ?? - {} + if (!isPlainToolInput(parsed)) { + throw createInvalidToolArgumentsError(contentBlock.name) + } + normalizedInput = parsed } else { + if (!isPlainToolInput(contentBlock.input)) { + throw createInvalidToolArgumentsError(contentBlock.name) + } normalizedInput = contentBlock.input }