-
Notifications
You must be signed in to change notification settings - Fork 10
fix(ai-proxy): validate model tool support at Router init (fail fast) #1452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6862d59
9f06d04
e4ab3a1
80301dd
ae0354d
cc316da
acbe27b
f62081f
3ff397f
394e5c8
ce09754
4d5db25
a2f1e0e
fce3f95
deaa050
3baa377
7aa150a
30f653e
f0c58bb
094895e
8fd7416
ba88d44
b9d8314
2533ae7
4847e4e
a8df84c
516706d
f3ceff7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| /** | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To build the list, I ran integration test with all openai model and if the model returns an error, I added it to the blacklist. |
||
| * OpenAI model prefixes that do NOT support tool calls via the chat completions API. | ||
| * | ||
| * Uses prefix matching: model === prefix OR model.startsWith(prefix + '-') | ||
| * | ||
| * Unknown models are allowed by default. | ||
| * If a model fails the integration test, add it here. | ||
| * | ||
| * @see https://platform.openai.com/docs/guides/function-calling | ||
| */ | ||
| const UNSUPPORTED_MODEL_PREFIXES = [ | ||
| // Legacy models | ||
| 'gpt-4', // Base gpt-4 doesn't honor tool_choice: required | ||
| 'text-davinci', | ||
| 'davinci', | ||
| 'curie', | ||
| 'babbage', | ||
| 'ada', | ||
| // O-series reasoning models - don't support parallel_tool_calls | ||
| 'o1', | ||
| 'o3', | ||
| 'o4', | ||
| // Non-chat model families | ||
| 'dall-e', | ||
| 'whisper', | ||
| 'tts', | ||
| 'text-embedding', | ||
| 'omni-moderation', | ||
| 'chatgpt', // chatgpt-4o-latest, chatgpt-image-latest | ||
| 'computer-use', // computer-use-preview | ||
| 'gpt-image', // gpt-image-1, gpt-image-1.5 | ||
| 'gpt-realtime', // gpt-realtime, gpt-realtime-mini | ||
| 'gpt-audio', // gpt-audio | ||
| 'sora', // sora-2, sora-2-pro | ||
| 'codex', // codex-mini-latest | ||
| ]; | ||
|
|
||
| /** | ||
| * OpenAI model patterns that do NOT support tool calls. | ||
| * Uses contains matching: model.includes(pattern) | ||
| */ | ||
| const UNSUPPORTED_MODEL_PATTERNS = [ | ||
| // Non-chat model variants (can appear in the middle of model names) | ||
| '-realtime', | ||
| '-audio', | ||
| '-transcribe', | ||
| '-tts', | ||
| '-search', | ||
| '-codex', | ||
| '-instruct', | ||
| // Models that only support v1/responses, not v1/chat/completions | ||
| '-pro', | ||
| '-deep-research', | ||
| ]; | ||
|
|
||
| /** | ||
| * Models that DO support tool calls even though they match an unsupported prefix. | ||
| * These override the UNSUPPORTED_MODEL_PREFIXES list. | ||
| */ | ||
| const SUPPORTED_MODEL_OVERRIDES = ['gpt-4-turbo', 'gpt-4o', 'gpt-4.1']; | ||
|
|
||
| /** | ||
| * Checks if a model is compatible with Forest Admin AI. | ||
| * | ||
| * Supported models must handle tool calls and the parallel_tool_calls parameter. | ||
| */ | ||
| export default function isModelSupportingTools(model: string): boolean { | ||
| // Check pattern matches first (contains) - these NEVER support tools | ||
| const matchesUnsupportedPattern = UNSUPPORTED_MODEL_PATTERNS.some(pattern => | ||
| model.includes(pattern), | ||
| ); | ||
| if (matchesUnsupportedPattern) return false; | ||
|
|
||
| // Check unsupported prefixes | ||
| const matchesUnsupportedPrefix = UNSUPPORTED_MODEL_PREFIXES.some( | ||
| prefix => model === prefix || model.startsWith(`${prefix}-`), | ||
| ); | ||
|
|
||
| // Check if model is in the supported overrides list | ||
| const isSupportedOverride = SUPPORTED_MODEL_OVERRIDES.some( | ||
| override => model === override || model.startsWith(`${override}-`), | ||
| ); | ||
|
|
||
| // If it matches an unsupported prefix but is not in overrides, reject it | ||
| if (matchesUnsupportedPrefix && !isSupportedOverride) return false; | ||
|
|
||
| return true; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,14 +11,42 @@ import type { Server } from 'http'; | |
|
|
||
| // eslint-disable-next-line import/extensions | ||
| import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; | ||
| import OpenAI from 'openai'; | ||
| import { z } from 'zod'; | ||
|
|
||
| import { Router } from '../src'; | ||
| import runMcpServer from '../src/examples/simple-mcp-server'; | ||
| import isModelSupportingTools from '../src/supported-models'; | ||
|
|
||
| const { OPENAI_API_KEY } = process.env; | ||
| const describeWithOpenAI = OPENAI_API_KEY ? describe : describe.skip; | ||
|
|
||
| /** | ||
| * Fetches available models from OpenAI API. | ||
| * Returns all models that pass `isModelSupportingTools`. | ||
| * | ||
| * If a model fails the integration test, update the blacklist in supported-models.ts. | ||
| */ | ||
| async function fetchChatModelsFromOpenAI(): Promise<string[]> { | ||
| const openai = new OpenAI({ apiKey: OPENAI_API_KEY }); | ||
|
|
||
| let models; | ||
| try { | ||
| models = await openai.models.list(); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Failed to fetch models from OpenAI API. ` + | ||
| `Ensure OPENAI_API_KEY is valid and network is available. ` + | ||
| `Original error: ${error}`, | ||
| ); | ||
| } | ||
|
|
||
| return models.data | ||
| .map(m => m.id) | ||
| .filter(id => isModelSupportingTools(id)) | ||
| .sort(); | ||
| } | ||
|
|
||
| describeWithOpenAI('OpenAI Integration (real API)', () => { | ||
| const router = new Router({ | ||
| aiConfigurations: [ | ||
|
|
@@ -688,4 +716,89 @@ describeWithOpenAI('OpenAI Integration (real API)', () => { | |
| }, 10000); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Model tool support verification', () => { | ||
| let modelsToTest: string[]; | ||
|
|
||
| beforeAll(async () => { | ||
| modelsToTest = await fetchChatModelsFromOpenAI(); | ||
| }); | ||
|
|
||
| it('should have found chat models from OpenAI API', () => { | ||
| expect(modelsToTest.length).toBeGreaterThan(0); | ||
| // eslint-disable-next-line no-console | ||
| console.log(`Testing ${modelsToTest.length} models:`, modelsToTest); | ||
| }); | ||
|
|
||
| it('all chat models should support tool calls', async () => { | ||
| const results: { model: string; success: boolean; error?: string }[] = []; | ||
|
|
||
| for (const model of modelsToTest) { | ||
| const modelRouter = new Router({ | ||
| aiConfigurations: [{ name: 'test', provider: 'openai', model, apiKey: OPENAI_API_KEY }], | ||
| }); | ||
|
|
||
| try { | ||
| const response = (await modelRouter.route({ | ||
| route: 'ai-query', | ||
| body: { | ||
| messages: [{ role: 'user', content: 'What is 2+2?' }], | ||
| tools: [ | ||
| { | ||
| type: 'function', | ||
| function: { | ||
| name: 'calculate', | ||
| description: 'Calculate a math expression', | ||
| parameters: { type: 'object', properties: { result: { type: 'number' } } }, | ||
| }, | ||
| }, | ||
| ], | ||
| tool_choice: 'required', | ||
| parallel_tool_calls: false, | ||
| }, | ||
| })) as ChatCompletionResponse; | ||
|
|
||
| const success = | ||
| response.choices[0].finish_reason === 'tool_calls' && | ||
| response.choices[0].message.tool_calls !== undefined; | ||
|
|
||
| results.push({ model, success }); | ||
| } catch (error) { | ||
| const errorMessage = String(error); | ||
|
|
||
| // Infrastructure errors should fail the test immediately | ||
| const isInfrastructureError = | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. avoid false positive to add the model to the blacklist. Not really clean but for a test, i guess it's ok. |
||
| errorMessage.includes('rate limit') || | ||
| errorMessage.includes('429') || | ||
| errorMessage.includes('401') || | ||
| errorMessage.includes('Authentication') || | ||
| errorMessage.includes('ECONNREFUSED') || | ||
| errorMessage.includes('ETIMEDOUT') || | ||
| errorMessage.includes('getaddrinfo'); | ||
|
|
||
| if (isInfrastructureError) { | ||
| throw new Error(`Infrastructure error testing model ${model}: ${errorMessage}`); | ||
| } | ||
|
|
||
| results.push({ model, success: false, error: errorMessage }); | ||
| } | ||
| } | ||
|
|
||
| const failures = results.filter(r => !r.success); | ||
| if (failures.length > 0) { | ||
| const failedModelNames = failures.map(f => f.model).join(', '); | ||
| // eslint-disable-next-line no-console | ||
| console.error( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error message is important because I want to tell to the developper to update the blacklist |
||
| `\n❌ ${failures.length} model(s) failed: ${failedModelNames}\n\n` + | ||
| `To fix this, add the failing model(s) to the blacklist in:\n` + | ||
| ` packages/ai-proxy/src/supported-models.ts\n\n` + | ||
| `Add to UNSUPPORTED_MODEL_PREFIXES (for prefix match)\n` + | ||
| `or UNSUPPORTED_MODEL_PATTERNS (for contains match)\n`, | ||
| failures, | ||
| ); | ||
| } | ||
|
|
||
| expect(failures).toEqual([]); | ||
| }, 300000); // 5 minutes for all models | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is removed, not really relevant I guess