From 7e5a5e2da79783554dc4e3f00787317db29a589a Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Tue, 5 Aug 2025 18:48:00 -0700 Subject: [PATCH] Detect and warn about cyclic tool refs when schema depth errors are encountered (#5609) Co-authored-by: Jacob Richman --- packages/core/src/core/geminiChat.ts | 39 ++++++++ packages/core/src/tools/tools.test.ts | 125 ++++++++++++++++++++++++++ packages/core/src/tools/tools.ts | 85 ++++++++++++++++++ 3 files changed, 249 insertions(+) create mode 100644 packages/core/src/tools/tools.test.ts diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index bd81400f..c0e41b5e 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -32,6 +32,8 @@ import { ApiResponseEvent, } from '../telemetry/types.js'; import { DEFAULT_GEMINI_FLASH_MODEL } from '../config/models.js'; +import { hasCycleInSchema } from '../tools/tools.js'; +import { isStructuredError } from '../utils/quotaErrorDetection.js'; /** * Returns true if the response is valid, false otherwise. @@ -299,6 +301,10 @@ export class GeminiChat { response = await retryWithBackoff(apiCall, { shouldRetry: (error: Error) => { + // Check for likely cyclic schema errors, don't retry those. + if (error.message.includes('maximum schema depth exceeded')) + return false; + // Check error messages for status codes, or specific error names if known if (error && error.message) { if (error.message.includes('429')) return true; if (error.message.match(/5\d{2}/)) return true; @@ -345,6 +351,7 @@ export class GeminiChat { } catch (error) { const durationMs = Date.now() - startTime; this._logApiError(durationMs, error, prompt_id); + await this.maybeIncludeSchemaDepthContext(error); this.sendPromise = Promise.resolve(); throw error; } @@ -413,6 +420,9 @@ export class GeminiChat { // If errors occur mid-stream, this setup won't resume the stream; it will restart it. const streamResponse = await retryWithBackoff(apiCall, { shouldRetry: (error: Error) => { + // Check for likely cyclic schema errors, don't retry those. + if (error.message.includes('maximum schema depth exceeded')) + return false; // Check error messages for status codes, or specific error names if known if (error && error.message) { if (error.message.includes('429')) return true; @@ -443,6 +453,7 @@ export class GeminiChat { const durationMs = Date.now() - startTime; this._logApiError(durationMs, error, prompt_id); this.sendPromise = Promise.resolve(); + await this.maybeIncludeSchemaDepthContext(error); throw error; } } @@ -674,4 +685,32 @@ export class GeminiChat { content.parts[0].thought === true ); } + + private async maybeIncludeSchemaDepthContext(error: unknown): Promise { + // Check for potentially problematic cyclic tools with cyclic schemas + // and include a recommendation to remove potentially problematic tools. + if ( + isStructuredError(error) && + error.message.includes('maximum schema depth exceeded') + ) { + const tools = (await this.config.getToolRegistry()).getAllTools(); + const cyclicSchemaTools: string[] = []; + for (const tool of tools) { + if ( + (tool.schema.parametersJsonSchema && + hasCycleInSchema(tool.schema.parametersJsonSchema)) || + (tool.schema.parameters && hasCycleInSchema(tool.schema.parameters)) + ) { + cyclicSchemaTools.push(tool.displayName); + } + } + if (cyclicSchemaTools.length > 0) { + const extraDetails = + `\n\nThis error was probably caused by cyclic schema references in one of the following tools, try disabling them:\n\n - ` + + cyclicSchemaTools.join(`\n - `) + + `\n`; + error.message += extraDetails; + } + } + } } diff --git a/packages/core/src/tools/tools.test.ts b/packages/core/src/tools/tools.test.ts new file mode 100644 index 00000000..9942d3a9 --- /dev/null +++ b/packages/core/src/tools/tools.test.ts @@ -0,0 +1,125 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { hasCycleInSchema } from './tools.js'; // Added getStringifiedResultForDisplay + +describe('hasCycleInSchema', () => { + it('should detect a simple direct cycle', () => { + const schema = { + properties: { + data: { + $ref: '#/properties/data', + }, + }, + }; + expect(hasCycleInSchema(schema)).toBe(true); + }); + + it('should detect a cycle from object properties referencing parent properties', () => { + const schema = { + type: 'object', + properties: { + data: { + type: 'object', + properties: { + child: { $ref: '#/properties/data' }, + }, + }, + }, + }; + expect(hasCycleInSchema(schema)).toBe(true); + }); + + it('should detect a cycle from array items referencing parent properties', () => { + const schema = { + type: 'object', + properties: { + data: { + type: 'array', + items: { + type: 'object', + properties: { + child: { $ref: '#/properties/data/items' }, + }, + }, + }, + }, + }; + expect(hasCycleInSchema(schema)).toBe(true); + }); + + it('should detect a cycle between sibling properties', () => { + const schema = { + type: 'object', + properties: { + a: { + type: 'object', + properties: { + child: { $ref: '#/properties/b' }, + }, + }, + b: { + type: 'object', + properties: { + child: { $ref: '#/properties/a' }, + }, + }, + }, + }; + expect(hasCycleInSchema(schema)).toBe(true); + }); + + it('should not detect a cycle in a valid schema', () => { + const schema = { + type: 'object', + properties: { + name: { type: 'string' }, + address: { $ref: '#/definitions/address' }, + }, + definitions: { + address: { + type: 'object', + properties: { + street: { type: 'string' }, + city: { type: 'string' }, + }, + }, + }, + }; + expect(hasCycleInSchema(schema)).toBe(false); + }); + + it('should handle non-cyclic sibling refs', () => { + const schema = { + properties: { + a: { $ref: '#/definitions/stringDef' }, + b: { $ref: '#/definitions/stringDef' }, + }, + definitions: { + stringDef: { type: 'string' }, + }, + }; + expect(hasCycleInSchema(schema)).toBe(false); + }); + + it('should handle nested but not cyclic refs', () => { + const schema = { + properties: { + a: { $ref: '#/definitions/defA' }, + }, + definitions: { + defA: { properties: { b: { $ref: '#/definitions/defB' } } }, + defB: { type: 'string' }, + }, + }; + expect(hasCycleInSchema(schema)).toBe(false); + }); + + it('should return false for an empty schema', () => { + expect(hasCycleInSchema({})).toBe(false); + }); +}); diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 0e3ffabf..5d9d9253 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -228,6 +228,91 @@ export interface ToolResult { }; } +/** + * Detects cycles in a JSON schemas due to `$ref`s. + * @param schema The root of the JSON schema. + * @returns `true` if a cycle is detected, `false` otherwise. + */ +export function hasCycleInSchema(schema: object): boolean { + function resolveRef(ref: string): object | null { + if (!ref.startsWith('#/')) { + return null; + } + const path = ref.substring(2).split('/'); + let current: unknown = schema; + for (const segment of path) { + if ( + typeof current !== 'object' || + current === null || + !Object.prototype.hasOwnProperty.call(current, segment) + ) { + return null; + } + current = (current as Record)[segment]; + } + return current as object; + } + + function traverse( + node: unknown, + visitedRefs: Set, + pathRefs: Set, + ): boolean { + if (typeof node !== 'object' || node === null) { + return false; + } + + if (Array.isArray(node)) { + for (const item of node) { + if (traverse(item, visitedRefs, pathRefs)) { + return true; + } + } + return false; + } + + if ('$ref' in node && typeof node.$ref === 'string') { + const ref = node.$ref; + if (ref === '#/' || pathRefs.has(ref)) { + // A ref to just '#/' is always a cycle. + return true; // Cycle detected! + } + if (visitedRefs.has(ref)) { + return false; // Bail early, we have checked this ref before. + } + + const resolvedNode = resolveRef(ref); + if (resolvedNode) { + // Add it to both visited and the current path + visitedRefs.add(ref); + pathRefs.add(ref); + const hasCycle = traverse(resolvedNode, visitedRefs, pathRefs); + pathRefs.delete(ref); // Backtrack, leaving it in visited + return hasCycle; + } + } + + // Crawl all the properties of node + for (const key in node) { + if (Object.prototype.hasOwnProperty.call(node, key)) { + if ( + traverse( + (node as Record)[key], + visitedRefs, + pathRefs, + ) + ) { + return true; + } + } + } + + return false; + } + + return traverse(schema, new Set(), new Set()); +} + export type ToolResultDisplay = string | FileDiff; export interface FileDiff {