fix(core): normalize MCP tool schemas to ensure type:object at root - #28839
fix(core): normalize MCP tool schemas to ensure type:object at root#28839Xsidz wants to merge 1 commit into
Conversation
|
📊 PR Size: size/M
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses compatibility issues where MCP servers provide malformed or non-object tool schemas that fail strict JSON Schema validation. By centralizing schema normalization, the core logic now guarantees that all tool definitions conform to the expected object structure, improving reliability when interacting with strict downstream services. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function normalizeToolSchema to standardize tool parameter schemas, ensuring they have type: 'object' at the root as required by strict JSON Schema validators like Vertex AI. This helper is integrated into mcp-client.ts and tool-registry.ts, replacing inline fallback logic, and unit tests are added to verify its behavior. The review feedback suggests enhancing normalizeToolSchema to guarantee that a valid properties object is always present when type is 'object' (even if the input schema lacks it or has a non-object type) and updating the unit tests accordingly.
| export function normalizeToolSchema(schema: unknown): Record<string, unknown> { | ||
| if (schema && typeof schema === 'object' && !Array.isArray(schema)) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| const s = schema as Record<string, unknown>; | ||
| return s['type'] === 'object' ? s : { ...s, type: 'object' }; | ||
| } | ||
| return { type: 'object', properties: {} }; | ||
| } |
There was a problem hiding this comment.
To ensure robust schema normalization and prevent downstream validation or runtime crashes, normalizeToolSchema should guarantee that properties is always a valid object when type is 'object'. Currently, if the input schema is {} or { type: 'string' }, the returned schema will lack a properties field, which is inconsistent with the fallback behavior for null/undefined and can cause strict JSON Schema validators (like Vertex AI) or other parts of the codebase to fail.
We should also ensure that if properties is present but malformed (e.g., null or an array), it is normalized to an empty object {}.
export function normalizeToolSchema(schema: unknown): Record<string, unknown> {
if (schema && typeof schema === 'object' && !Array.isArray(schema)) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
const s = schema as Record<string, unknown>;
const hasValidProperties = s['properties'] && typeof s['properties'] === 'object' && !Array.isArray(s['properties']);
const properties = hasValidProperties ? s['properties'] as Record<string, unknown> : {};
if (s['type'] === 'object' && hasValidProperties) {
return s;
}
return { ...s, type: 'object', properties };
}
return { type: 'object', properties: {} };
}| describe('normalizeToolSchema', () => { | ||
| it('passes through a valid {type:object} schema unchanged', () => { | ||
| const s = { type: 'object', properties: { x: { type: 'string' } } }; | ||
| expect(normalizeToolSchema(s)).toBe(s); | ||
| }); | ||
| it('injects type:object when type is missing', () => { | ||
| expect(normalizeToolSchema({ properties: {} })).toEqual({ | ||
| properties: {}, | ||
| type: 'object', | ||
| }); | ||
| }); | ||
| it('overrides a non-object type', () => { | ||
| expect(normalizeToolSchema({ type: 'string' })).toEqual({ type: 'object' }); | ||
| }); | ||
| it('returns default schema for null/undefined/array/primitive', () => { | ||
| const def = { type: 'object', properties: {} }; | ||
| expect(normalizeToolSchema(null)).toEqual(def); | ||
| expect(normalizeToolSchema(undefined)).toEqual(def); | ||
| expect(normalizeToolSchema([])).toEqual(def); | ||
| expect(normalizeToolSchema('string')).toEqual(def); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Update the test cases for normalizeToolSchema to reflect the robust normalization behavior where properties is always guaranteed to be a valid object.
describe('normalizeToolSchema', () => {
it('passes through a valid {type:object} schema unchanged', () => {
const s = { type: 'object', properties: { x: { type: 'string' } } };
expect(normalizeToolSchema(s)).toBe(s);
});
it('injects type:object when type is missing', () => {
expect(normalizeToolSchema({ properties: {} })).toEqual({
properties: {},
type: 'object',
});
});
it('overrides a non-object type and ensures properties is present', () => {
expect(normalizeToolSchema({ type: 'string' })).toEqual({ type: 'object', properties: {} });
});
it('returns default schema for null/undefined/array/primitive', () => {
const def = { type: 'object', properties: {} };
expect(normalizeToolSchema(null)).toEqual(def);
expect(normalizeToolSchema(undefined)).toEqual(def);
expect(normalizeToolSchema([])).toEqual(def);
expect(normalizeToolSchema('string')).toEqual(def);
});
});
gemini-cli
Bot
added
priority/p2
Fixes google-gemini#23382 MCP servers sometimes advertise tool schemas with missing type, non-object type, or malformed structure. Strict providers (Vertex AI strict mode) reject these with 'Input should be object'. Added normalizeToolSchema() in mcp-tool.ts and applied it at both call sites: DiscoveredMCPTool instantiation in mcp-client.ts and DiscoveredTool registration in tool-registry.ts.
92c19fa to
c95e900
Compare
|
Fixed in c95e900: normalizeToolSchema now also ensures properties is always a valid object — malformed or missing properties fields default to {}. |
1 participant
Summary
type, a non-objecttype, or a completely malformed structure. Strict JSON Schema validators (Vertex AI in strict mode, or any provider that validates schemas before forwarding) reject these withtools.N.custom.input_schema.type: Input should be 'object'.normalizeToolSchema()inmcp-tool.tsand applied it at both instantiation points.Details
normalizeToolSchema(schema):{type: 'object', ...}schema → returned as-is (no allocation)typeor with wrongtype→ spreads existing fields, injectstype: 'object'null/undefined/array/primitive → returns{type: 'object', properties: {}}Applied at:
mcp-client.tsline 1374:toolDef.inputSchema ?? {…}→normalizeToolSchema(toolDef.inputSchema)tool-registry.tslines 500–505: replaced the existing object/array guard →normalizeToolSchema(func.parametersJsonSchema)Both callers already import from
mcp-tool.tsso no new dependency edge.Related Issues
Fixes #23382
How to Validate
npm test -w @google/gemini-cli-core -- src/tools/mcp-tool.test.tsPre-Merge Checklist
normalizeToolSchematype: 'object'injected