diff --git a/src/server/llm/client-pure.test.ts b/src/server/llm/client-pure.test.ts index 8727ef38..53483080 100644 --- a/src/server/llm/client-pure.test.ts +++ b/src/server/llm/client-pure.test.ts @@ -377,12 +377,38 @@ describe('llm client pure helpers', () => { expect(withoutFlag).toEqual([{ role: 'user', content: 'hi' }]) }) - it('converts tool definitions to openai function schema', () => { + it('converts tool definitions to openai function schema and sanitizes invalid schema fields', () => { expect( convertTools([ - { type: 'function', function: { name: 'grep', description: 'Search', parameters: { type: 'object' } } }, + { + type: 'function', + function: { + name: 'grep', + description: 'Search', + parameters: { + type: 'object', + properties: { + tags: { type: 'array', items: {} }, + }, + }, + }, + }, ]), - ).toEqual([{ type: 'function', function: { name: 'grep', description: 'Search', parameters: { type: 'object' } } }]) + ).toEqual([ + { + type: 'function', + function: { + name: 'grep', + description: 'Search', + parameters: { + type: 'object', + properties: { + tags: { type: 'array', items: { type: 'string' } }, + }, + }, + }, + }, + ]) }) it('maps finish reasons', () => { @@ -588,7 +614,10 @@ describe('llm client pure helpers', () => { model: 'test-model', messages: [{ role: 'user', content: 'hello' }], tools: [ - { type: 'function', function: { name: 'glob', description: 'Search', parameters: { type: 'object' } } }, + { + type: 'function', + function: { name: 'glob', description: 'Search', parameters: { type: 'object', properties: {} } }, + }, ], tool_choice: 'auto', temperature: 0.2, @@ -629,7 +658,10 @@ describe('llm client pure helpers', () => { model: 'test-model', messages: [{ role: 'user', content: 'hello' }], tools: [ - { type: 'function', function: { name: 'glob', description: 'Search', parameters: { type: 'object' } } }, + { + type: 'function', + function: { name: 'glob', description: 'Search', parameters: { type: 'object', properties: {} } }, + }, ], tool_choice: 'auto', temperature: 0.2, @@ -670,7 +702,10 @@ describe('llm client pure helpers', () => { model: 'test-model', messages: [{ role: 'user', content: 'hello' }], tools: [ - { type: 'function', function: { name: 'glob', description: 'Search', parameters: { type: 'object' } } }, + { + type: 'function', + function: { name: 'glob', description: 'Search', parameters: { type: 'object', properties: {} } }, + }, ], tool_choice: 'auto', temperature: 0.2, @@ -711,7 +746,10 @@ describe('llm client pure helpers', () => { model: 'test-model', messages: [{ role: 'user', content: 'hello' }], tools: [ - { type: 'function', function: { name: 'glob', description: 'Search', parameters: { type: 'object' } } }, + { + type: 'function', + function: { name: 'glob', description: 'Search', parameters: { type: 'object', properties: {} } }, + }, ], tool_choice: 'auto', temperature: 0.2, @@ -753,7 +791,10 @@ describe('llm client pure helpers', () => { model: 'test-model', messages: [{ role: 'user', content: 'hello' }], tools: [ - { type: 'function', function: { name: 'glob', description: 'Search', parameters: { type: 'object' } } }, + { + type: 'function', + function: { name: 'glob', description: 'Search', parameters: { type: 'object', properties: {} } }, + }, ], tool_choice: 'auto', temperature: 0.2, diff --git a/src/server/llm/client-pure.ts b/src/server/llm/client-pure.ts index 8e7a6637..403d856c 100644 --- a/src/server/llm/client-pure.ts +++ b/src/server/llm/client-pure.ts @@ -22,6 +22,7 @@ import { extractPdfBlocksFromDataUrl, formatVisionFallbackDescription, } from './resolve-attachments.js' +import { sanitizeToolSchema } from './schema-sanitizer.js' import type { ContentPart } from './resolve-attachments.js' export { resolveAttachmentsInMessages } from './resolve-attachments.js' @@ -255,7 +256,7 @@ export function convertTools(tools: LLMToolDefinition[]): ChatCompletionTool[] { function: { name: tool.function.name, description: tool.function.description, - parameters: tool.function.parameters, + parameters: sanitizeToolSchema(tool.function.parameters), }, })) } diff --git a/src/server/llm/schema-sanitizer.test.ts b/src/server/llm/schema-sanitizer.test.ts new file mode 100644 index 00000000..f9cb91c7 --- /dev/null +++ b/src/server/llm/schema-sanitizer.test.ts @@ -0,0 +1,199 @@ +import { describe, it, expect } from 'vitest' +import { sanitizeToolSchema } from './schema-sanitizer.js' + +describe('sanitizeToolSchema', () => { + it('returns valid default schema for non-object inputs', () => { + expect(sanitizeToolSchema(null)).toEqual({ type: 'object', properties: {} }) + expect(sanitizeToolSchema(undefined)).toEqual({ type: 'object', properties: {} }) + expect(sanitizeToolSchema('string')).toEqual({ type: 'object', properties: {} }) + expect(sanitizeToolSchema([])).toEqual({ type: 'object', properties: {} }) + }) + + it('strips unsupported JSON schema keywords and additionalProperties', () => { + const input = { + $schema: 'http://json-schema.org/draft-07/schema#', + $id: 'http://example.com/schema.json', + type: 'object', + additionalProperties: false, + properties: { + name: { type: 'string', patternProperties: {} }, + }, + } + expect(sanitizeToolSchema(input)).toEqual({ + type: 'object', + properties: { + name: { type: 'string' }, + }, + }) + }) + + it('strips null and undefined values such as default: null', () => { + const input = { + type: 'object', + properties: { + expand: { type: 'string', default: null }, + include: { type: 'string', default: null }, + count: { type: 'number', default: 10 }, + }, + } + expect(sanitizeToolSchema(input)).toEqual({ + type: 'object', + properties: { + expand: { type: 'string' }, + include: { type: 'string' }, + count: { type: 'number', default: 10 }, + }, + }) + }) + + it('converts const to enum', () => { + const input = { + type: 'object', + properties: { + mode: { const: 'exact' }, + }, + } + expect(sanitizeToolSchema(input)).toEqual({ + type: 'object', + properties: { + mode: { enum: ['exact'] }, + }, + }) + }) + + it('normalizes empty or missing array items to { type: "string" }', () => { + const input = { + type: 'object', + properties: { + tags: { type: 'array', items: {} }, + labels: { type: 'array' }, + }, + } + expect(sanitizeToolSchema(input)).toEqual({ + type: 'object', + properties: { + tags: { type: 'array', items: { type: 'string' } }, + labels: { type: 'array', items: { type: 'string' } }, + }, + }) + }) + + it('ensures type: object always has a properties map', () => { + const input = { + type: 'object', + properties: { + env: { type: 'object' }, + }, + } + expect(sanitizeToolSchema(input)).toEqual({ + type: 'object', + properties: { + env: { type: 'object', properties: {} }, + }, + }) + }) + + it('normalizes string property values in properties map', () => { + const input = { + type: 'object', + properties: { + env: 'object', + name: 'string', + }, + } + expect(sanitizeToolSchema(input)).toEqual({ + type: 'object', + properties: { + env: { type: 'object', properties: {} }, + name: { type: 'string' }, + }, + }) + }) + + it('sanitizes real Jira MCP tool schema containing additionalProperties and null defaults', () => { + const jiraSchema = { + type: 'object', + properties: { + issue_key: { + description: "Jira issue key (e.g., 'PROJ-123')", + type: 'string', + }, + expand: { + default: null, + description: '(Optional) Fields to expand', + type: 'string', + }, + properties: { + description: '(Optional) A comma-separated list of issue properties to return', + default: null, + type: 'string', + }, + comment_limit: { + default: 10, + maximum: 100, + minimum: 0, + type: 'integer', + }, + }, + required: ['issue_key'], + additionalProperties: false, + } + + const sanitized = sanitizeToolSchema(jiraSchema) + + expect(sanitized).toEqual({ + type: 'object', + properties: { + issue_key: { + description: "Jira issue key (e.g., 'PROJ-123')", + type: 'string', + }, + expand: { + description: '(Optional) Fields to expand', + type: 'string', + }, + props: { + description: '(Optional) A comma-separated list of issue properties to return', + type: 'string', + }, + comment_limit: { + default: 10, + maximum: 100, + minimum: 0, + type: 'integer', + }, + }, + required: ['issue_key'], + }) + expect(sanitized).not.toHaveProperty('additionalProperties') + }) + + it('recursively sanitizes nested properties and oneOf/anyOf branches', () => { + const input = { + type: 'object', + properties: { + config: { + type: 'object', + properties: { + subOptions: { + oneOf: [{ const: 'auto' }, { type: 'array', items: {} }], + }, + }, + }, + }, + } + expect(sanitizeToolSchema(input)).toEqual({ + type: 'object', + properties: { + config: { + type: 'object', + properties: { + subOptions: { + oneOf: [{ enum: ['auto'] }, { type: 'array', items: { type: 'string' } }], + }, + }, + }, + }, + }) + }) +}) diff --git a/src/server/llm/schema-sanitizer.ts b/src/server/llm/schema-sanitizer.ts new file mode 100644 index 00000000..e457f3d7 --- /dev/null +++ b/src/server/llm/schema-sanitizer.ts @@ -0,0 +1,117 @@ +/** + * Schema Sanitizer + * + * Sanitizes JSON Schema tool parameters for broad LLM provider compatibility + * (OpenAI, Gemini/Vertex AI/Antigravity, Anthropic, Ollama, etc.). + * + * Normalizes invalid or unsupported constructs: + * - Strips `additionalProperties` (unsupported by Vertex AI / Antigravity protobuf Schema) + * - Strips `null` values (e.g. `default: null`) + * - Ensures `type: 'object'` always defines a valid `properties` map + * - Empty or missing `items` in array schemas (`items: {}` -> `items: { type: 'string' }`) + * - Strips meta keywords ($schema, $id, $vocabulary, etc.) + * - Converts `const` values to `enum: [val]` + * - Recursively processes properties, array items, and union branches (anyOf, oneOf, allOf) + */ + +export function sanitizeToolSchema(schema: unknown): Record { + if (!schema || typeof schema !== 'object' || Array.isArray(schema)) { + return { type: 'object', properties: {} } + } + + return cleanSchemaNode(schema as Record) +} + +function cleanSchemaNode(node: Record): Record { + const result: Record = {} + + for (const [key, val] of Object.entries(node)) { + // Strip null / undefined values (e.g. default: null) + if (val === null || val === undefined) { + continue + } + + // Strip meta keywords & additionalProperties unsupported across providers + if ( + key === '$schema' || + key === '$id' || + key === '$vocabulary' || + key === '$anchor' || + key === 'dependentRequired' || + key === 'dependentSchemas' || + key === 'unevaluatedProperties' || + key === 'unevaluatedItems' || + key === 'patternProperties' || + key === 'additionalProperties' + ) { + continue + } + + if (key === 'required' && Array.isArray(val)) { + result['required'] = val.map((item) => (item === 'properties' ? 'props' : item)) + continue + } + + if (key === 'const') { + result['enum'] = [val] + continue + } + + if (key === 'properties' && val && typeof val === 'object' && !Array.isArray(val)) { + const sanitizedProps: Record = {} + for (const [propKey, propVal] of Object.entries(val as Record)) { + // A property named 'properties' inside a properties map creates ambiguous/conflicting + // Protobuf schemas for Gemini / Vertex AI (e.g. parameters.properties.properties). + // Safely rename it to 'props'. + const targetKey = propKey === 'properties' ? 'props' : propKey + + if (typeof propVal === 'string') { + sanitizedProps[targetKey] = propVal === 'object' ? { type: 'object', properties: {} } : { type: propVal } + } else if (propVal && typeof propVal === 'object' && !Array.isArray(propVal)) { + sanitizedProps[targetKey] = cleanSchemaNode(propVal as Record) + } else { + sanitizedProps[targetKey] = propVal + } + } + result['properties'] = sanitizedProps + continue + } + + if (key === 'items') { + if (!val || typeof val !== 'object' || Object.keys(val).length === 0) { + result['items'] = { type: 'string' } + } else if (Array.isArray(val)) { + result['items'] = val.map((item) => + item && typeof item === 'object' ? cleanSchemaNode(item as Record) : item, + ) + } else { + result['items'] = cleanSchemaNode(val as Record) + } + continue + } + + if ((key === 'anyOf' || key === 'oneOf' || key === 'allOf') && Array.isArray(val)) { + result[key] = val.map((item) => + item && typeof item === 'object' ? cleanSchemaNode(item as Record) : item, + ) + continue + } + + result[key] = val + } + + // If type is object but properties is missing, provide empty properties map + if (result['type'] === 'object' && !result['properties']) { + result['properties'] = {} + } + + // If type is array but items is missing or empty, supply a default string item schema + if ( + result['type'] === 'array' && + (!result['items'] || (typeof result['items'] === 'object' && Object.keys(result['items'] as object).length === 0)) + ) { + result['items'] = { type: 'string' } + } + + return result +} diff --git a/src/server/mcp/manager.test.ts b/src/server/mcp/manager.test.ts index 153a1b3f..1e05db7f 100644 --- a/src/server/mcp/manager.test.ts +++ b/src/server/mcp/manager.test.ts @@ -519,6 +519,41 @@ describe('createMcpTools', () => { expect(result.success).toBe(true) expect(result.output).toBe('Sunny, 72°F') }) + + it('remaps a renamed props argument back to properties on execution', async () => { + mockClientInstance.listTools.mockResolvedValueOnce({ + tools: [ + { + name: 'config_tool', + description: 'Config tool', + inputSchema: { + type: 'object', + properties: { + properties: { type: 'object', properties: { a: { type: 'string' } } }, + }, + }, + }, + ], + }) + const manager = new McpManager() + await manager.addServer('test', { transport: 'stdio', command: 'node' }) + + const tools = createMcpTools(manager) + // The sanitizer renames the top-level `properties` param to `props` in the + // LLM-facing schema, so the model answers with `props`. + expect(tools[0]!.definition.function.parameters).toEqual({ + type: 'object', + properties: { props: { type: 'object', properties: { a: { type: 'string' } } } }, + }) + + await tools[0]!.execute({ props: { a: 'x' } }, {} as any) + + // The MCP server must receive the original param name, with no stray `props` key. + const lastCall = mockClientInstance.callTool.mock.calls.at(-1)! + const payload = lastCall[lastCall.length - 1] as { arguments?: Record } + expect(payload.arguments).toEqual({ properties: { a: 'x' } }) + expect(payload.arguments).not.toHaveProperty('props') + }) }) describe('estimateToolTokens', () => { @@ -617,4 +652,25 @@ describe('McpManager token estimation', () => { expect(betaRe.status).toBe('connected') expect(betaRe.tools.length).toBe(2) }) + + it('creates MCP tools with sanitized parameters schema', async () => { + mockClientInstance.listTools.mockResolvedValueOnce({ + tools: [ + { + name: 'get_weather', + description: 'Get weather', + inputSchema: { type: 'object', properties: { location: { type: 'string' } } }, + }, + ], + }) + const manager = new McpManager() + await manager.addServer('test', { transport: 'stdio', command: 'node' }) + const tools = createMcpTools(manager) + expect(tools).toHaveLength(1) + expect(tools[0]!.name).toBe('test_get_weather') + expect(tools[0]!.definition.function.parameters).toEqual({ + type: 'object', + properties: { location: { type: 'string' } }, + }) + }) }) diff --git a/src/server/mcp/manager.ts b/src/server/mcp/manager.ts index 67a88704..c56117a1 100644 --- a/src/server/mcp/manager.ts +++ b/src/server/mcp/manager.ts @@ -8,6 +8,7 @@ import type { LLMToolDefinition } from '../llm/types.js' import { logger } from '../utils/logger.js' import { McpOAuthProvider } from './oauth-provider.js' import { readMcpOAuthEntry } from './oauth-store.js' +import { sanitizeToolSchema } from '../llm/schema-sanitizer.js' /** * The SDK merges requestInit headers after the ones it derives from the auth provider, so a static @@ -243,7 +244,7 @@ export class McpManager { function: { name: `${entry.state.name}_${tool.name}`, description: tool.description ?? '', - parameters: tool.inputSchema as Record, + parameters: sanitizeToolSchema(tool.inputSchema as Record), }, }) } diff --git a/src/server/mcp/tool-adapter.ts b/src/server/mcp/tool-adapter.ts index 5b724d21..3ca32bed 100644 --- a/src/server/mcp/tool-adapter.ts +++ b/src/server/mcp/tool-adapter.ts @@ -2,6 +2,7 @@ import type { Tool, ToolContext } from '../tools/types.js' import type { LLMToolDefinition } from '../llm/types.js' import type { McpManager } from './manager.js' import type { ToolResult } from '../../shared/types.js' +import { sanitizeToolSchema } from '../llm/schema-sanitizer.js' export function createMcpTools(mcpManager: McpManager): Tool[] { const tools: Tool[] = [] @@ -17,7 +18,7 @@ export function createMcpTools(mcpManager: McpManager): Tool[] { function: { name: prefixedName, description: mcpTool.description ?? '', - parameters: mcpTool.inputSchema as Record, + parameters: sanitizeToolSchema(mcpTool.inputSchema as Record), }, } @@ -27,7 +28,12 @@ export function createMcpTools(mcpManager: McpManager): Tool[] { mcpServer: server.name, execute: async (args: Record, _context: ToolContext): Promise => { const start = Date.now() - const result = await mcpManager.callTool(server.name, mcpTool.name, args) + const normalizedArgs = { ...args } + if ('props' in normalizedArgs && !('properties' in normalizedArgs)) { + normalizedArgs['properties'] = normalizedArgs['props'] + delete normalizedArgs['props'] + } + const result = await mcpManager.callTool(server.name, mcpTool.name, normalizedArgs) return { success: result.success, ...(result.output ? { output: result.output } : {}), diff --git a/src/server/providers/adapters/transport-client.test.ts b/src/server/providers/adapters/transport-client.test.ts index a4c58a44..ca338d8f 100644 --- a/src/server/providers/adapters/transport-client.test.ts +++ b/src/server/providers/adapters/transport-client.test.ts @@ -368,6 +368,67 @@ describe('createTransportLLMClient', () => { expect(client.getReasoningEffort?.()).toBe('deep') }) + it('sanitizes tool schemas before passing to complete() and stream()', async () => { + const provider: Provider = { + id: 'copilot', + name: 'GitHub Copilot', + url: 'https://api.githubcopilot.com', + backend: 'openai', + models: [{ id: 'gpt-4o', contextWindow: 128000, source: 'backend' }], + isActive: true, + createdAt: new Date().toISOString(), + } + + let capturedTools: unknown[] = [] + const mockComplete = vi.fn(async (request: { tools?: unknown[] }) => { + capturedTools = request.tools ?? [] + return { + id: 'r1', + content: '', + toolCalls: [], + finishReason: 'stop' as const, + usage: { promptTokens: 0, completionTokens: 0, totalTokens: 0 }, + } + }) + const client = createTransportLLMClient(provider, 'gpt-4o', { ...transport, complete: mockComplete }) + + await client.complete({ + messages: [{ role: 'user', content: 'test' }], + tools: [ + { + type: 'function', + function: { + name: 'test_tool', + description: 'A test tool', + parameters: { + type: 'object', + additionalProperties: false, + properties: { + opt: { type: 'string', default: null }, + }, + }, + }, + }, + ], + }) + + expect(capturedTools).toEqual([ + { + type: 'function', + function: { + name: 'test_tool', + description: 'A test tool', + parameters: { + type: 'object', + properties: { + opt: { type: 'string' }, + }, + }, + }, + }, + ]) + }) + it('passes an in-list client effort through unchanged', () => { const provider: Provider = { id: 'openai', diff --git a/src/server/providers/adapters/transport-client.ts b/src/server/providers/adapters/transport-client.ts index 22feb442..42501e30 100644 --- a/src/server/providers/adapters/transport-client.ts +++ b/src/server/providers/adapters/transport-client.ts @@ -4,6 +4,7 @@ import { getModelProfile } from '../../llm/profiles.js' import type { LLMClientWithModel } from '../../llm/client.js' import type { ProviderTransportAdapter } from '../../../provider/index.js' import { resolveAttachmentsInMessages } from '../../llm/client-pure.js' +import { sanitizeToolSchema } from '../../llm/schema-sanitizer.js' import { resolveEffortForModel, resolveModeModelId } from '../../../shared/reasoning-effort.js' export function createTransportLLMClient( @@ -80,6 +81,17 @@ export function createTransportLLMClient( request: { ...request, messages: await resolveAttachmentsInMessages(request.messages, supportsVision), + ...(request.tools + ? { + tools: request.tools.map((t) => ({ + ...t, + function: { + ...t.function, + parameters: sanitizeToolSchema(t.function.parameters), + }, + })), + } + : {}), ...(resolvedEffort && !send.suppressEffort ? { reasoningEffort: resolvedEffort } : {}), }, effort: resolvedEffort, diff --git a/src/server/tools/ask.ts b/src/server/tools/ask.ts index 3e78737f..f61bad90 100644 --- a/src/server/tools/ask.ts +++ b/src/server/tools/ask.ts @@ -43,18 +43,13 @@ export const askUserTool: Tool = { description: 'Options for choice-type questions. Each entry may be a plain string or an object {value, label, description?} (or legacy {label, description?}). The server normalizes everything to {value, label, description?}.', items: { - oneOf: [ - { type: 'string' }, - { - type: 'object', - properties: { - value: { type: 'string' }, - label: { type: 'string' }, - description: { type: 'string' }, - }, - required: ['label'], - }, - ], + type: 'object', + properties: { + value: { type: 'string' }, + label: { type: 'string' }, + description: { type: 'string' }, + }, + required: ['label'], }, }, }, diff --git a/src/server/tools/project-tasks.ts b/src/server/tools/project-tasks.ts index 9a87d701..4de93bf8 100644 --- a/src/server/tools/project-tasks.ts +++ b/src/server/tools/project-tasks.ts @@ -94,7 +94,15 @@ export const projectTasksTool = createTool( attachments: { type: 'array', description: 'Optional attachments (same shape as chat attachments)', - items: {}, + items: { + type: 'object', + properties: { + id: { type: 'string', description: 'Attachment ID' }, + filename: { type: 'string', description: 'File name' }, + mimeType: { type: 'string', description: 'MIME type' }, + size: { type: 'number', description: 'File size in bytes' }, + }, + }, }, agentId: { type: 'string', description: 'Selected agent id' }, providerId: { type: 'string', description: 'Provider id used when a session is spawned' },