From d7a89cee299cba7fc75900bcf0e4d3f140b6a9c9 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 5 Jan 2026 18:31:39 -0500 Subject: [PATCH] fix: add additionalProperties: false to MCP tool schemas for OpenAI Responses API OpenAI's Responses API requires additionalProperties: false on all object schemas, even for non-strict tools (MCP tools). This adds defensive schema conversion to ensure compliance regardless of whether the MCP server includes this property. Changes: - base-provider.ts: Add additionalProperties: false in convertToolSchemaForOpenAI() - openai-native.ts: Add ensureAdditionalPropertiesFalse() for MCP tools - Only add the property if not already set to false (avoids unnecessary mutations) - New tests for additionalProperties handling in both providers Fixes ROO-374 --- .../providers/__tests__/base-provider.spec.ts | 283 ++++++++++++++++++ .../__tests__/openai-native-tools.spec.ts | 219 ++++++++++++++ src/api/providers/base-provider.ts | 7 + src/api/providers/openai-native.ts | 47 ++- 4 files changed, 555 insertions(+), 1 deletion(-) create mode 100644 src/api/providers/__tests__/base-provider.spec.ts diff --git a/src/api/providers/__tests__/base-provider.spec.ts b/src/api/providers/__tests__/base-provider.spec.ts new file mode 100644 index 00000000000..ced452f5a55 --- /dev/null +++ b/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]) + }) + }) +}) diff --git a/src/api/providers/__tests__/openai-native-tools.spec.ts b/src/api/providers/__tests__/openai-native-tools.spec.ts index 1a3b93b9c21..7be4814b633 100644 --- a/src/api/providers/__tests__/openai-native-tools.spec.ts +++ b/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 + }) +}) diff --git a/src/api/providers/base-provider.ts b/src/api/providers/base-provider.ts index 64d99b3f0c1..a6adeeadbd4 100644 --- a/src/api/providers/base-provider.ts +++ b/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 diff --git a/src/api/providers/openai-native.ts b/src/api/providers/openai-native.ts index 8f9cc2297f8..58a62497f71 100644 --- a/src/api/providers/openai-native.ts +++ b/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, } }),