Add detection of tools with bad schemas and automatically omit them with a warning (#5694)
This commit is contained in:
Jacob MacDonald
GitHub
parent
024b8207eb
commit
b3cfaeb6d3
|
|
@ -12,6 +12,7 @@ import {
|
|||
isEnabled,
|
||||
discoverTools,
|
||||
discoverPrompts,
|
||||
hasValidTypes,
|
||||
} from './mcp-client.js';
|
||||
import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js';
|
||||
import * as SdkClientStdioLib from '@modelcontextprotocol/sdk/client/stdio.js';
|
||||
|
|
@ -97,6 +98,182 @@ describe('mcp-client', () => {
|
|||
`Error discovering tool: 'invalid tool name' from MCP server 'test-server': ${testError.message}`,
|
||||
);
|
||||
});
|
||||
|
||||
it('should skip tools if a parameter is missing a type', async () => {
|
||||
const mockedClient = {} as unknown as ClientLib.Client;
|
||||
const consoleWarnSpy = vi
|
||||
.spyOn(console, 'warn')
|
||||
.mockImplementation(() => {});
|
||||
vi.mocked(GenAiLib.mcpToTool).mockReturnValue({
|
||||
tool: () =>
|
||||
Promise.resolve({
|
||||
functionDeclarations: [
|
||||
{
|
||||
name: 'validTool',
|
||||
parametersJsonSchema: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: { type: 'string' },
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
name: 'invalidTool',
|
||||
parametersJsonSchema: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: { description: 'a param with no type' },
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
}),
|
||||
} as unknown as GenAiLib.CallableTool);
|
||||
|
||||
const tools = await discoverTools('test-server', {}, mockedClient);
|
||||
|
||||
expect(tools.length).toBe(1);
|
||||
expect(vi.mocked(DiscoveredMCPTool).mock.calls[0][2]).toBe('validTool');
|
||||
expect(consoleWarnSpy).toHaveBeenCalledOnce();
|
||||
expect(consoleWarnSpy).toHaveBeenCalledWith(
|
||||
`Skipping tool 'invalidTool' from MCP server 'test-server' because it has ` +
|
||||
`missing types in its parameter schema. Please file an issue with the owner of the MCP server.`,
|
||||
);
|
||||
consoleWarnSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('should skip tools if a nested parameter is missing a type', async () => {
|
||||
const mockedClient = {} as unknown as ClientLib.Client;
|
||||
const consoleWarnSpy = vi
|
||||
.spyOn(console, 'warn')
|
||||
.mockImplementation(() => {});
|
||||
vi.mocked(GenAiLib.mcpToTool).mockReturnValue({
|
||||
tool: () =>
|
||||
Promise.resolve({
|
||||
functionDeclarations: [
|
||||
{
|
||||
name: 'invalidTool',
|
||||
parametersJsonSchema: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
nestedParam: {
|
||||
description: 'a nested param with no type',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
}),
|
||||
} as unknown as GenAiLib.CallableTool);
|
||||
|
||||
const tools = await discoverTools('test-server', {}, mockedClient);
|
||||
|
||||
expect(tools.length).toBe(0);
|
||||
expect(consoleWarnSpy).toHaveBeenCalledOnce();
|
||||
expect(consoleWarnSpy).toHaveBeenCalledWith(
|
||||
`Skipping tool 'invalidTool' from MCP server 'test-server' because it has ` +
|
||||
`missing types in its parameter schema. Please file an issue with the owner of the MCP server.`,
|
||||
);
|
||||
consoleWarnSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('should skip tool if an array item is missing a type', async () => {
|
||||
const mockedClient = {} as unknown as ClientLib.Client;
|
||||
const consoleWarnSpy = vi
|
||||
.spyOn(console, 'warn')
|
||||
.mockImplementation(() => {});
|
||||
vi.mocked(GenAiLib.mcpToTool).mockReturnValue({
|
||||
tool: () =>
|
||||
Promise.resolve({
|
||||
functionDeclarations: [
|
||||
{
|
||||
name: 'invalidTool',
|
||||
parametersJsonSchema: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: {
|
||||
type: 'array',
|
||||
items: {
|
||||
description: 'an array item with no type',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
}),
|
||||
} as unknown as GenAiLib.CallableTool);
|
||||
|
||||
const tools = await discoverTools('test-server', {}, mockedClient);
|
||||
|
||||
expect(tools.length).toBe(0);
|
||||
expect(consoleWarnSpy).toHaveBeenCalledOnce();
|
||||
expect(consoleWarnSpy).toHaveBeenCalledWith(
|
||||
`Skipping tool 'invalidTool' from MCP server 'test-server' because it has ` +
|
||||
`missing types in its parameter schema. Please file an issue with the owner of the MCP server.`,
|
||||
);
|
||||
consoleWarnSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('should discover tool with no properties in schema', async () => {
|
||||
const mockedClient = {} as unknown as ClientLib.Client;
|
||||
const consoleWarnSpy = vi
|
||||
.spyOn(console, 'warn')
|
||||
.mockImplementation(() => {});
|
||||
vi.mocked(GenAiLib.mcpToTool).mockReturnValue({
|
||||
tool: () =>
|
||||
Promise.resolve({
|
||||
functionDeclarations: [
|
||||
{
|
||||
name: 'validTool',
|
||||
parametersJsonSchema: {
|
||||
type: 'object',
|
||||
},
|
||||
},
|
||||
],
|
||||
}),
|
||||
} as unknown as GenAiLib.CallableTool);
|
||||
|
||||
const tools = await discoverTools('test-server', {}, mockedClient);
|
||||
|
||||
expect(tools.length).toBe(1);
|
||||
expect(vi.mocked(DiscoveredMCPTool).mock.calls[0][2]).toBe('validTool');
|
||||
expect(consoleWarnSpy).not.toHaveBeenCalled();
|
||||
consoleWarnSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('should discover tool with empty properties object in schema', async () => {
|
||||
const mockedClient = {} as unknown as ClientLib.Client;
|
||||
const consoleWarnSpy = vi
|
||||
.spyOn(console, 'warn')
|
||||
.mockImplementation(() => {});
|
||||
vi.mocked(GenAiLib.mcpToTool).mockReturnValue({
|
||||
tool: () =>
|
||||
Promise.resolve({
|
||||
functionDeclarations: [
|
||||
{
|
||||
name: 'validTool',
|
||||
parametersJsonSchema: {
|
||||
type: 'object',
|
||||
properties: {},
|
||||
},
|
||||
},
|
||||
],
|
||||
}),
|
||||
} as unknown as GenAiLib.CallableTool);
|
||||
|
||||
const tools = await discoverTools('test-server', {}, mockedClient);
|
||||
|
||||
expect(tools.length).toBe(1);
|
||||
expect(vi.mocked(DiscoveredMCPTool).mock.calls[0][2]).toBe('validTool');
|
||||
expect(consoleWarnSpy).not.toHaveBeenCalled();
|
||||
consoleWarnSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
describe('discoverPrompts', () => {
|
||||
|
|
@ -433,4 +610,163 @@ describe('mcp-client', () => {
|
|||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('hasValidTypes', () => {
|
||||
it('should return true for a valid schema with anyOf', () => {
|
||||
const schema = {
|
||||
anyOf: [{ type: 'string' }, { type: 'number' }],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return false for an invalid schema with anyOf', () => {
|
||||
const schema = {
|
||||
anyOf: [{ type: 'string' }, { description: 'no type' }],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true for a valid schema with allOf', () => {
|
||||
const schema = {
|
||||
allOf: [
|
||||
{ type: 'string' },
|
||||
{ type: 'object', properties: { foo: { type: 'string' } } },
|
||||
],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return false for an invalid schema with allOf', () => {
|
||||
const schema = {
|
||||
allOf: [{ type: 'string' }, { description: 'no type' }],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true for a valid schema with oneOf', () => {
|
||||
const schema = {
|
||||
oneOf: [{ type: 'string' }, { type: 'number' }],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return false for an invalid schema with oneOf', () => {
|
||||
const schema = {
|
||||
oneOf: [{ type: 'string' }, { description: 'no type' }],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true for a valid schema with nested subschemas', () => {
|
||||
const schema = {
|
||||
anyOf: [
|
||||
{ type: 'string' },
|
||||
{
|
||||
allOf: [
|
||||
{ type: 'object', properties: { a: { type: 'string' } } },
|
||||
{ type: 'object', properties: { b: { type: 'number' } } },
|
||||
],
|
||||
},
|
||||
],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return false for an invalid schema with nested subschemas', () => {
|
||||
const schema = {
|
||||
anyOf: [
|
||||
{ type: 'string' },
|
||||
{
|
||||
allOf: [
|
||||
{ type: 'object', properties: { a: { type: 'string' } } },
|
||||
{ description: 'no type' },
|
||||
],
|
||||
},
|
||||
],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true for a schema with a type and subschemas', () => {
|
||||
const schema = {
|
||||
type: 'string',
|
||||
anyOf: [{ minLength: 1 }, { maxLength: 5 }],
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return false for a schema with no type and no subschemas', () => {
|
||||
const schema = {
|
||||
description: 'a schema with no type',
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true for a valid schema', () => {
|
||||
const schema = {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: { type: 'string' },
|
||||
},
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return false if a parameter is missing a type', () => {
|
||||
const schema = {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: { description: 'a param with no type' },
|
||||
},
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return false if a nested parameter is missing a type', () => {
|
||||
const schema = {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
nestedParam: {
|
||||
description: 'a nested param with no type',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return false if an array item is missing a type', () => {
|
||||
const schema = {
|
||||
type: 'object',
|
||||
properties: {
|
||||
param1: {
|
||||
type: 'array',
|
||||
items: {
|
||||
description: 'an array item with no type',
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true for a schema with no properties', () => {
|
||||
const schema = {
|
||||
type: 'object',
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
|
||||
it('should return true for a schema with an empty properties object', () => {
|
||||
const schema = {
|
||||
type: 'object',
|
||||
properties: {},
|
||||
};
|
||||
expect(hasValidTypes(schema)).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -416,6 +416,65 @@ export async function connectAndDiscover(
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Recursively validates that a JSON schema and all its nested properties and
|
||||
* items have a `type` defined.
|
||||
*
|
||||
* @param schema The JSON schema to validate.
|
||||
* @returns `true` if the schema is valid, `false` otherwise.
|
||||
*
|
||||
* @visiblefortesting
|
||||
*/
|
||||
export function hasValidTypes(schema: unknown): boolean {
|
||||
if (typeof schema !== 'object' || schema === null) {
|
||||
// Not a schema object we can validate, or not a schema at all.
|
||||
// Treat as valid as it has no properties to be invalid.
|
||||
return true;
|
||||
}
|
||||
|
||||
const s = schema as Record<string, unknown>;
|
||||
|
||||
if (!s.type) {
|
||||
// These keywords contain an array of schemas that should be validated.
|
||||
//
|
||||
// If no top level type was given, then they must each have a type.
|
||||
let hasSubSchema = false;
|
||||
const schemaArrayKeywords = ['anyOf', 'allOf', 'oneOf'];
|
||||
for (const keyword of schemaArrayKeywords) {
|
||||
const subSchemas = s[keyword];
|
||||
if (Array.isArray(subSchemas)) {
|
||||
hasSubSchema = true;
|
||||
for (const subSchema of subSchemas) {
|
||||
if (!hasValidTypes(subSchema)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// If the node itself is missing a type and had no subschemas, then it isn't valid.
|
||||
if (!hasSubSchema) return false;
|
||||
}
|
||||
|
||||
if (s.type === 'object' && s.properties) {
|
||||
if (typeof s.properties === 'object' && s.properties !== null) {
|
||||
for (const prop of Object.values(s.properties)) {
|
||||
if (!hasValidTypes(prop)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (s.type === 'array' && s.items) {
|
||||
if (!hasValidTypes(s.items)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Discovers and sanitizes tools from a connected MCP client.
|
||||
* It retrieves function declarations from the client, filters out disabled tools,
|
||||
|
|
@ -448,6 +507,15 @@ export async function discoverTools(
|
|||
continue;
|
||||
}
|
||||
|
||||
if (!hasValidTypes(funcDecl.parametersJsonSchema)) {
|
||||
console.warn(
|
||||
`Skipping tool '${funcDecl.name}' from MCP server '${mcpServerName}' ` +
|
||||
`because it has missing types in its parameter schema. Please file an ` +
|
||||
`issue with the owner of the MCP server.`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
|
||||
discoveredTools.push(
|
||||
new DiscoveredMCPTool(
|
||||
mcpCallableTool,
|
||||
|
|
|
|||
Loading…
Reference in New Issue