From ff3722a3a74b09cd25b03de41933944a55db6351 Mon Sep 17 00:00:00 2001 From: Bryan Morgan Date: Mon, 14 Jul 2025 16:20:06 -0400 Subject: [PATCH] Fix circular reference JSON serialization in telemetry logging (#4150) --- .../clearcut-logger/clearcut-logger.ts | 5 +- .../telemetry/integration.test.circular.ts | 62 +++++++++ .../src/telemetry/loggers.test.circular.ts | 119 ++++++++++++++++++ packages/core/src/telemetry/loggers.ts | 3 +- .../core/src/utils/quotaErrorDetection.ts | 17 +-- .../core/src/utils/safeJsonStringify.test.ts | 73 +++++++++++ packages/core/src/utils/safeJsonStringify.ts | 32 +++++ 7 files changed, 294 insertions(+), 17 deletions(-) create mode 100644 packages/core/src/telemetry/integration.test.circular.ts create mode 100644 packages/core/src/telemetry/loggers.test.circular.ts create mode 100644 packages/core/src/utils/safeJsonStringify.test.ts create mode 100644 packages/core/src/utils/safeJsonStringify.ts diff --git a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts index cd2abe81..07c40c86 100644 --- a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts +++ b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts @@ -23,6 +23,7 @@ import { getCachedGoogleAccount, getLifetimeGoogleAccounts, } from '../../utils/user_account.js'; +import { safeJsonStringify } from '../../utils/safeJsonStringify.js'; const start_session_event_name = 'start_session'; const new_prompt_event_name = 'new_prompt'; @@ -65,7 +66,7 @@ export class ClearcutLogger { this.events.push([ { event_time_ms: Date.now(), - source_extension_json: JSON.stringify(event), + source_extension_json: safeJsonStringify(event), }, ]); } @@ -121,7 +122,7 @@ export class ClearcutLogger { log_event: eventsToSend, }, ]; - const body = JSON.stringify(request); + const body = safeJsonStringify(request); const options = { hostname: 'play.googleapis.com', path: '/log', diff --git a/packages/core/src/telemetry/integration.test.circular.ts b/packages/core/src/telemetry/integration.test.circular.ts new file mode 100644 index 00000000..958ec3cb --- /dev/null +++ b/packages/core/src/telemetry/integration.test.circular.ts @@ -0,0 +1,62 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Integration test to verify circular reference handling with proxy agents + */ + +import { describe, it, expect } from 'vitest'; +import { ClearcutLogger } from './clearcut-logger/clearcut-logger.js'; +import { Config } from '../config/config.js'; + +describe('Circular Reference Integration Test', () => { + it('should handle HttpsProxyAgent-like circular references in clearcut logging', () => { + // Create a mock config with proxy + const mockConfig = { + getTelemetryEnabled: () => true, + getUsageStatisticsEnabled: () => true, + getSessionId: () => 'test-session', + getModel: () => 'test-model', + getEmbeddingModel: () => 'test-embedding', + getDebugMode: () => false, + getProxy: () => 'http://proxy.example.com:8080', + } as unknown as Config; + + // Simulate the structure that causes the circular reference error + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const proxyAgentLike: any = { + sockets: {}, + options: { proxy: 'http://proxy.example.com:8080' }, + }; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const socketLike: any = { + _httpMessage: { + agent: proxyAgentLike, + socket: null, + }, + }; + + socketLike._httpMessage.socket = socketLike; // Create circular reference + proxyAgentLike.sockets['cloudcode-pa.googleapis.com:443'] = [socketLike]; + + // Create an event that would contain this circular structure + const problematicEvent = { + error: new Error('Network error'), + function_args: { + filePath: '/test/file.txt', + httpAgent: proxyAgentLike, // This would cause the circular reference + }, + }; + + // Test that ClearcutLogger can handle this + const logger = ClearcutLogger.getInstance(mockConfig); + + expect(() => { + logger?.enqueueLogEvent(problematicEvent); + }).not.toThrow(); + }); +}); diff --git a/packages/core/src/telemetry/loggers.test.circular.ts b/packages/core/src/telemetry/loggers.test.circular.ts new file mode 100644 index 00000000..62a61bfd --- /dev/null +++ b/packages/core/src/telemetry/loggers.test.circular.ts @@ -0,0 +1,119 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Test to verify circular reference handling in telemetry logging + */ + +import { describe, it, expect } from 'vitest'; +import { logToolCall } from './loggers.js'; +import { ToolCallEvent } from './types.js'; +import { Config } from '../config/config.js'; +import { CompletedToolCall } from '../core/coreToolScheduler.js'; +import { ToolCallRequestInfo, ToolCallResponseInfo } from '../core/turn.js'; +import { Tool } from '../tools/tools.js'; + +describe('Circular Reference Handling', () => { + it('should handle circular references in tool function arguments', () => { + // Create a mock config + const mockConfig = { + getTelemetryEnabled: () => true, + getUsageStatisticsEnabled: () => true, + getSessionId: () => 'test-session', + getModel: () => 'test-model', + getEmbeddingModel: () => 'test-embedding', + getDebugMode: () => false, + } as unknown as Config; + + // Create an object with circular references (similar to HttpsProxyAgent) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const circularObject: any = { + sockets: {}, + agent: null, + }; + circularObject.agent = circularObject; // Create circular reference + circularObject.sockets['test-host'] = [ + { _httpMessage: { agent: circularObject } }, + ]; + + // Create a mock CompletedToolCall with circular references in function_args + const mockRequest: ToolCallRequestInfo = { + callId: 'test-call-id', + name: 'ReadFile', + args: circularObject, // This would cause the original error + isClientInitiated: false, + prompt_id: 'test-prompt-id', + }; + + const mockResponse: ToolCallResponseInfo = { + callId: 'test-call-id', + responseParts: [{ text: 'test result' }], + resultDisplay: undefined, + error: undefined, // undefined means success + }; + + const mockCompletedToolCall: CompletedToolCall = { + status: 'success', + request: mockRequest, + response: mockResponse, + tool: {} as Tool, + durationMs: 100, + }; + + // Create a tool call event with circular references in function_args + const event = new ToolCallEvent(mockCompletedToolCall); + + // This should not throw an error + expect(() => { + logToolCall(mockConfig, event); + }).not.toThrow(); + }); + + it('should handle normal objects without circular references', () => { + const mockConfig = { + getTelemetryEnabled: () => true, + getUsageStatisticsEnabled: () => true, + getSessionId: () => 'test-session', + getModel: () => 'test-model', + getEmbeddingModel: () => 'test-embedding', + getDebugMode: () => false, + } as unknown as Config; + + const normalObject = { + filePath: '/test/path', + options: { encoding: 'utf8' }, + }; + + const mockRequest: ToolCallRequestInfo = { + callId: 'test-call-id', + name: 'ReadFile', + args: normalObject, + isClientInitiated: false, + prompt_id: 'test-prompt-id', + }; + + const mockResponse: ToolCallResponseInfo = { + callId: 'test-call-id', + responseParts: [{ text: 'test result' }], + resultDisplay: undefined, + error: undefined, // undefined means success + }; + + const mockCompletedToolCall: CompletedToolCall = { + status: 'success', + request: mockRequest, + response: mockResponse, + tool: {} as Tool, + durationMs: 100, + }; + + const event = new ToolCallEvent(mockCompletedToolCall); + + expect(() => { + logToolCall(mockConfig, event); + }).not.toThrow(); + }); +}); diff --git a/packages/core/src/telemetry/loggers.ts b/packages/core/src/telemetry/loggers.ts index 5929ec58..3cf3794b 100644 --- a/packages/core/src/telemetry/loggers.ts +++ b/packages/core/src/telemetry/loggers.ts @@ -35,6 +35,7 @@ import { import { isTelemetrySdkInitialized } from './sdk.js'; import { uiTelemetryService, UiEvent } from './uiTelemetry.js'; import { ClearcutLogger } from './clearcut-logger/clearcut-logger.js'; +import { safeJsonStringify } from '../utils/safeJsonStringify.js'; const shouldLogUserPrompts = (config: Config): boolean => config.getTelemetryLogPromptsEnabled(); @@ -115,7 +116,7 @@ export function logToolCall(config: Config, event: ToolCallEvent): void { ...event, 'event.name': EVENT_TOOL_CALL, 'event.timestamp': new Date().toISOString(), - function_args: JSON.stringify(event.function_args, null, 2), + function_args: safeJsonStringify(event.function_args, 2), }; if (event.error) { attributes['error.message'] = event.error; diff --git a/packages/core/src/utils/quotaErrorDetection.ts b/packages/core/src/utils/quotaErrorDetection.ts index a8e87a5d..b07309cd 100644 --- a/packages/core/src/utils/quotaErrorDetection.ts +++ b/packages/core/src/utils/quotaErrorDetection.ts @@ -44,20 +44,9 @@ export function isProQuotaExceededError(error: unknown): boolean { // - "Quota exceeded for quota metric 'Gemini 2.5-preview Pro Requests'" // We use string methods instead of regex to avoid ReDoS vulnerabilities - const checkMessage = (message: string): boolean => { - console.log('[DEBUG] isProQuotaExceededError checking message:', message); - const result = - message.includes("Quota exceeded for quota metric 'Gemini") && - message.includes("Pro Requests'"); - console.log('[DEBUG] isProQuotaExceededError result:', result); - return result; - }; - - // Log the full error object to understand its structure - console.log( - '[DEBUG] isProQuotaExceededError - full error object:', - JSON.stringify(error, null, 2), - ); + const checkMessage = (message: string): boolean => + message.includes("Quota exceeded for quota metric 'Gemini") && + message.includes("Pro Requests'"); if (typeof error === 'string') { return checkMessage(error); diff --git a/packages/core/src/utils/safeJsonStringify.test.ts b/packages/core/src/utils/safeJsonStringify.test.ts new file mode 100644 index 00000000..9a38c048 --- /dev/null +++ b/packages/core/src/utils/safeJsonStringify.test.ts @@ -0,0 +1,73 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { safeJsonStringify } from './safeJsonStringify.js'; + +describe('safeJsonStringify', () => { + it('should stringify normal objects without issues', () => { + const obj = { name: 'test', value: 42 }; + const result = safeJsonStringify(obj); + expect(result).toBe('{"name":"test","value":42}'); + }); + + it('should handle circular references by replacing them with [Circular]', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const obj: any = { name: 'test' }; + obj.circular = obj; // Create circular reference + + const result = safeJsonStringify(obj); + expect(result).toBe('{"name":"test","circular":"[Circular]"}'); + }); + + it('should handle complex circular structures like HttpsProxyAgent', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const agent: any = { + sockets: {}, + options: { host: 'example.com' }, + }; + agent.sockets['example.com'] = [{ agent }]; + + const result = safeJsonStringify(agent); + expect(result).toContain('[Circular]'); + expect(result).toContain('example.com'); + }); + + it('should respect the space parameter for formatting', () => { + const obj = { name: 'test', value: 42 }; + const result = safeJsonStringify(obj, 2); + expect(result).toBe('{\n "name": "test",\n "value": 42\n}'); + }); + + it('should handle circular references with formatting', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const obj: any = { name: 'test' }; + obj.circular = obj; + + const result = safeJsonStringify(obj, 2); + expect(result).toBe('{\n "name": "test",\n "circular": "[Circular]"\n}'); + }); + + it('should handle arrays with circular references', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const arr: any[] = [{ id: 1 }]; + arr[0].parent = arr; // Create circular reference + + const result = safeJsonStringify(arr); + expect(result).toBe('[{"id":1,"parent":"[Circular]"}]'); + }); + + it('should handle null and undefined values', () => { + expect(safeJsonStringify(null)).toBe('null'); + expect(safeJsonStringify(undefined)).toBe(undefined); + }); + + it('should handle primitive values', () => { + expect(safeJsonStringify('test')).toBe('"test"'); + expect(safeJsonStringify(42)).toBe('42'); + expect(safeJsonStringify(true)).toBe('true'); + }); +}); diff --git a/packages/core/src/utils/safeJsonStringify.ts b/packages/core/src/utils/safeJsonStringify.ts new file mode 100644 index 00000000..f439bcea --- /dev/null +++ b/packages/core/src/utils/safeJsonStringify.ts @@ -0,0 +1,32 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Safely stringifies an object to JSON, handling circular references by replacing them with [Circular]. + * + * @param obj - The object to stringify + * @param space - Optional space parameter for formatting (defaults to no formatting) + * @returns JSON string with circular references replaced by [Circular] + */ +export function safeJsonStringify( + obj: unknown, + space?: string | number, +): string { + const seen = new WeakSet(); + return JSON.stringify( + obj, + (key, value) => { + if (typeof value === 'object' && value !== null) { + if (seen.has(value)) { + return '[Circular]'; + } + seen.add(value); + } + return value; + }, + space, + ); +}