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

@@ -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",

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",

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',

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<OpenAI.Chat.ChatCompletionChunk>;
@@ -741,6 +740,16 @@ export class OpenAIContentGenerator implements ContentGenerator {
     return convertTypes(converted) as Record<string, unknown> | 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<OpenAI.Chat.ChatCompletionTool[]> {
@@ -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<string, unknown> | 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<string, unknown>),
+                };
+                parameters = paramsCopy;
+              }
+            } else if (func.parameters) {
+              // Gemini tool format - convert parameters to OpenAI format
+              parameters = this.convertGeminiParametersToOpenAI(
+                func.parameters as Record<string, unknown>,
+              );
+            }
+
             openAITools.push({
               type: 'function',
               function: {
                 name: func.name,
                 description: func.description,
-                parameters: this.convertGeminiParametersToOpenAI(
-                  (func.parameters || {}) as Record<string, unknown>,
-                ),
+                parameters,
               },
             });
           }
@@ -1147,12 +1173,7 @@ export class OpenAIContentGenerator implements ContentGenerator {
         if (toolCall.function) {
           let args: Record<string, unknown> = {};
           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<string, unknown> = {};
             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({

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');
+    });
+  });
+});

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<T = Record<string, unknown>>(
+  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;
+    }
+  }
+}