Browse Source

fix: fix cli logs on deep objects

Catriel Müller 3 tháng trước cách đây
mục cha
commit
2cb95f9ea0

+ 13 - 11
cli/src/host/ExtensionHost.ts

@@ -3,6 +3,7 @@ import { createVSCodeAPIMock, type IdentityInfo, type ExtensionContext } from ".
 import { logs } from "../services/logs.js"
 import type { ExtensionMessage, WebviewMessage, ExtensionState } from "../types/messages.js"
 import { getTelemetryService } from "../services/telemetry/index.js"
+import { argsToMessage } from "../utils/safe-stringify.js"
 
 export interface ExtensionHostOptions {
 	workspacePath: string
@@ -389,29 +390,29 @@ export class ExtensionHost extends EventEmitter {
 		}
 
 		// Override console methods to forward to LogsService ONLY (no console output)
-		// IMPORTANT: Use original console methods to avoid circular dependency
+		// IMPORTANT: Use safe serialization to avoid circular reference errors
 		console.log = (...args: unknown[]) => {
-			const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+			const message = argsToMessage(args)
 			logs.info(message, "Extension")
 		}
 
 		console.error = (...args: unknown[]) => {
-			const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+			const message = argsToMessage(args)
 			logs.error(message, "Extension")
 		}
 
 		console.warn = (...args: unknown[]) => {
-			const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+			const message = argsToMessage(args)
 			logs.warn(message, "Extension")
 		}
 
 		console.debug = (...args: unknown[]) => {
-			const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+			const message = argsToMessage(args)
 			logs.debug(message, "Extension")
 		}
 
 		console.info = (...args: unknown[]) => {
-			const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+			const message = argsToMessage(args)
 			logs.info(message, "Extension")
 		}
 	}
@@ -498,25 +499,26 @@ export class ExtensionHost extends EventEmitter {
 			} as unknown as NodeModule
 
 			// Store the intercepted console in global for module injection
+			// Use safe serialization to avoid circular reference errors
 			;(global as unknown as { __interceptedConsole: Console }).__interceptedConsole = {
 				log: (...args: unknown[]) => {
-					const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+					const message = argsToMessage(args)
 					logs.info(message, "Extension")
 				},
 				error: (...args: unknown[]) => {
-					const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+					const message = argsToMessage(args)
 					logs.error(message, "Extension")
 				},
 				warn: (...args: unknown[]) => {
-					const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+					const message = argsToMessage(args)
 					logs.warn(message, "Extension")
 				},
 				debug: (...args: unknown[]) => {
-					const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+					const message = argsToMessage(args)
 					logs.debug(message, "Extension")
 				},
 				info: (...args: unknown[]) => {
-					const message = args.map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))).join(" ")
+					const message = argsToMessage(args)
 					logs.info(message, "Extension")
 				},
 			} as Console

+ 7 - 2
cli/src/host/VSCode.ts

@@ -1195,8 +1195,13 @@ export class FileSystemAPI {
 		try {
 			const content = fs.readFileSync(uri.fsPath)
 			return new Uint8Array(content)
-		} catch {
-			throw new Error(`Failed to read file: ${uri.fsPath}`)
+		} catch (error) {
+			// Check if it's a file not found error (ENOENT)
+			if ((error as NodeJS.ErrnoException).code === "ENOENT") {
+				throw FileSystemError.FileNotFound(uri)
+			}
+			// For other errors, throw a generic FileSystemError
+			throw new FileSystemError(`Failed to read file: ${uri.fsPath}`)
 		}
 	}
 

+ 266 - 0
cli/src/services/__tests__/logs.test.ts

@@ -0,0 +1,266 @@
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"
+import { LogsService } from "../logs.js"
+
+describe("LogsService", () => {
+	let logsService: LogsService
+
+	beforeEach(() => {
+		// Get a fresh instance for each test
+		logsService = LogsService.getInstance()
+	})
+
+	afterEach(() => {
+		// Clean up test logs
+		logsService.clear()
+	})
+
+	describe("Circular Reference Handling", () => {
+		it("should handle circular references in objects", () => {
+			const obj: Record<string, unknown> = { name: "test" }
+			obj.self = obj // Create circular reference
+
+			// This should not throw
+			expect(() => {
+				logsService.info("Test circular reference", "test", { obj })
+			}).not.toThrow()
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			expect(logs[0]?.context?.obj).toBeDefined()
+		})
+
+		it("should handle circular references in arrays", () => {
+			const arr: unknown[] = [1, 2, 3]
+			arr.push(arr) // Create circular reference
+
+			expect(() => {
+				logsService.info("Test circular array", "test", { arr })
+			}).not.toThrow()
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+		})
+
+		it("should handle deeply nested circular references", () => {
+			const obj1: Record<string, unknown> = { name: "obj1" }
+			const obj2: Record<string, unknown> = { name: "obj2", parent: obj1 }
+			obj1.child = obj2
+			obj2.circular = obj1 // Create circular reference
+
+			expect(() => {
+				logsService.info("Test nested circular", "test", { obj1 })
+			}).not.toThrow()
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+		})
+
+		it("should handle circular references in complex objects like ExtensionContext", () => {
+			// Simulate a VSCode ExtensionContext-like object with circular references
+			const mockContext: Record<string, unknown> = {
+				subscriptions: [],
+				workspaceState: {},
+				globalState: {},
+			}
+
+			const mockSubscription = {
+				dispose: () => {},
+				context: mockContext,
+			}
+
+			;(mockContext.subscriptions as unknown[]).push(mockSubscription)
+
+			expect(() => {
+				logsService.error("Extension error", "test", { context: mockContext })
+			}).not.toThrow()
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			expect(logs[0]?.level).toBe("error")
+		})
+	})
+
+	describe("Error Serialization", () => {
+		it("should serialize Error objects correctly", () => {
+			const error = new Error("Test error")
+			error.stack = "Error: Test error\n    at test.ts:1:1"
+
+			logsService.error("Error occurred", "test", { error })
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			const context = logs[0]?.context as { error: { message: string; name: string; stack: string } }
+			expect(context.error.message).toBe("Test error")
+			expect(context.error.name).toBe("Error")
+			expect(context.error.stack).toContain("Error: Test error")
+		})
+
+		it("should handle custom error properties", () => {
+			class CustomError extends Error {
+				code: string
+				constructor(message: string, code: string) {
+					super(message)
+					this.name = "CustomError"
+					this.code = code
+				}
+			}
+
+			const error = new CustomError("Custom error", "ERR_CUSTOM")
+
+			logsService.error("Custom error occurred", "test", { error })
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			const context = logs[0]?.context as { error: { message: string; name: string; code: string } }
+			expect(context.error.message).toBe("Custom error")
+			expect(context.error.name).toBe("CustomError")
+			expect(context.error.code).toBe("ERR_CUSTOM")
+		})
+	})
+
+	describe("Special Object Types", () => {
+		it("should handle Date objects", () => {
+			const date = new Date("2024-01-01T00:00:00.000Z")
+
+			logsService.info("Date test", "test", { date })
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			const context = logs[0]?.context as { date: string }
+			expect(context.date).toBe("2024-01-01T00:00:00.000Z")
+		})
+
+		it("should handle RegExp objects", () => {
+			const regex = /test\d+/gi
+
+			logsService.info("RegExp test", "test", { regex })
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			const context = logs[0]?.context as { regex: string }
+			expect(context.regex).toBe("/test\\d+/gi")
+		})
+
+		it("should handle null and undefined", () => {
+			logsService.info("Null/undefined test", "test", {
+				nullValue: null,
+				undefinedValue: undefined,
+			})
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			const context = logs[0]?.context as { nullValue: null; undefinedValue: undefined }
+			expect(context.nullValue).toBeNull()
+			expect(context.undefinedValue).toBeUndefined()
+		})
+	})
+
+	describe("Basic Logging", () => {
+		it("should log info messages", () => {
+			logsService.info("Test info", "test")
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			expect(logs[0]?.level).toBe("info")
+			expect(logs[0]?.message).toBe("Test info")
+			expect(logs[0]?.source).toBe("test")
+		})
+
+		it("should log debug messages", () => {
+			logsService.debug("Test debug", "test")
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			expect(logs[0]?.level).toBe("debug")
+		})
+
+		it("should log error messages", () => {
+			logsService.error("Test error", "test")
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			expect(logs[0]?.level).toBe("error")
+		})
+
+		it("should log warn messages", () => {
+			logsService.warn("Test warn", "test")
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(1)
+			expect(logs[0]?.level).toBe("warn")
+		})
+	})
+
+	describe("Log Management", () => {
+		it("should maintain max entries limit", () => {
+			logsService.setMaxEntries(5)
+
+			for (let i = 0; i < 10; i++) {
+				logsService.info(`Message ${i}`, "test")
+			}
+
+			const logs = logsService.getLogs()
+			expect(logs.length).toBeLessThanOrEqual(5)
+		})
+
+		it("should clear all logs", () => {
+			logsService.info("Test 1", "test")
+			logsService.info("Test 2", "test")
+
+			logsService.clear()
+
+			const logs = logsService.getLogs()
+			expect(logs).toHaveLength(0)
+		})
+
+		it("should filter logs by level", () => {
+			logsService.info("Info message", "test")
+			logsService.error("Error message", "test")
+			logsService.debug("Debug message", "test")
+
+			const errorLogs = logsService.getLogs({ levels: ["error"] })
+			expect(errorLogs).toHaveLength(1)
+			expect(errorLogs[0]?.level).toBe("error")
+		})
+
+		it("should filter logs by source", () => {
+			logsService.info("Message 1", "source1")
+			logsService.info("Message 2", "source2")
+			logsService.info("Message 3", "source1")
+
+			const source1Logs = logsService.getLogs({ source: "source1" })
+			expect(source1Logs).toHaveLength(2)
+		})
+	})
+
+	describe("Subscriptions", () => {
+		it("should notify subscribers of new logs", () => {
+			const listener = vi.fn()
+			const unsubscribe = logsService.subscribe(listener)
+
+			logsService.info("Test message", "test")
+
+			expect(listener).toHaveBeenCalledTimes(1)
+			expect(listener).toHaveBeenCalledWith(
+				expect.objectContaining({
+					level: "info",
+					message: "Test message",
+					source: "test",
+				}),
+			)
+
+			unsubscribe()
+		})
+
+		it("should allow unsubscribing", () => {
+			const listener = vi.fn()
+			const unsubscribe = logsService.subscribe(listener)
+
+			logsService.info("Message 1", "test")
+			unsubscribe()
+			logsService.info("Message 2", "test")
+
+			expect(listener).toHaveBeenCalledTimes(1)
+		})
+	})
+})

+ 66 - 14
cli/src/services/logs.ts

@@ -92,25 +92,66 @@ export class LogsService {
 	}
 
 	/**
-	 * Serialize context object, handling Error objects specially
+	 * Safe JSON stringify that handles circular references
+	 */
+	private safeStringify(obj: unknown, seen = new WeakSet()): unknown {
+		// Handle primitives
+		if (obj === null || typeof obj !== "object") {
+			return obj
+		}
+
+		// Handle circular references
+		if (seen.has(obj as object)) {
+			return "[Circular]"
+		}
+
+		// Handle Error objects
+		if (obj instanceof Error) {
+			return this.serializeError(obj)
+		}
+
+		// Handle arrays
+		if (Array.isArray(obj)) {
+			seen.add(obj)
+			const result = obj.map((item) => this.safeStringify(item, seen))
+			seen.delete(obj)
+			return result
+		}
+
+		// Handle Date objects
+		if (obj instanceof Date) {
+			return obj.toISOString()
+		}
+
+		// Handle RegExp objects
+		if (obj instanceof RegExp) {
+			return obj.toString()
+		}
+
+		// Handle plain objects
+		seen.add(obj)
+		const result: Record<string, unknown> = {}
+		for (const [key, value] of Object.entries(obj)) {
+			try {
+				result[key] = this.safeStringify(value, seen)
+			} catch (_error) {
+				// If serialization fails for a property, mark it
+				result[key] = "[Serialization Error]"
+			}
+		}
+		seen.delete(obj)
+		return result
+	}
+
+	/**
+	 * Serialize context object, handling Error objects and circular references
 	 */
 	private serializeContext(context?: Record<string, unknown>): Record<string, unknown> | undefined {
 		if (!context) {
 			return undefined
 		}
 
-		const serialized: Record<string, unknown> = {}
-		for (const [key, value] of Object.entries(context)) {
-			if (value instanceof Error) {
-				serialized[key] = this.serializeError(value)
-			} else if (typeof value === "object" && value !== null) {
-				// Recursively handle nested objects that might contain errors
-				serialized[key] = this.serializeContext(value as Record<string, unknown>) || value
-			} else {
-				serialized[key] = value
-			}
-		}
-		return serialized
+		return this.safeStringify(context) as Record<string, unknown>
 	}
 
 	/**
@@ -225,7 +266,18 @@ export class LogsService {
 		const ts = new Date(entry.ts).toISOString()
 		const source = entry.source ? `[${entry.source}]` : ""
 		const prefix = `${ts} ${source}`
-		const contextStr = entry.context ? ` ${JSON.stringify(entry.context)}` : ""
+
+		// Use safe stringify to handle circular references
+		let contextStr = ""
+		if (entry.context) {
+			try {
+				const safeContext = this.safeStringify(entry.context)
+				contextStr = ` ${JSON.stringify(safeContext)}`
+			} catch (_error) {
+				// Fallback if even safe stringify fails
+				contextStr = " [Context serialization failed]"
+			}
+		}
 
 		switch (entry.level) {
 			case "error":

+ 177 - 0
cli/src/utils/__tests__/safe-stringify.test.ts

@@ -0,0 +1,177 @@
+import { describe, it, expect } from "vitest"
+import { safeStringify, argToString, argsToMessage } from "../safe-stringify.js"
+
+describe("safe-stringify", () => {
+	describe("safeStringify", () => {
+		it("should handle circular references in objects", () => {
+			const obj: { a: number; self?: unknown } = { a: 1 }
+			obj.self = obj
+
+			expect(() => {
+				safeStringify(obj)
+			}).not.toThrow()
+
+			const result = safeStringify(obj) as { a: number; self: string }
+			expect(result.a).toBe(1)
+			expect(result.self).toBe("[Circular]")
+		})
+
+		it("should handle circular references in arrays", () => {
+			const arr: unknown[] = [1, 2]
+			arr.push(arr)
+
+			expect(() => {
+				safeStringify(arr)
+			}).not.toThrow()
+
+			const result = safeStringify(arr) as unknown[]
+			expect(result[0]).toBe(1)
+			expect(result[1]).toBe(2)
+			expect(result[2]).toBe("[Circular]")
+		})
+
+		it("should handle nested circular references", () => {
+			const obj1: { name: string; ref?: unknown } = { name: "obj1" }
+			const obj2: { name: string; ref?: unknown } = { name: "obj2" }
+			obj1.ref = obj2
+			obj2.ref = obj1
+
+			expect(() => {
+				safeStringify(obj1)
+			}).not.toThrow()
+
+			const result = safeStringify(obj1) as { name: string; ref: { name: string; ref: string } }
+			expect(result.name).toBe("obj1")
+			expect(result.ref.name).toBe("obj2")
+			expect(result.ref.ref).toBe("[Circular]")
+		})
+
+		it("should handle Error objects", () => {
+			const error = new Error("Test error")
+			const result = safeStringify(error) as { message: string; name: string; stack: string }
+
+			expect(result.message).toBe("Test error")
+			expect(result.name).toBe("Error")
+			expect(result.stack).toBeDefined()
+		})
+
+		it("should handle custom Error objects with additional properties", () => {
+			class CustomError extends Error {
+				code: string
+				constructor(message: string, code: string) {
+					super(message)
+					this.name = "CustomError"
+					this.code = code
+				}
+			}
+
+			const error = new CustomError("Custom error", "ERR_CUSTOM")
+			const result = safeStringify(error) as { message: string; name: string; code: string }
+
+			expect(result.message).toBe("Custom error")
+			expect(result.name).toBe("CustomError")
+			expect(result.code).toBe("ERR_CUSTOM")
+		})
+
+		it("should handle Date objects", () => {
+			const date = new Date("2024-01-01T00:00:00.000Z")
+			const result = safeStringify(date)
+
+			expect(result).toBe("2024-01-01T00:00:00.000Z")
+		})
+
+		it("should handle RegExp objects", () => {
+			const regex = /test\d+/gi
+			const result = safeStringify(regex)
+
+			expect(result).toBe("/test\\d+/gi")
+		})
+
+		it("should handle null and undefined", () => {
+			expect(safeStringify(null)).toBeNull()
+			expect(safeStringify(undefined)).toBeUndefined()
+		})
+
+		it("should handle primitives", () => {
+			expect(safeStringify(42)).toBe(42)
+			expect(safeStringify("hello")).toBe("hello")
+			expect(safeStringify(true)).toBe(true)
+		})
+
+		it("should handle ExtensionContext-like circular structures", () => {
+			// Simulate the ExtensionContext circular reference structure
+			const subscriptions: unknown[] = []
+			const contextProxy = {
+				originalContext: null as unknown,
+			}
+			const context = {
+				subscriptions,
+				extensionPath: "/path/to/extension",
+			}
+
+			// Create circular reference
+			contextProxy.originalContext = context
+			subscriptions.push({ contextProxy })
+
+			expect(() => {
+				safeStringify(context)
+			}).not.toThrow()
+
+			const result = safeStringify(context) as {
+				subscriptions: Array<{ contextProxy: { originalContext: string } }>
+				extensionPath: string
+			}
+			expect(result.extensionPath).toBe("/path/to/extension")
+			expect(result.subscriptions[0]?.contextProxy.originalContext).toBe("[Circular]")
+		})
+	})
+
+	describe("argToString", () => {
+		it("should handle string arguments", () => {
+			expect(argToString("hello")).toBe("hello")
+		})
+
+		it("should handle object arguments", () => {
+			const result = argToString({ a: 1, b: 2 })
+			expect(result).toBe('{"a":1,"b":2}')
+		})
+
+		it("should handle circular references", () => {
+			const obj: { a: number; self?: unknown } = { a: 1 }
+			obj.self = obj
+
+			expect(() => {
+				argToString(obj)
+			}).not.toThrow()
+
+			const result = argToString(obj)
+			expect(result).toContain('"a":1')
+			expect(result).toContain('"self":"[Circular]"')
+		})
+	})
+
+	describe("argsToMessage", () => {
+		it("should handle multiple arguments", () => {
+			const result = argsToMessage(["hello", 42, { a: 1 }])
+			expect(result).toBe('hello 42 {"a":1}')
+		})
+
+		it("should handle circular references in arguments", () => {
+			const obj: { a: number; self?: unknown } = { a: 1 }
+			obj.self = obj
+
+			expect(() => {
+				argsToMessage(["test", obj])
+			}).not.toThrow()
+
+			const result = argsToMessage(["test", obj])
+			expect(result).toContain("test")
+			expect(result).toContain('"a":1')
+			expect(result).toContain('"self":"[Circular]"')
+		})
+
+		it("should handle empty arguments", () => {
+			expect(argsToMessage([])).toBe("")
+		})
+	})
+})

+ 103 - 0
cli/src/utils/safe-stringify.ts

@@ -0,0 +1,103 @@
+/**
+ * Safe JSON stringification utility that handles circular references,
+ * Error objects, and other special types.
+ */
+
+/**
+ * Serialize an error object to a plain object with all relevant properties
+ */
+function serializeError(error: Error): Record<string, unknown> {
+	return {
+		message: error.message,
+		name: error.name,
+		stack: error.stack,
+		// Include any additional enumerable properties
+		...Object.getOwnPropertyNames(error)
+			.filter((key) => key !== "message" && key !== "name" && key !== "stack")
+			.reduce(
+				(acc, key) => {
+					acc[key] = (error as unknown as Record<string, unknown>)[key]
+					return acc
+				},
+				{} as Record<string, unknown>,
+			),
+	}
+}
+
+/**
+ * Safe stringify that handles circular references, Error objects, Dates, RegExp, etc.
+ * Returns a serializable version of the object.
+ */
+export function safeStringify(obj: unknown, seen = new WeakSet()): unknown {
+	// Handle primitives
+	if (obj === null || typeof obj !== "object") {
+		return obj
+	}
+
+	// Handle circular references
+	if (seen.has(obj as object)) {
+		return "[Circular]"
+	}
+
+	// Handle Error objects
+	if (obj instanceof Error) {
+		return serializeError(obj)
+	}
+
+	// Handle arrays
+	if (Array.isArray(obj)) {
+		seen.add(obj)
+		const result = obj.map((item) => safeStringify(item, seen))
+		seen.delete(obj)
+		return result
+	}
+
+	// Handle Date objects
+	if (obj instanceof Date) {
+		return obj.toISOString()
+	}
+
+	// Handle RegExp objects
+	if (obj instanceof RegExp) {
+		return obj.toString()
+	}
+
+	// Handle plain objects
+	seen.add(obj)
+	const result: Record<string, unknown> = {}
+	for (const [key, value] of Object.entries(obj)) {
+		try {
+			result[key] = safeStringify(value, seen)
+		} catch (_error) {
+			// If serialization fails for a property, mark it
+			result[key] = "[Serialization Error]"
+		}
+	}
+	seen.delete(obj)
+	return result
+}
+
+/**
+ * Convert an argument to a string representation, handling circular references
+ * and special types. This is specifically designed for console logging.
+ */
+export function argToString(arg: unknown): string {
+	if (typeof arg === "string") {
+		return arg
+	}
+
+	try {
+		const safe = safeStringify(arg)
+		return JSON.stringify(safe)
+	} catch (_error) {
+		// Fallback if even safe stringify fails
+		return "[Unable to stringify]"
+	}
+}
+
+/**
+ * Convert multiple arguments to a single string message, handling circular references
+ */
+export function argsToMessage(args: unknown[]): string {
+	return args.map(argToString).join(" ")
+}

+ 2 - 1
src/services/code-index/managed/webview.ts

@@ -60,7 +60,8 @@ export async function tryStartManagedIndexing(provider: ClineProvider): Promise<
  * This should be called whenever the API configuration changes
  */
 export async function updateCodeIndexWithKiloProps(provider: ClineProvider): Promise<void> {
-	console.log("updateCodeIndexWithKiloProps", provider)
+	// ClineProvider it's too big to log
+	//console.log("updateCodeIndexWithKiloProps", provider)
 
 	try {
 		const { apiConfiguration } = await provider.getState()