From 2403061bab6fb7c2ea98446ec19b3d0760998424 Mon Sep 17 00:00:00 2001 From: Mingholy Date: Thu, 14 Aug 2025 21:18:26 +0800 Subject: [PATCH] fix: OpenAI tools (#328) - MCP tool params schema lost causing all MCP not working well - Compatible with occasional llm return tool call parameters that are invalid json --- package-lock.json | 10 ++ packages/core/package.json | 1 + .../src/core/openaiContentGenerator.test.ts | 84 ++++++++++ .../core/src/core/openaiContentGenerator.ts | 52 +++--- packages/core/src/utils/safeJsonParse.test.ts | 149 ++++++++++++++++++ packages/core/src/utils/safeJsonParse.ts | 45 ++++++ 6 files changed, 322 insertions(+), 19 deletions(-) create mode 100644 packages/core/src/utils/safeJsonParse.test.ts create mode 100644 packages/core/src/utils/safeJsonParse.ts diff --git a/package-lock.json b/package-lock.json index 6594f006..2146c8f0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7421,6 +7421,15 @@ "json5": "lib/cli.js" } }, + "node_modules/jsonrepair": { + "version": "3.13.0", + "resolved": "https://registry.npmjs.org/jsonrepair/-/jsonrepair-3.13.0.tgz", + "integrity": "sha512-5YRzlAQ7tuzV1nAJu3LvDlrKtBFIALHN2+a+I1MGJCt3ldRDBF/bZuvIPzae8Epot6KBXd0awRZZcuoeAsZ/mw==", + "license": "ISC", + "bin": { + "jsonrepair": "bin/cli.js" + } + }, "node_modules/jsx-ast-utils": { "version": "3.3.5", "resolved": "https://registry.npmjs.org/jsx-ast-utils/-/jsx-ast-utils-3.3.5.tgz", @@ -11916,6 +11925,7 @@ "html-to-text": "^9.0.5", "https-proxy-agent": "^7.0.6", "ignore": "^7.0.0", + "jsonrepair": "^3.13.0", "marked": "^15.0.12", "micromatch": "^4.0.8", "open": "^10.1.2", diff --git a/packages/core/package.json b/packages/core/package.json index 41b79d1f..f22939b0 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -39,6 +39,7 @@ "html-to-text": "^9.0.5", "https-proxy-agent": "^7.0.6", "ignore": "^7.0.0", + "jsonrepair": "^3.13.0", "marked": "^15.0.12", "micromatch": "^4.0.8", "open": "^10.1.2", diff --git a/packages/core/src/core/openaiContentGenerator.test.ts b/packages/core/src/core/openaiContentGenerator.test.ts index 637c6135..92de6235 100644 --- a/packages/core/src/core/openaiContentGenerator.test.ts +++ b/packages/core/src/core/openaiContentGenerator.test.ts @@ -1160,6 +1160,90 @@ describe('OpenAIContentGenerator', () => { ); }); + it('should handle MCP tools with parametersJsonSchema', async () => { + const mockResponse = { + id: 'chatcmpl-123', + choices: [ + { + index: 0, + message: { role: 'assistant', content: 'Response' }, + finish_reason: 'stop', + }, + ], + created: 1677652288, + model: 'gpt-4', + }; + + mockOpenAIClient.chat.completions.create.mockResolvedValue(mockResponse); + + const request: GenerateContentParameters = { + contents: [{ role: 'user', parts: [{ text: 'Test' }] }], + model: 'gpt-4', + config: { + tools: [ + { + callTool: vi.fn(), + tool: () => + Promise.resolve({ + functionDeclarations: [ + { + name: 'list-items', + description: 'Get a list of items', + parametersJsonSchema: { + type: 'object', + properties: { + page_number: { + type: 'number', + description: 'Page number', + }, + page_size: { + type: 'number', + description: 'Number of items per page', + }, + }, + additionalProperties: false, + $schema: 'http://json-schema.org/draft-07/schema#', + }, + }, + ], + }), + } as unknown as CallableTool, + ], + }, + }; + + await generator.generateContent(request, 'test-prompt-id'); + + expect(mockOpenAIClient.chat.completions.create).toHaveBeenCalledWith( + expect.objectContaining({ + tools: [ + { + type: 'function', + function: { + name: 'list-items', + description: 'Get a list of items', + parameters: { + type: 'object', + properties: { + page_number: { + type: 'number', + description: 'Page number', + }, + page_size: { + type: 'number', + description: 'Number of items per page', + }, + }, + additionalProperties: false, + $schema: 'http://json-schema.org/draft-07/schema#', + }, + }, + }, + ], + }), + ); + }); + it('should handle nested parameter objects', async () => { const mockResponse = { id: 'chatcmpl-123', diff --git a/packages/core/src/core/openaiContentGenerator.ts b/packages/core/src/core/openaiContentGenerator.ts index 16404d30..b7d0ad1b 100644 --- a/packages/core/src/core/openaiContentGenerator.ts +++ b/packages/core/src/core/openaiContentGenerator.ts @@ -26,6 +26,7 @@ import { logApiError, logApiResponse } from '../telemetry/loggers.js'; import { ApiErrorEvent, ApiResponseEvent } from '../telemetry/types.js'; import { Config } from '../config/config.js'; import { openaiLogger } from '../utils/openaiLogger.js'; +import { safeJsonParse } from '../utils/safeJsonParse.js'; // OpenAI API type definitions for logging interface OpenAIToolCall { @@ -365,8 +366,6 @@ export class OpenAIContentGenerator implements ContentGenerator { ); } - // console.log('createParams', createParams); - const stream = (await this.client.chat.completions.create( createParams, )) as AsyncIterable; @@ -741,6 +740,16 @@ export class OpenAIContentGenerator implements ContentGenerator { return convertTypes(converted) as Record | undefined; } + /** + * Converts Gemini tools to OpenAI format for API compatibility. + * Handles both Gemini tools (using 'parameters' field) and MCP tools (using 'parametersJsonSchema' field). + * + * Gemini tools use a custom parameter format that needs conversion to OpenAI JSON Schema format. + * MCP tools already use JSON Schema format in the parametersJsonSchema field and can be used directly. + * + * @param geminiTools - Array of Gemini tools to convert + * @returns Promise resolving to array of OpenAI-compatible tools + */ private async convertGeminiToolsToOpenAI( geminiTools: ToolListUnion, ): Promise { @@ -761,14 +770,31 @@ export class OpenAIContentGenerator implements ContentGenerator { if (actualTool.functionDeclarations) { for (const func of actualTool.functionDeclarations) { if (func.name && func.description) { + let parameters: Record | undefined; + + // Handle both Gemini tools (parameters) and MCP tools (parametersJsonSchema) + if (func.parametersJsonSchema) { + // MCP tool format - use parametersJsonSchema directly + if (func.parametersJsonSchema) { + // Create a shallow copy to avoid mutating the original object + const paramsCopy = { + ...(func.parametersJsonSchema as Record), + }; + parameters = paramsCopy; + } + } else if (func.parameters) { + // Gemini tool format - convert parameters to OpenAI format + parameters = this.convertGeminiParametersToOpenAI( + func.parameters as Record, + ); + } + openAITools.push({ type: 'function', function: { name: func.name, description: func.description, - parameters: this.convertGeminiParametersToOpenAI( - (func.parameters || {}) as Record, - ), + parameters, }, }); } @@ -1147,12 +1173,7 @@ export class OpenAIContentGenerator implements ContentGenerator { if (toolCall.function) { let args: Record = {}; if (toolCall.function.arguments) { - try { - args = JSON.parse(toolCall.function.arguments); - } catch (error) { - console.error('Failed to parse function arguments:', error); - args = {}; - } + args = safeJsonParse(toolCall.function.arguments, {}); } parts.push({ @@ -1264,14 +1285,7 @@ export class OpenAIContentGenerator implements ContentGenerator { if (accumulatedCall.name) { let args: Record = {}; if (accumulatedCall.arguments) { - try { - args = JSON.parse(accumulatedCall.arguments); - } catch (error) { - console.error( - 'Failed to parse final tool call arguments:', - error, - ); - } + args = safeJsonParse(accumulatedCall.arguments, {}); } parts.push({ diff --git a/packages/core/src/utils/safeJsonParse.test.ts b/packages/core/src/utils/safeJsonParse.test.ts new file mode 100644 index 00000000..e2463f91 --- /dev/null +++ b/packages/core/src/utils/safeJsonParse.test.ts @@ -0,0 +1,149 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { safeJsonParse } from './safeJsonParse.js'; + +describe('safeJsonParse', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('valid JSON parsing', () => { + it('should parse valid JSON correctly', () => { + const validJson = '{"name": "test", "value": 123}'; + const result = safeJsonParse(validJson); + + expect(result).toEqual({ name: 'test', value: 123 }); + }); + + it('should parse valid JSON arrays', () => { + const validArray = '["item1", "item2", "item3"]'; + const result = safeJsonParse(validArray); + + expect(result).toEqual(['item1', 'item2', 'item3']); + }); + + it('should parse valid JSON with nested objects', () => { + const validNested = + '{"config": {"paths": ["testlogs/*.py"], "options": {"recursive": true}}}'; + const result = safeJsonParse(validNested); + + expect(result).toEqual({ + config: { + paths: ['testlogs/*.py'], + options: { recursive: true }, + }, + }); + }); + }); + + describe('malformed JSON with jsonrepair fallback', () => { + it('should handle malformed JSON with single quotes', () => { + const malformedJson = "{'name': 'test', 'value': 123}"; + const result = safeJsonParse(malformedJson); + + expect(result).toEqual({ name: 'test', value: 123 }); + }); + + it('should handle malformed JSON with unquoted keys', () => { + const malformedJson = '{name: "test", value: 123}'; + const result = safeJsonParse(malformedJson); + + expect(result).toEqual({ name: 'test', value: 123 }); + }); + + it('should handle malformed JSON with trailing commas', () => { + const malformedJson = '{"name": "test", "value": 123,}'; + const result = safeJsonParse(malformedJson); + + expect(result).toEqual({ name: 'test', value: 123 }); + }); + + it('should handle malformed JSON with comments', () => { + const malformedJson = '{"name": "test", // comment\n "value": 123}'; + const result = safeJsonParse(malformedJson); + + expect(result).toEqual({ name: 'test', value: 123 }); + }); + }); + + describe('fallback behavior', () => { + it('should return fallback value for empty string', () => { + const emptyString = ''; + const fallback = { default: 'value' }; + + const result = safeJsonParse(emptyString, fallback); + + expect(result).toEqual(fallback); + }); + + it('should return fallback value for null input', () => { + const nullInput = null as unknown as string; + const fallback = { default: 'value' }; + + const result = safeJsonParse(nullInput, fallback); + + expect(result).toEqual(fallback); + }); + + it('should return fallback value for undefined input', () => { + const undefinedInput = undefined as unknown as string; + const fallback = { default: 'value' }; + + const result = safeJsonParse(undefinedInput, fallback); + + expect(result).toEqual(fallback); + }); + + it('should return empty object as default fallback', () => { + const invalidJson = 'invalid json'; + + const result = safeJsonParse(invalidJson); + + // jsonrepair returns the original string for completely invalid JSON + expect(result).toEqual('invalid json'); + }); + + it('should return custom fallback when parsing fails', () => { + const invalidJson = 'invalid json'; + const customFallback = { error: 'parsing failed', data: null }; + + const result = safeJsonParse(invalidJson, customFallback); + + // jsonrepair returns the original string for completely invalid JSON + expect(result).toEqual('invalid json'); + }); + }); + + describe('type safety', () => { + it('should preserve generic type when parsing valid JSON', () => { + const validJson = '{"name": "test", "value": 123}'; + const result = safeJsonParse<{ name: string; value: number }>(validJson); + + expect(result).toEqual({ name: 'test', value: 123 }); + // TypeScript should infer the correct type + expect(typeof result.name).toBe('string'); + expect(typeof result.value).toBe('number'); + }); + + it('should return fallback type when parsing fails', () => { + const invalidJson = 'invalid json'; + const fallback = { error: 'fallback' } as const; + + const result = safeJsonParse(invalidJson, fallback); + + // jsonrepair returns the original string for completely invalid JSON + expect(result).toEqual('invalid json'); + // TypeScript should preserve the fallback type + expect(typeof result).toBe('string'); + }); + }); +}); diff --git a/packages/core/src/utils/safeJsonParse.ts b/packages/core/src/utils/safeJsonParse.ts new file mode 100644 index 00000000..03dc6a27 --- /dev/null +++ b/packages/core/src/utils/safeJsonParse.ts @@ -0,0 +1,45 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { jsonrepair } from 'jsonrepair'; + +/** + * Safely parse JSON string with jsonrepair fallback for malformed JSON. + * This function attempts to parse JSON normally first, and if that fails, + * it uses jsonrepair to fix common JSON formatting issues before parsing. + * + * @param jsonString - The JSON string to parse + * @param fallbackValue - The value to return if parsing fails completely + * @returns The parsed object or the fallback value + */ +export function safeJsonParse>( + jsonString: string, + fallbackValue: T = {} as T, +): T { + if (!jsonString || typeof jsonString !== 'string') { + return fallbackValue; + } + + try { + // First attempt: try normal JSON.parse + return JSON.parse(jsonString) as T; + } catch (error) { + try { + // Second attempt: use jsonrepair to fix common JSON issues + const repairedJson = jsonrepair(jsonString); + + // jsonrepair always returns a string, so we need to parse it + return JSON.parse(repairedJson) as T; + } catch (repairError) { + console.error('Failed to parse JSON even with jsonrepair:', { + originalError: error, + repairError, + jsonString, + }); + return fallbackValue; + } + } +}