Преглед изворни кода

fix: add additionalProperties: false to MCP tool schemas for OpenAI Responses API (#10472)

Daniel пре 1 месец
родитељ
комит
6b13d1d84e

+ 283 - 0
src/api/providers/__tests__/base-provider.spec.ts

@@ -0,0 +1,283 @@
+import { Anthropic } from "@anthropic-ai/sdk"
+
+import type { ModelInfo } from "@roo-code/types"
+
+import { BaseProvider } from "../base-provider"
+import type { ApiStream } from "../../transform/stream"
+
+// Create a concrete implementation for testing
+class TestProvider extends BaseProvider {
+	createMessage(_systemPrompt: string, _messages: Anthropic.Messages.MessageParam[]): ApiStream {
+		throw new Error("Not implemented")
+	}
+
+	getModel(): { id: string; info: ModelInfo } {
+		return {
+			id: "test-model",
+			info: {
+				maxTokens: 4096,
+				contextWindow: 128000,
+				supportsPromptCache: false,
+			},
+		}
+	}
+
+	// Expose protected method for testing
+	public testConvertToolSchemaForOpenAI(schema: any): any {
+		return this.convertToolSchemaForOpenAI(schema)
+	}
+
+	// Expose protected method for testing
+	public testConvertToolsForOpenAI(tools: any[] | undefined): any[] | undefined {
+		return this.convertToolsForOpenAI(tools)
+	}
+}
+
+describe("BaseProvider", () => {
+	let provider: TestProvider
+
+	beforeEach(() => {
+		provider = new TestProvider()
+	})
+
+	describe("convertToolSchemaForOpenAI", () => {
+		it("should add additionalProperties: false to object schemas", () => {
+			const schema = {
+				type: "object",
+				properties: {
+					name: { type: "string" },
+				},
+			}
+
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result.additionalProperties).toBe(false)
+		})
+
+		it("should add required array with all properties for strict mode", () => {
+			const schema = {
+				type: "object",
+				properties: {
+					name: { type: "string" },
+					age: { type: "number" },
+				},
+			}
+
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result.required).toEqual(["name", "age"])
+		})
+
+		it("should recursively add additionalProperties: false to nested objects", () => {
+			const schema = {
+				type: "object",
+				properties: {
+					user: {
+						type: "object",
+						properties: {
+							name: { type: "string" },
+						},
+					},
+				},
+			}
+
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result.additionalProperties).toBe(false)
+			expect(result.properties.user.additionalProperties).toBe(false)
+		})
+
+		it("should recursively add additionalProperties: false to array item objects", () => {
+			const schema = {
+				type: "object",
+				properties: {
+					users: {
+						type: "array",
+						items: {
+							type: "object",
+							properties: {
+								name: { type: "string" },
+							},
+						},
+					},
+				},
+			}
+
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result.additionalProperties).toBe(false)
+			expect(result.properties.users.items.additionalProperties).toBe(false)
+		})
+
+		it("should handle deeply nested objects", () => {
+			const schema = {
+				type: "object",
+				properties: {
+					level1: {
+						type: "object",
+						properties: {
+							level2: {
+								type: "object",
+								properties: {
+									level3: {
+										type: "object",
+										properties: {
+											value: { type: "string" },
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			}
+
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result.additionalProperties).toBe(false)
+			expect(result.properties.level1.additionalProperties).toBe(false)
+			expect(result.properties.level1.properties.level2.additionalProperties).toBe(false)
+			expect(result.properties.level1.properties.level2.properties.level3.additionalProperties).toBe(false)
+		})
+
+		it("should convert nullable types to non-nullable", () => {
+			const schema = {
+				type: "object",
+				properties: {
+					name: { type: ["string", "null"] },
+				},
+			}
+
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result.properties.name.type).toBe("string")
+		})
+
+		it("should return non-object schemas unchanged", () => {
+			const schema = { type: "string" }
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result).toEqual(schema)
+		})
+
+		it("should return null/undefined unchanged", () => {
+			expect(provider.testConvertToolSchemaForOpenAI(null)).toBeNull()
+			expect(provider.testConvertToolSchemaForOpenAI(undefined)).toBeUndefined()
+		})
+
+		it("should handle empty properties object", () => {
+			const schema = {
+				type: "object",
+				properties: {},
+			}
+
+			const result = provider.testConvertToolSchemaForOpenAI(schema)
+
+			expect(result.additionalProperties).toBe(false)
+			expect(result.required).toEqual([])
+		})
+	})
+
+	describe("convertToolsForOpenAI", () => {
+		it("should return undefined for undefined input", () => {
+			const result = provider.testConvertToolsForOpenAI(undefined)
+			expect(result).toBeUndefined()
+		})
+
+		it("should set strict: true for non-MCP tools", () => {
+			const tools = [
+				{
+					type: "function",
+					function: {
+						name: "read_file",
+						description: "Read a file",
+						parameters: { type: "object", properties: {} },
+					},
+				},
+			]
+
+			const result = provider.testConvertToolsForOpenAI(tools)
+
+			expect(result?.[0].function.strict).toBe(true)
+		})
+
+		it("should set strict: false for MCP tools (mcp-- prefix)", () => {
+			const tools = [
+				{
+					type: "function",
+					function: {
+						name: "mcp--github--get_me",
+						description: "Get current user",
+						parameters: { type: "object", properties: {} },
+					},
+				},
+			]
+
+			const result = provider.testConvertToolsForOpenAI(tools)
+
+			expect(result?.[0].function.strict).toBe(false)
+		})
+
+		it("should apply schema conversion to non-MCP tools", () => {
+			const tools = [
+				{
+					type: "function",
+					function: {
+						name: "read_file",
+						description: "Read a file",
+						parameters: {
+							type: "object",
+							properties: {
+								path: { type: "string" },
+							},
+						},
+					},
+				},
+			]
+
+			const result = provider.testConvertToolsForOpenAI(tools)
+
+			expect(result?.[0].function.parameters.additionalProperties).toBe(false)
+			expect(result?.[0].function.parameters.required).toEqual(["path"])
+		})
+
+		it("should not apply schema conversion to MCP tools in base-provider", () => {
+			// Note: In base-provider, MCP tools are passed through unchanged
+			// The openai-native provider has its own handling for MCP tools
+			const tools = [
+				{
+					type: "function",
+					function: {
+						name: "mcp--github--get_me",
+						description: "Get current user",
+						parameters: {
+							type: "object",
+							properties: {
+								token: { type: "string" },
+							},
+							required: ["token"],
+						},
+					},
+				},
+			]
+
+			const result = provider.testConvertToolsForOpenAI(tools)
+
+			// MCP tools pass through original parameters in base-provider
+			expect(result?.[0].function.parameters.additionalProperties).toBeUndefined()
+		})
+
+		it("should preserve non-function tools unchanged", () => {
+			const tools = [
+				{
+					type: "other_type",
+					data: "some data",
+				},
+			]
+
+			const result = provider.testConvertToolsForOpenAI(tools)
+
+			expect(result?.[0]).toEqual(tools[0])
+		})
+	})
+})

+ 219 - 0
src/api/providers/__tests__/openai-native-tools.spec.ts

@@ -1,6 +1,8 @@
 import OpenAI from "openai"
 
 import { OpenAiHandler } from "../openai"
+import { OpenAiNativeHandler } from "../openai-native"
+import type { ApiHandlerOptions } from "../../../shared/api"
 
 describe("OpenAiHandler native tools", () => {
 	it("includes tools in request when custom model info lacks supportsNativeTools (regression test)", async () => {
@@ -75,3 +77,220 @@ describe("OpenAiHandler native tools", () => {
 		)
 	})
 })
+
+describe("OpenAiNativeHandler MCP tool schema handling", () => {
+	it("should add additionalProperties: false to MCP tools while keeping strict: false", async () => {
+		let capturedRequestBody: any
+
+		const handler = new OpenAiNativeHandler({
+			openAiNativeApiKey: "test-key",
+			apiModelId: "gpt-4o",
+		} as ApiHandlerOptions)
+
+		// Mock the responses API call
+		const mockClient = {
+			responses: {
+				create: vi.fn().mockImplementation((body: any) => {
+					capturedRequestBody = body
+					return {
+						[Symbol.asyncIterator]: async function* () {
+							yield {
+								type: "response.done",
+								response: {
+									output: [{ type: "message", content: [{ type: "output_text", text: "test" }] }],
+									usage: { input_tokens: 10, output_tokens: 5 },
+								},
+							}
+						},
+					}
+				}),
+			},
+		}
+		;(handler as any).client = mockClient
+
+		const mcpTools: OpenAI.Chat.ChatCompletionTool[] = [
+			{
+				type: "function",
+				function: {
+					name: "mcp--github--get_me",
+					description: "Get current GitHub user",
+					parameters: {
+						type: "object",
+						properties: {
+							token: { type: "string", description: "API token" },
+						},
+						required: ["token"],
+					},
+				},
+			},
+		]
+
+		const stream = handler.createMessage("system prompt", [], {
+			taskId: "test-task-id",
+			tools: mcpTools,
+			toolProtocol: "native" as const,
+		})
+
+		// Consume the stream
+		for await (const _ of stream) {
+			// Just consume
+		}
+
+		// Verify the request body
+		expect(capturedRequestBody.tools).toBeDefined()
+		expect(capturedRequestBody.tools.length).toBe(1)
+
+		const tool = capturedRequestBody.tools[0]
+		expect(tool.name).toBe("mcp--github--get_me")
+		expect(tool.strict).toBe(false) // MCP tools should have strict: false
+		expect(tool.parameters.additionalProperties).toBe(false) // Should have additionalProperties: false
+		expect(tool.parameters.required).toEqual(["token"]) // Should preserve original required array
+	})
+
+	it("should add additionalProperties: false and required array to non-MCP tools with strict: true", async () => {
+		let capturedRequestBody: any
+
+		const handler = new OpenAiNativeHandler({
+			openAiNativeApiKey: "test-key",
+			apiModelId: "gpt-4o",
+		} as ApiHandlerOptions)
+
+		// Mock the responses API call
+		const mockClient = {
+			responses: {
+				create: vi.fn().mockImplementation((body: any) => {
+					capturedRequestBody = body
+					return {
+						[Symbol.asyncIterator]: async function* () {
+							yield {
+								type: "response.done",
+								response: {
+									output: [{ type: "message", content: [{ type: "output_text", text: "test" }] }],
+									usage: { input_tokens: 10, output_tokens: 5 },
+								},
+							}
+						},
+					}
+				}),
+			},
+		}
+		;(handler as any).client = mockClient
+
+		const regularTools: OpenAI.Chat.ChatCompletionTool[] = [
+			{
+				type: "function",
+				function: {
+					name: "read_file",
+					description: "Read a file from the filesystem",
+					parameters: {
+						type: "object",
+						properties: {
+							path: { type: "string", description: "File path" },
+							encoding: { type: "string", description: "File encoding" },
+						},
+					},
+				},
+			},
+		]
+
+		const stream = handler.createMessage("system prompt", [], {
+			taskId: "test-task-id",
+			tools: regularTools,
+			toolProtocol: "native" as const,
+		})
+
+		// Consume the stream
+		for await (const _ of stream) {
+			// Just consume
+		}
+
+		// Verify the request body
+		expect(capturedRequestBody.tools).toBeDefined()
+		expect(capturedRequestBody.tools.length).toBe(1)
+
+		const tool = capturedRequestBody.tools[0]
+		expect(tool.name).toBe("read_file")
+		expect(tool.strict).toBe(true) // Non-MCP tools should have strict: true
+		expect(tool.parameters.additionalProperties).toBe(false) // Should have additionalProperties: false
+		expect(tool.parameters.required).toEqual(["path", "encoding"]) // Should have all properties as required
+	})
+
+	it("should recursively add additionalProperties: false to nested objects in MCP tools", async () => {
+		let capturedRequestBody: any
+
+		const handler = new OpenAiNativeHandler({
+			openAiNativeApiKey: "test-key",
+			apiModelId: "gpt-4o",
+		} as ApiHandlerOptions)
+
+		// Mock the responses API call
+		const mockClient = {
+			responses: {
+				create: vi.fn().mockImplementation((body: any) => {
+					capturedRequestBody = body
+					return {
+						[Symbol.asyncIterator]: async function* () {
+							yield {
+								type: "response.done",
+								response: {
+									output: [{ type: "message", content: [{ type: "output_text", text: "test" }] }],
+									usage: { input_tokens: 10, output_tokens: 5 },
+								},
+							}
+						},
+					}
+				}),
+			},
+		}
+		;(handler as any).client = mockClient
+
+		const mcpToolsWithNestedObjects: OpenAI.Chat.ChatCompletionTool[] = [
+			{
+				type: "function",
+				function: {
+					name: "mcp--linear--create_issue",
+					description: "Create a Linear issue",
+					parameters: {
+						type: "object",
+						properties: {
+							title: { type: "string" },
+							metadata: {
+								type: "object",
+								properties: {
+									priority: { type: "number" },
+									labels: {
+										type: "array",
+										items: {
+											type: "object",
+											properties: {
+												name: { type: "string" },
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		]
+
+		const stream = handler.createMessage("system prompt", [], {
+			taskId: "test-task-id",
+			tools: mcpToolsWithNestedObjects,
+			toolProtocol: "native" as const,
+		})
+
+		// Consume the stream
+		for await (const _ of stream) {
+			// Just consume
+		}
+
+		// Verify the request body
+		const tool = capturedRequestBody.tools[0]
+		expect(tool.strict).toBe(false) // MCP tool should have strict: false
+		expect(tool.parameters.additionalProperties).toBe(false) // Root level
+		expect(tool.parameters.properties.metadata.additionalProperties).toBe(false) // Nested object
+		expect(tool.parameters.properties.metadata.properties.labels.items.additionalProperties).toBe(false) // Array items
+	})
+})

+ 7 - 0
src/api/providers/base-provider.ts

@@ -55,6 +55,7 @@ export abstract class BaseProvider implements ApiHandler {
 	 * Converts tool schemas to be compatible with OpenAI's strict mode by:
 	 * - Ensuring all properties are in the required array (strict mode requirement)
 	 * - Converting nullable types (["type", "null"]) to non-nullable ("type")
+	 * - Adding additionalProperties: false to all object schemas (required by OpenAI Responses API)
 	 * - Recursively processing nested objects and arrays
 	 *
 	 * This matches the behavior of ensureAllRequired in openai-native.ts
@@ -66,6 +67,12 @@ export abstract class BaseProvider implements ApiHandler {
 
 		const result = { ...schema }
 
+		// OpenAI Responses API requires additionalProperties: false on all object schemas
+		// Only add if not already set to false (to avoid unnecessary mutations)
+		if (result.additionalProperties !== false) {
+			result.additionalProperties = false
+		}
+
 		if (result.properties) {
 			const allKeys = Object.keys(result.properties)
 			// OpenAI strict mode requires ALL properties to be in required array

+ 46 - 1
src/api/providers/openai-native.ts

@@ -196,6 +196,12 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
 
 			const result = { ...schema }
 
+			// OpenAI Responses API requires additionalProperties: false on all object schemas
+			// Only add if not already set to false (to avoid unnecessary mutations)
+			if (result.additionalProperties !== false) {
+				result.additionalProperties = false
+			}
+
 			if (result.properties) {
 				const allKeys = Object.keys(result.properties)
 				result.required = allKeys
@@ -219,6 +225,42 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
 			return result
 		}
 
+		// Adds additionalProperties: false to all object schemas recursively
+		// without modifying required array. Used for MCP tools with strict: false
+		// to comply with OpenAI Responses API requirements.
+		const ensureAdditionalPropertiesFalse = (schema: any): any => {
+			if (!schema || typeof schema !== "object" || schema.type !== "object") {
+				return schema
+			}
+
+			const result = { ...schema }
+
+			// OpenAI Responses API requires additionalProperties: false on all object schemas
+			// Only add if not already set to false (to avoid unnecessary mutations)
+			if (result.additionalProperties !== false) {
+				result.additionalProperties = false
+			}
+
+			if (result.properties) {
+				// Recursively process nested objects
+				const newProps = { ...result.properties }
+				for (const key of Object.keys(result.properties)) {
+					const prop = newProps[key]
+					if (prop && prop.type === "object") {
+						newProps[key] = ensureAdditionalPropertiesFalse(prop)
+					} else if (prop && prop.type === "array" && prop.items?.type === "object") {
+						newProps[key] = {
+							...prop,
+							items: ensureAdditionalPropertiesFalse(prop.items),
+						}
+					}
+				}
+				result.properties = newProps
+			}
+
+			return result
+		}
+
 		// Build a request body for the OpenAI Responses API.
 		// Ensure we explicitly pass max_output_tokens based on Roo's reserved model response calculation
 		// so requests do not default to very large limits (e.g., 120k).
@@ -295,12 +337,15 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
 					.map((tool) => {
 						// MCP tools use the 'mcp--' prefix - disable strict mode for them
 						// to preserve optional parameters from the MCP server schema
+						// But we still need to add additionalProperties: false for OpenAI Responses API
 						const isMcp = isMcpTool(tool.function.name)
 						return {
 							type: "function",
 							name: tool.function.name,
 							description: tool.function.description,
-							parameters: isMcp ? tool.function.parameters : ensureAllRequired(tool.function.parameters),
+							parameters: isMcp
+								? ensureAdditionalPropertiesFalse(tool.function.parameters)
+								: ensureAllRequired(tool.function.parameters),
 							strict: !isMcp,
 						}
 					}),