From ccaa193eec5761f0972ffb58eb3189a81a9244b0 Mon Sep 17 00:00:00 2001 From: Juan Camilo Auriti Date: Wed, 8 Apr 2026 10:42:11 +0200 Subject: [PATCH] fix: preserve only originally-required properties in strict tool schemas (#471) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #430. In normalizeSchemaForOpenAI(), the strict branch was adding every property key to required[], including optional ones. This caused providers like Groq, Azure OpenAI, and others to reject valid tool calls with a 400 / tool_use_failed error because the model correctly omits optional arguments but the provider sees them as missing required fields. Root cause: the strict branch used `[...existingRequired, ...allKeys]` instead of `existingRequired.filter(k => k in normalizedProps)`. The Gemini branch already had the correct logic. Fix: align the strict branch with the Gemini branch — only keep properties that were already marked required in the original schema. The additionalProperties: false constraint is preserved as strict-mode providers still require it. Add regression test covering the Read tool schema (file_path required, offset/limit/pages optional). --- src/services/api/openaiShim.test.ts | 69 +++++++++++++++++++++++++---- src/services/api/openaiShim.ts | 12 ++--- 2 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/services/api/openaiShim.test.ts b/src/services/api/openaiShim.test.ts index cc553d21..8eab605d 100644 --- a/src/services/api/openaiShim.test.ts +++ b/src/services/api/openaiShim.test.ts @@ -1806,12 +1806,70 @@ test('sanitizes malformed MCP tool schemas before sending them to OpenAI', async | undefined expect(parameters?.additionalProperties).toBe(false) - expect(parameters?.required).toEqual(['priority']) + // No required[] in the original schema → none added (optional properties must not be forced required) + expect(parameters?.required).toEqual([]) expect(properties?.priority?.type).toBe('integer') expect(properties?.priority?.enum).toEqual([0, 1, 2, 3]) expect(properties?.priority).not.toHaveProperty('default') }) +test('optional tool properties are not added to required[] — fixes Groq/Azure 400 tool_use_failed', async () => { + // Regression test for: all optional properties being sent as required in strict mode, + // causing providers like Groq to reject valid tool calls where the model omits optional args. + let requestBody: Record | undefined + + globalThis.fetch = (async (_input, init) => { + requestBody = JSON.parse(String(init?.body)) + + return new Response( + JSON.stringify({ + id: 'chatcmpl-4', + model: 'gpt-4o', + choices: [{ message: { role: 'assistant', content: 'ok' }, finish_reason: 'stop' }], + usage: { prompt_tokens: 5, completion_tokens: 2, total_tokens: 7 }, + }), + { headers: { 'Content-Type': 'application/json' } }, + ) + }) as FetchType + + const client = createOpenAIShimClient({}) as OpenAIShimClient + + await client.beta.messages.create({ + model: 'gpt-4o', + messages: [{ role: 'user', content: 'read a file' }], + tools: [ + { + name: 'Read', + description: 'Read a file', + input_schema: { + type: 'object', + properties: { + file_path: { type: 'string', description: 'Absolute path to file' }, + offset: { type: 'number', description: 'Line to start from' }, + limit: { type: 'number', description: 'Max lines to read' }, + pages: { type: 'string', description: 'Page range for PDFs' }, + }, + required: ['file_path'], + }, + }, + ], + max_tokens: 16, + stream: false, + }) + + const parameters = ( + requestBody?.tools as Array<{ function?: { parameters?: Record } }> + )?.[0]?.function?.parameters + + expect(parameters?.required).toEqual(['file_path']) + + const required = parameters?.required as string[] | undefined + expect(required).not.toContain('offset') + expect(required).not.toContain('limit') + expect(required).not.toContain('pages') + expect(parameters?.additionalProperties).toBe(false) +}) + // --------------------------------------------------------------------------- // Issue #202 — consecutive role coalescing (Devstral, Mistral strict templates) // --------------------------------------------------------------------------- @@ -1849,7 +1907,7 @@ test('coalesces consecutive user messages to avoid alternation errors (issue #20 stream: false, }) - expect(sentMessages?.length).toBe(2) // system + 1 merged user + expect(sentMessages?.length).toBe(2) expect(sentMessages?.[0]?.role).toBe('system') expect(sentMessages?.[1]?.role).toBe('user') const userContent = sentMessages?.[1]?.content as string @@ -1883,9 +1941,8 @@ test('coalesces consecutive assistant messages preserving tool_calls (issue #202 stream: false, }) - // system + user + merged assistant + tool const assistantMsgs = sentMessages?.filter(m => m.role === 'assistant') - expect(assistantMsgs?.length).toBe(1) // two assistant turns merged into one + expect(assistantMsgs?.length).toBe(1) expect(assistantMsgs?.[0]?.tool_calls?.length).toBeGreaterThan(0) }) @@ -1975,8 +2032,6 @@ test('non-streaming: empty string content does not fall through to reasoning_con stream: false, })) as { content: Array> } - // reasoning_content should be a thinking block, and also used as text - // since content is empty string (treated as absent) expect(result.content).toEqual([ { type: 'thinking', thinking: 'Chain of thought here.' }, { type: 'text', text: 'Chain of thought here.' }, @@ -2104,7 +2159,6 @@ test('streaming: thinking block closed before tool call', async () => { const types = events.map(e => e.type) - // Verify thinking block is started, then closed, then tool call starts const thinkingStartIdx = types.indexOf('content_block_start') const firstStopIdx = types.indexOf('content_block_stop') const toolStartIdx = types.indexOf( @@ -2116,7 +2170,6 @@ test('streaming: thinking block closed before tool call', async () => { expect(firstStopIdx).toBeGreaterThan(thinkingStartIdx) expect(toolStartIdx).toBeGreaterThan(firstStopIdx) - // Verify thinking block start content const thinkingStart = events[thinkingStartIdx] as { content_block?: Record } diff --git a/src/services/api/openaiShim.ts b/src/services/api/openaiShim.ts index dfe8491f..32c85287 100644 --- a/src/services/api/openaiShim.ts +++ b/src/services/api/openaiShim.ts @@ -421,11 +421,13 @@ function normalizeSchemaForOpenAI( record.properties = normalizedProps if (strict) { - // OpenAI strict mode requires every property to be listed in required[] - const allKeys = Object.keys(normalizedProps) - record.required = Array.from(new Set([...existingRequired, ...allKeys])) - // OpenAI strict mode requires additionalProperties: false on all object - // schemas — override unconditionally to ensure nested objects comply. + // Keep only the properties that were originally marked required in the schema. + // Adding every property to required[] (the previous behaviour) caused strict + // OpenAI-compatible providers (Groq, Azure, etc.) to reject tool calls because + // the model correctly omits optional arguments — but the provider treats them + // as missing required fields and returns a 400 / tool_use_failed error. + record.required = existingRequired.filter(k => k in normalizedProps) + // additionalProperties: false is still required by strict-mode providers. record.additionalProperties = false } else { // For Gemini: keep only existing required keys that are present in properties