diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 65b42088a2..35175027b5 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -519,10 +519,10 @@ export async function main() { adminControlsListner.setConfig(config); if (config.isInteractive() && settings.merged.general.devtools) { - const { registerActivityLogger } = await import( + const { setupInitialActivityLogger } = await import( './utils/devtoolsService.js' ); - await registerActivityLogger(config); + await setupInitialActivityLogger(config); } // Register config for telemetry shutdown diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 886bfd3587..bc9cd192cf 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -38,9 +38,9 @@ import type { LoadedSettings } from './config/settings.js'; // Mock core modules vi.mock('./ui/hooks/atCommandProcessor.js'); -const mockRegisterActivityLogger = vi.hoisted(() => vi.fn()); +const mockSetupInitialActivityLogger = vi.hoisted(() => vi.fn()); vi.mock('./utils/devtoolsService.js', () => ({ - registerActivityLogger: mockRegisterActivityLogger, + setupInitialActivityLogger: mockSetupInitialActivityLogger, })); const mockCoreEvents = vi.hoisted(() => ({ @@ -286,7 +286,7 @@ describe('runNonInteractive', () => { prompt_id: 'prompt-id-activity-logger', }); - expect(mockRegisterActivityLogger).toHaveBeenCalledWith(mockConfig); + expect(mockSetupInitialActivityLogger).toHaveBeenCalledWith(mockConfig); vi.unstubAllEnvs(); }); @@ -309,7 +309,7 @@ describe('runNonInteractive', () => { prompt_id: 'prompt-id-activity-logger-off', }); - expect(mockRegisterActivityLogger).not.toHaveBeenCalled(); + expect(mockSetupInitialActivityLogger).not.toHaveBeenCalled(); vi.unstubAllEnvs(); }); diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index f8ed72169b..44af6bc81e 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -72,10 +72,10 @@ export async function runNonInteractive({ }); if (process.env['GEMINI_CLI_ACTIVITY_LOG_TARGET']) { - const { registerActivityLogger } = await import( + const { setupInitialActivityLogger } = await import( './utils/devtoolsService.js' ); - await registerActivityLogger(config); + await setupInitialActivityLogger(config); } const { stdout: workingStdout } = createWorkingStdio(); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index de55821a14..c18b9f24e8 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -1518,7 +1518,34 @@ Logging in with Google... Restarting Gemini CLI to continue. } if (keyMatchers[Command.SHOW_ERROR_DETAILS](key)) { - setShowErrorDetails((prev) => !prev); + if (settings.merged.general.devtools) { + void (async () => { + try { + const { startDevToolsServer } = await import( + '../utils/devtoolsService.js' + ); + const { openBrowserSecurely, shouldLaunchBrowser } = await import( + '@google/gemini-cli-core' + ); + const url = await startDevToolsServer(config); + if (shouldLaunchBrowser()) { + try { + await openBrowserSecurely(url); + } catch (e) { + setShowErrorDetails((prev) => !prev); + debugLogger.warn('Failed to open browser securely:', e); + } + } else { + setShowErrorDetails((prev) => !prev); + } + } catch (e) { + setShowErrorDetails(true); + debugLogger.error('Failed to start DevTools server:', e); + } + })(); + } else { + setShowErrorDetails((prev) => !prev); + } return true; } else if (keyMatchers[Command.SUSPEND_APP](key)) { const undoMessage = isITerm2() @@ -1651,6 +1678,7 @@ Logging in with Google... Restarting Gemini CLI to continue. lastOutputTimeRef, tabFocusTimeoutRef, showTransientMessage, + settings.merged.general.devtools, ], ); diff --git a/packages/cli/src/utils/activityLogger.test.ts b/packages/cli/src/utils/activityLogger.test.ts new file mode 100644 index 0000000000..e80eff6485 --- /dev/null +++ b/packages/cli/src/utils/activityLogger.test.ts @@ -0,0 +1,135 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ActivityLogger, type NetworkLog } from './activityLogger.js'; +import type { ConsoleLogPayload } from '@google/gemini-cli-core'; + +describe('ActivityLogger', () => { + let logger: ActivityLogger; + + beforeEach(() => { + logger = ActivityLogger.getInstance(); + logger.clearBufferedLogs(); + }); + + it('buffers the last 10 requests with all their events grouped', () => { + // Emit 15 requests, each with an initial + response event + for (let i = 0; i < 15; i++) { + const initial: NetworkLog = { + id: `req-${i}`, + timestamp: i * 2, + method: 'GET', + url: 'http://example.com', + headers: {}, + pending: true, + }; + logger.emitNetworkEvent(initial); + logger.emitNetworkEvent({ + id: `req-${i}`, + pending: false, + response: { + status: 200, + headers: {}, + body: 'ok', + durationMs: 10, + }, + }); + } + + const logs = logger.getBufferedLogs(); + // 10 requests * 2 events each = 20 events + expect(logs.network.length).toBe(20); + // Oldest kept should be req-5 (first 5 evicted) + expect(logs.network[0].id).toBe('req-5'); + // Last should be req-14 + expect(logs.network[19].id).toBe('req-14'); + }); + + it('keeps all chunk events for a buffered request', () => { + // One request with many chunks + logger.emitNetworkEvent({ + id: 'chunked', + timestamp: 1, + method: 'POST', + url: 'http://example.com', + headers: {}, + pending: true, + }); + for (let i = 0; i < 5; i++) { + logger.emitNetworkEvent({ + id: 'chunked', + pending: true, + chunk: { index: i, data: `chunk-${i}`, timestamp: 2 + i }, + }); + } + logger.emitNetworkEvent({ + id: 'chunked', + pending: false, + response: { status: 200, headers: {}, body: 'done', durationMs: 50 }, + }); + + const logs = logger.getBufferedLogs(); + // 1 initial + 5 chunks + 1 response = 7 events, all for 'chunked' + expect(logs.network.length).toBe(7); + expect(logs.network.every((l) => l.id === 'chunked')).toBe(true); + }); + + it('buffers only the last 10 console logs', () => { + for (let i = 0; i < 15; i++) { + const log: ConsoleLogPayload = { content: `log-${i}`, type: 'log' }; + logger.logConsole(log); + } + + const logs = logger.getBufferedLogs(); + expect(logs.console.length).toBe(10); + expect(logs.console[0].content).toBe('log-5'); + expect(logs.console[9].content).toBe('log-14'); + }); + + it('getBufferedLogs is non-destructive', () => { + logger.logConsole({ content: 'test', type: 'log' }); + const first = logger.getBufferedLogs(); + const second = logger.getBufferedLogs(); + expect(first.console.length).toBe(1); + expect(second.console.length).toBe(1); + }); + + it('clearBufferedLogs empties both buffers', () => { + logger.logConsole({ content: 'test', type: 'log' }); + logger.emitNetworkEvent({ + id: 'r1', + timestamp: 1, + method: 'GET', + url: 'http://example.com', + headers: {}, + }); + logger.clearBufferedLogs(); + const logs = logger.getBufferedLogs(); + expect(logs.console.length).toBe(0); + expect(logs.network.length).toBe(0); + }); + + it('drainBufferedLogs returns and clears atomically', () => { + logger.logConsole({ content: 'drain-test', type: 'log' }); + logger.emitNetworkEvent({ + id: 'r1', + timestamp: 1, + method: 'GET', + url: 'http://example.com', + headers: {}, + }); + + const drained = logger.drainBufferedLogs(); + expect(drained.console.length).toBe(1); + expect(drained.network.length).toBe(1); + + // Buffer should now be empty + const after = logger.getBufferedLogs(); + expect(after.console.length).toBe(0); + expect(after.network.length).toBe(0); + }); +}); diff --git a/packages/cli/src/utils/activityLogger.ts b/packages/cli/src/utils/activityLogger.ts index 721b0d1cb5..cf852257a9 100644 --- a/packages/cli/src/utils/activityLogger.ts +++ b/packages/cli/src/utils/activityLogger.ts @@ -4,23 +4,31 @@ * SPDX-License-Identifier: Apache-2.0 */ -/* eslint-disable @typescript-eslint/no-explicit-any */ -/* eslint-disable @typescript-eslint/no-unnecessary-type-assertion */ -/* eslint-disable @typescript-eslint/no-this-alias */ - import http from 'node:http'; import https from 'node:https'; import zlib from 'node:zlib'; import fs from 'node:fs'; import path from 'node:path'; import { EventEmitter } from 'node:events'; -import { CoreEvent, coreEvents, debugLogger } from '@google/gemini-cli-core'; -import type { Config } from '@google/gemini-cli-core'; +import { + CoreEvent, + coreEvents, + debugLogger, + type ConsoleLogPayload, + type Config, +} from '@google/gemini-cli-core'; import WebSocket from 'ws'; const ACTIVITY_ID_HEADER = 'x-activity-request-id'; const MAX_BUFFER_SIZE = 100; +/** Type guard: Array.isArray doesn't narrow readonly arrays in TS 5.8 */ +function isHeaderRecord( + h: http.OutgoingHttpHeaders | readonly string[], +): h is http.OutgoingHttpHeaders { + return !Array.isArray(h); +} + export interface NetworkLog { id: string; timestamp: number; @@ -43,6 +51,9 @@ export interface NetworkLog { error?: string; } +/** Partial update to an existing network log. */ +export type PartialNetworkLog = { id: string } & Partial; + /** * Capture utility for session activities (network and console). * Provides a stream of events that can be persisted for analysis or inspection. @@ -53,6 +64,14 @@ export class ActivityLogger extends EventEmitter { private requestStartTimes = new Map(); private networkLoggingEnabled = false; + private networkBufferMap = new Map< + string, + Array + >(); + private networkBufferIds: string[] = []; + private consoleBuffer: Array = []; + private readonly bufferLimit = 10; + static getInstance(): ActivityLogger { if (!ActivityLogger.instance) { ActivityLogger.instance = new ActivityLogger(); @@ -73,6 +92,47 @@ export class ActivityLogger extends EventEmitter { return this.networkLoggingEnabled; } + /** + * Atomically returns and clears all buffered logs. + * Prevents data loss from events emitted between get and clear. + */ + drainBufferedLogs(): { + network: Array; + console: Array; + } { + const network: Array = []; + for (const id of this.networkBufferIds) { + const events = this.networkBufferMap.get(id); + if (events) network.push(...events); + } + const console = [...this.consoleBuffer]; + this.networkBufferMap.clear(); + this.networkBufferIds = []; + this.consoleBuffer = []; + return { network, console }; + } + + getBufferedLogs(): { + network: Array; + console: Array; + } { + const network: Array = []; + for (const id of this.networkBufferIds) { + const events = this.networkBufferMap.get(id); + if (events) network.push(...events); + } + return { + network, + console: [...this.consoleBuffer], + }; + } + + clearBufferedLogs(): void { + this.networkBufferMap.clear(); + this.networkBufferIds = []; + this.consoleBuffer = []; + } + private stringifyHeaders(headers: unknown): Record { const result: Record = {}; if (!headers) return result; @@ -91,13 +151,15 @@ export class ActivityLogger extends EventEmitter { return result; } - private sanitizeNetworkLog(log: any): any { + private sanitizeNetworkLog( + log: NetworkLog | PartialNetworkLog, + ): NetworkLog | PartialNetworkLog { if (!log || typeof log !== 'object') return log; const sanitized = { ...log }; // Sanitize request headers - if (sanitized.headers) { + if ('headers' in sanitized && sanitized.headers) { const headers = { ...sanitized.headers }; for (const key of Object.keys(headers)) { if ( @@ -112,7 +174,7 @@ export class ActivityLogger extends EventEmitter { } // Sanitize response headers - if (sanitized.response?.headers) { + if ('response' in sanitized && sanitized.response?.headers) { const resHeaders = { ...sanitized.response.headers }; for (const key of Object.keys(resHeaders)) { if (['set-cookie'].includes(key.toLowerCase())) { @@ -125,8 +187,27 @@ export class ActivityLogger extends EventEmitter { return sanitized; } - private safeEmitNetwork(payload: any) { - this.emit('network', this.sanitizeNetworkLog(payload)); + /** @internal Emit a network event — public for testing only. */ + emitNetworkEvent(payload: NetworkLog | PartialNetworkLog) { + this.safeEmitNetwork(payload); + } + + private safeEmitNetwork(payload: NetworkLog | PartialNetworkLog) { + const sanitized = this.sanitizeNetworkLog(payload); + const id = sanitized.id; + + if (!this.networkBufferMap.has(id)) { + this.networkBufferIds.push(id); + this.networkBufferMap.set(id, []); + // Evict oldest request group if over limit + if (this.networkBufferIds.length > this.bufferLimit) { + const evictId = this.networkBufferIds.shift()!; + this.networkBufferMap.delete(evictId); + } + } + this.networkBufferMap.get(id)!.push(sanitized); + + this.emit('network', sanitized); } enable() { @@ -147,8 +228,7 @@ export class ActivityLogger extends EventEmitter { ? input : input instanceof URL ? input.toString() - : // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - (input as any).url; + : input.url; if (url.includes('127.0.0.1') || url.includes('localhost')) return originalFetch(input, init); @@ -277,57 +357,108 @@ export class ActivityLogger extends EventEmitter { } private patchNodeHttp() { + // eslint-disable-next-line @typescript-eslint/no-this-alias const self = this; const originalRequest = http.request; const originalHttpsRequest = https.request; - const wrapRequest = (originalFn: any, args: any[], protocol: string) => { - const options = args[0]; - const url = - typeof options === 'string' - ? options - : options.href || - `${protocol}//${options.hostname || options.host || 'localhost'}${options.path || '/'}`; + const wrapRequest = ( + originalFn: typeof http.request, + args: unknown[], + protocol: string, + ) => { + const firstArg = args[0]; + let options: http.RequestOptions | string | URL; + if (typeof firstArg === 'string') { + options = firstArg; + } else if (firstArg instanceof URL) { + options = firstArg; + } else { + options = (firstArg ?? {}) as http.RequestOptions; + } + + let url = ''; + if (typeof options === 'string') { + url = options; + } else if (options instanceof URL) { + url = options.href; + } else { + // Some callers pass URL-like objects that include href + const href = + 'href' in options && typeof options.href === 'string' + ? options.href + : ''; + url = + href || + `${protocol}//${options.hostname || options.host || 'localhost'}${options.path || '/'}`; + } if (url.includes('127.0.0.1') || url.includes('localhost')) - return originalFn.apply(http, args); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion + return originalFn.apply(http, args as any); - const headers = - typeof options === 'object' && typeof options !== 'function' - ? (options as any).headers - : {}; - if (headers && headers[ACTIVITY_ID_HEADER]) { + const rawHeaders = + typeof options === 'object' && + options !== null && + !(options instanceof URL) + ? options.headers + : undefined; + let headers: http.OutgoingHttpHeaders = {}; + if (rawHeaders && isHeaderRecord(rawHeaders)) { + headers = rawHeaders; + } + + if (headers[ACTIVITY_ID_HEADER]) { delete headers[ACTIVITY_ID_HEADER]; - return originalFn.apply(http, args); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion + return originalFn.apply(http, args as any); } const id = Math.random().toString(36).substring(7); - self.requestStartTimes.set(id, Date.now()); - const req = originalFn.apply(http, args); + this.requestStartTimes.set(id, Date.now()); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion + const req = originalFn.apply(http, args as any); const requestChunks: Buffer[] = []; const oldWrite = req.write; const oldEnd = req.end; - req.write = function (chunk: any, ...etc: any[]) { + req.write = function (chunk: unknown, ...etc: unknown[]) { if (chunk) { const encoding = // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion typeof etc[0] === 'string' ? (etc[0] as BufferEncoding) : undefined; requestChunks.push( - Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk, encoding), + Buffer.isBuffer(chunk) + ? chunk + : typeof chunk === 'string' + ? Buffer.from(chunk, encoding) + : Buffer.from( + chunk instanceof Uint8Array ? chunk : String(chunk), + ), ); } - return oldWrite.apply(this, [chunk, ...etc]); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion + return oldWrite.apply(this, [chunk, ...etc] as any); }; - req.end = function (this: any, chunk: any, ...etc: any[]) { + req.end = function ( + this: http.ClientRequest, + chunk: unknown, + ...etc: unknown[] + ) { if (chunk && typeof chunk !== 'function') { const encoding = // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion typeof etc[0] === 'string' ? (etc[0] as BufferEncoding) : undefined; requestChunks.push( - Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk, encoding), + Buffer.isBuffer(chunk) + ? chunk + : typeof chunk === 'string' + ? Buffer.from(chunk, encoding) + : Buffer.from( + chunk instanceof Uint8Array ? chunk : String(chunk), + ), ); } const body = Buffer.concat(requestChunks).toString('utf8'); @@ -341,10 +472,11 @@ export class ActivityLogger extends EventEmitter { body, pending: true, }); - return oldEnd.apply(this, [chunk, ...etc]); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion + return (oldEnd as any).apply(this, [chunk, ...etc]); }; - req.on('response', (res: any) => { + req.on('response', (res: http.IncomingMessage) => { const responseChunks: Buffer[] = []; let chunkIndex = 0; @@ -378,7 +510,7 @@ export class ActivityLogger extends EventEmitter { id, pending: false, response: { - status: res.statusCode, + status: res.statusCode || 0, headers: self.stringifyHeaders(res.headers), body: resBody, durationMs, @@ -400,23 +532,34 @@ export class ActivityLogger extends EventEmitter { }); }); - req.on('error', (err: any) => { + req.on('error', (err: Error) => { self.requestStartTimes.delete(id); - const message = err instanceof Error ? err.message : String(err); - self.safeEmitNetwork({ id, pending: false, error: message }); + const message = err.message; + self.safeEmitNetwork({ + id, + pending: false, + error: message, + }); }); return req; }; - http.request = (...args: any[]) => + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion + (http as any).request = (...args: unknown[]) => wrapRequest(originalRequest, args, 'http:'); - https.request = (...args: any[]) => - wrapRequest(originalHttpsRequest, args, 'https:'); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion + (https as any).request = (...args: unknown[]) => + wrapRequest(originalHttpsRequest as typeof http.request, args, 'https:'); } - logConsole(payload: unknown) { - this.emit('console', payload); + logConsole(payload: ConsoleLogPayload) { + const enriched = { ...payload, timestamp: Date.now() }; + this.consoleBuffer.push(enriched); + if (this.consoleBuffer.length > this.bufferLimit) { + this.consoleBuffer.shift(); + } + this.emit('console', enriched); } } @@ -476,7 +619,7 @@ function setupNetworkLogging( config: Config, onReconnectFailed?: () => void, ) { - const buffer: Array> = []; + const transportBuffer: object[] = []; let ws: WebSocket | null = null; let reconnectTimer: NodeJS.Timeout | null = null; let sessionId: string | null = null; @@ -501,8 +644,21 @@ function setupNetworkLogging( ws.on('message', (data: Buffer) => { try { - const message = JSON.parse(data.toString()); - handleServerMessage(message); + const parsed: unknown = JSON.parse(data.toString()); + if ( + typeof parsed === 'object' && + parsed !== null && + 'type' in parsed && + typeof parsed.type === 'string' + ) { + handleServerMessage({ + type: parsed.type, + sessionId: + 'sessionId' in parsed && typeof parsed.sessionId === 'string' + ? parsed.sessionId + : undefined, + }); + } } catch (err) { debugLogger.debug('Invalid WebSocket message:', err); } @@ -523,10 +679,13 @@ function setupNetworkLogging( } }; - const handleServerMessage = (message: any) => { + const handleServerMessage = (message: { + type: string; + sessionId?: string; + }) => { switch (message.type) { case 'registered': - sessionId = message.sessionId; + sessionId = message.sessionId || null; debugLogger.debug(`WebSocket session registered: ${sessionId}`); // Start ping interval @@ -549,13 +708,13 @@ function setupNetworkLogging( } }; - const sendMessage = (message: any) => { + const sendMessage = (message: object) => { if (ws && ws.readyState === WebSocket.OPEN) { ws.send(JSON.stringify(message)); } }; - const sendToNetwork = (type: 'console' | 'network', payload: unknown) => { + const sendToNetwork = (type: 'console' | 'network', payload: object) => { const message = { type, payload, @@ -569,8 +728,8 @@ function setupNetworkLogging( ws.readyState !== WebSocket.OPEN || !capture.isNetworkLoggingEnabled() ) { - buffer.push(message); - if (buffer.length > MAX_BUFFER_SIZE) buffer.shift(); + transportBuffer.push(message); + if (transportBuffer.length > MAX_BUFFER_SIZE) transportBuffer.shift(); return; } @@ -586,9 +745,39 @@ function setupNetworkLogging( return; } - debugLogger.debug(`Flushing ${buffer.length} buffered logs...`); - while (buffer.length > 0) { - const message = buffer.shift()!; + const { network, console: consoleLogs } = capture.drainBufferedLogs(); + const allInitialLogs: Array<{ + type: 'network' | 'console'; + payload: object; + timestamp: number; + }> = [ + ...network.map((l) => ({ + type: 'network' as const, + payload: l, + timestamp: 'timestamp' in l && l.timestamp ? l.timestamp : Date.now(), + })), + ...consoleLogs.map((l) => ({ + type: 'console' as const, + payload: l, + timestamp: l.timestamp, + })), + ].sort((a, b) => a.timestamp - b.timestamp); + + debugLogger.debug( + `Flushing ${allInitialLogs.length} initial buffered logs and ${transportBuffer.length} transport buffered logs...`, + ); + + for (const log of allInitialLogs) { + sendMessage({ + type: log.type, + payload: log.payload, + sessionId: sessionId || config.getSessionId(), + timestamp: Date.now(), + }); + } + + while (transportBuffer.length > 0) { + const message = transportBuffer.shift()!; sendMessage(message); } }; @@ -625,6 +814,7 @@ function setupNetworkLogging( capture.on('console', (payload) => sendToNetwork('console', payload)); capture.on('network', (payload) => sendToNetwork('network', payload)); + capture.on('network-logging-enabled', () => { debugLogger.debug('Network logging enabled, flushing buffer...'); flushBuffer(); @@ -666,7 +856,8 @@ export function initActivityLogger( port: number; onReconnectFailed?: () => void; } - | { mode: 'file'; filePath?: string }, + | { mode: 'file'; filePath?: string } + | { mode: 'buffer' }, ): void { const capture = ActivityLogger.getInstance(); capture.enable(); @@ -680,9 +871,10 @@ export function initActivityLogger( options.onReconnectFailed, ); capture.enableNetworkLogging(); - } else { + } else if (options.mode === 'file') { setupFileLogging(capture, config, options.filePath); } + // buffer mode: no transport, just intercept + bridge bridgeCoreEvents(capture); } diff --git a/packages/cli/src/utils/devtoolsService.test.ts b/packages/cli/src/utils/devtoolsService.test.ts index 2ac9cc9f9e..69d46dee7d 100644 --- a/packages/cli/src/utils/devtoolsService.test.ts +++ b/packages/cli/src/utils/devtoolsService.test.ts @@ -56,9 +56,18 @@ const mockDevToolsInstance = vi.hoisted(() => ({ getPort: vi.fn(), })); +const mockActivityLoggerInstance = vi.hoisted(() => ({ + disableNetworkLogging: vi.fn(), + enableNetworkLogging: vi.fn(), + drainBufferedLogs: vi.fn().mockReturnValue({ network: [], console: [] }), +})); + vi.mock('./activityLogger.js', () => ({ initActivityLogger: mockInitActivityLogger, addNetworkTransport: mockAddNetworkTransport, + ActivityLogger: { + getInstance: () => mockActivityLoggerInstance, + }, })); vi.mock('@google/gemini-cli-core', () => ({ @@ -80,7 +89,11 @@ vi.mock('gemini-cli-devtools', () => ({ })); // --- Import under test (after mocks) --- -import { registerActivityLogger, resetForTesting } from './devtoolsService.js'; +import { + setupInitialActivityLogger, + startDevToolsServer, + resetForTesting, +} from './devtoolsService.js'; function createMockConfig(overrides: Record = {}) { return { @@ -100,104 +113,218 @@ describe('devtoolsService', () => { delete process.env['GEMINI_CLI_ACTIVITY_LOG_TARGET']; }); - describe('registerActivityLogger', () => { - it('connects to existing DevTools server when probe succeeds', async () => { + describe('setupInitialActivityLogger', () => { + it('stays in buffer mode when no existing server found', async () => { const config = createMockConfig(); + const promise = setupInitialActivityLogger(config); - // The probe WebSocket will succeed - const promise = registerActivityLogger(config); + // Probe fires immediately — no server running + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); + MockWebSocket.instances[0].simulateError(); - // Wait for WebSocket to be created - await vi.waitFor(() => { - expect(MockWebSocket.instances.length).toBe(1); + await promise; + + expect(mockInitActivityLogger).toHaveBeenCalledWith(config, { + mode: 'buffer', }); + expect(mockAddNetworkTransport).not.toHaveBeenCalled(); + }); - // Simulate probe success + it('attaches transport when existing server found at startup', async () => { + const config = createMockConfig(); + const promise = setupInitialActivityLogger(config); + + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); MockWebSocket.instances[0].simulateOpen(); await promise; expect(mockInitActivityLogger).toHaveBeenCalledWith(config, { - mode: 'network', - host: '127.0.0.1', - port: 25417, - onReconnectFailed: expect.any(Function), + mode: 'buffer', }); + expect(mockAddNetworkTransport).toHaveBeenCalledWith( + config, + '127.0.0.1', + 25417, + expect.any(Function), + ); + expect( + mockActivityLoggerInstance.enableNetworkLogging, + ).toHaveBeenCalled(); }); - it('starts new DevTools server when probe fails', async () => { + it('F12 short-circuits when startup already connected', async () => { const config = createMockConfig(); - mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25417'); - mockDevToolsInstance.getPort.mockReturnValue(25417); - const promise = registerActivityLogger(config); + // Startup: probe succeeds + const setupPromise = setupInitialActivityLogger(config); + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); + MockWebSocket.instances[0].simulateOpen(); + await setupPromise; - // Wait for probe WebSocket - await vi.waitFor(() => { - expect(MockWebSocket.instances.length).toBe(1); - }); + mockAddNetworkTransport.mockClear(); + mockActivityLoggerInstance.enableNetworkLogging.mockClear(); - // Simulate probe failure - MockWebSocket.instances[0].simulateError(); + // F12: should return URL immediately + const url = await startDevToolsServer(config); - await promise; - - expect(mockDevToolsInstance.start).toHaveBeenCalled(); - expect(mockInitActivityLogger).toHaveBeenCalledWith(config, { - mode: 'network', - host: '127.0.0.1', - port: 25417, - onReconnectFailed: expect.any(Function), - }); + expect(url).toBe('http://localhost:25417'); + expect(mockAddNetworkTransport).not.toHaveBeenCalled(); + expect(mockDevToolsInstance.start).not.toHaveBeenCalled(); }); - it('falls back to file mode when target env var is set', async () => { + it('initializes in file mode when target env var is set', async () => { process.env['GEMINI_CLI_ACTIVITY_LOG_TARGET'] = '/tmp/test.jsonl'; const config = createMockConfig(); - - await registerActivityLogger(config); + await setupInitialActivityLogger(config); expect(mockInitActivityLogger).toHaveBeenCalledWith(config, { mode: 'file', filePath: '/tmp/test.jsonl', }); + // No probe attempted + expect(MockWebSocket.instances.length).toBe(0); }); it('does nothing in file mode when config.storage is missing', async () => { process.env['GEMINI_CLI_ACTIVITY_LOG_TARGET'] = '/tmp/test.jsonl'; const config = createMockConfig({ storage: undefined }); - - await registerActivityLogger(config); + await setupInitialActivityLogger(config); expect(mockInitActivityLogger).not.toHaveBeenCalled(); + expect(MockWebSocket.instances.length).toBe(0); + }); + }); + + describe('startDevToolsServer', () => { + it('starts new server when none exists and enables logging', async () => { + const config = createMockConfig(); + mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25417'); + mockDevToolsInstance.getPort.mockReturnValue(25417); + + const promise = startDevToolsServer(config); + + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); + MockWebSocket.instances[0].simulateError(); + + const url = await promise; + + expect(url).toBe('http://localhost:25417'); + expect(mockAddNetworkTransport).toHaveBeenCalledWith( + config, + '127.0.0.1', + 25417, + expect.any(Function), + ); + expect( + mockActivityLoggerInstance.enableNetworkLogging, + ).toHaveBeenCalled(); }); - it('falls back to file logging when DevTools start fails', async () => { + it('connects to existing server if one is found', async () => { + const config = createMockConfig(); + + const promise = startDevToolsServer(config); + + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); + MockWebSocket.instances[0].simulateOpen(); + + const url = await promise; + + expect(url).toBe('http://localhost:25417'); + expect(mockAddNetworkTransport).toHaveBeenCalled(); + expect( + mockActivityLoggerInstance.enableNetworkLogging, + ).toHaveBeenCalled(); + }); + + it('deduplicates concurrent calls (returns same promise)', async () => { + const config = createMockConfig(); + mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25417'); + mockDevToolsInstance.getPort.mockReturnValue(25417); + + const promise1 = startDevToolsServer(config); + const promise2 = startDevToolsServer(config); + + expect(promise1).toBe(promise2); + + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); + MockWebSocket.instances[0].simulateError(); + + const [url1, url2] = await Promise.all([promise1, promise2]); + expect(url1).toBe('http://localhost:25417'); + expect(url2).toBe('http://localhost:25417'); + // Only one probe + one server start + expect(mockAddNetworkTransport).toHaveBeenCalledTimes(1); + }); + + it('throws when DevTools server fails to start', async () => { const config = createMockConfig(); mockDevToolsInstance.start.mockRejectedValue( new Error('MODULE_NOT_FOUND'), ); - const promise = registerActivityLogger(config); + const promise = startDevToolsServer(config); - // Wait for probe WebSocket - await vi.waitFor(() => { - expect(MockWebSocket.instances.length).toBe(1); - }); - - // Probe fails → tries to start server → server start fails → file fallback + // Probe fails first + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); MockWebSocket.instances[0].simulateError(); - await promise; - - expect(mockInitActivityLogger).toHaveBeenCalledWith(config, { - mode: 'file', - filePath: undefined, - }); + await expect(promise).rejects.toThrow('MODULE_NOT_FOUND'); + expect(mockAddNetworkTransport).not.toHaveBeenCalled(); + }); + + it('allows retry after server start failure', async () => { + const config = createMockConfig(); + mockDevToolsInstance.start.mockRejectedValueOnce( + new Error('MODULE_NOT_FOUND'), + ); + + const promise1 = startDevToolsServer(config); + + // Probe fails + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); + MockWebSocket.instances[0].simulateError(); + + await expect(promise1).rejects.toThrow('MODULE_NOT_FOUND'); + + // Second attempt should work (not return the cached rejected promise) + mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25417'); + mockDevToolsInstance.getPort.mockReturnValue(25417); + + const promise2 = startDevToolsServer(config); + + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(2)); + MockWebSocket.instances[1].simulateError(); + + const url = await promise2; + expect(url).toBe('http://localhost:25417'); + expect(mockAddNetworkTransport).toHaveBeenCalled(); + }); + + it('short-circuits on second F12 after successful start', async () => { + const config = createMockConfig(); + mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25417'); + mockDevToolsInstance.getPort.mockReturnValue(25417); + + const promise1 = startDevToolsServer(config); + + await vi.waitFor(() => expect(MockWebSocket.instances.length).toBe(1)); + MockWebSocket.instances[0].simulateError(); + + const url1 = await promise1; + expect(url1).toBe('http://localhost:25417'); + + mockAddNetworkTransport.mockClear(); + mockDevToolsInstance.start.mockClear(); + + // Second call should short-circuit via connectedUrl + const url2 = await startDevToolsServer(config); + expect(url2).toBe('http://localhost:25417'); + expect(mockAddNetworkTransport).not.toHaveBeenCalled(); + expect(mockDevToolsInstance.start).not.toHaveBeenCalled(); }); - }); - describe('startOrJoinDevTools (via registerActivityLogger)', () => { it('stops own server and connects to existing when losing port race', async () => { const config = createMockConfig(); @@ -205,7 +332,7 @@ describe('devtoolsService', () => { mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25418'); mockDevToolsInstance.getPort.mockReturnValue(25418); - const promise = registerActivityLogger(config); + const promise = startDevToolsServer(config); // First: probe for existing server (fails) await vi.waitFor(() => { @@ -220,16 +347,15 @@ describe('devtoolsService', () => { // Winner is alive MockWebSocket.instances[1].simulateOpen(); - await promise; + const url = await promise; expect(mockDevToolsInstance.stop).toHaveBeenCalled(); - expect(mockInitActivityLogger).toHaveBeenCalledWith( + expect(url).toBe('http://localhost:25417'); + expect(mockAddNetworkTransport).toHaveBeenCalledWith( config, - expect.objectContaining({ - mode: 'network', - host: '127.0.0.1', - port: 25417, // connected to winner's port - }), + '127.0.0.1', + 25417, + expect.any(Function), ); }); @@ -239,7 +365,7 @@ describe('devtoolsService', () => { mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25418'); mockDevToolsInstance.getPort.mockReturnValue(25418); - const promise = registerActivityLogger(config); + const promise = startDevToolsServer(config); // Probe for existing (fails) await vi.waitFor(() => { @@ -253,27 +379,27 @@ describe('devtoolsService', () => { }); MockWebSocket.instances[1].simulateError(); - await promise; + const url = await promise; expect(mockDevToolsInstance.stop).not.toHaveBeenCalled(); - expect(mockInitActivityLogger).toHaveBeenCalledWith( + expect(url).toBe('http://localhost:25418'); + expect(mockAddNetworkTransport).toHaveBeenCalledWith( config, - expect.objectContaining({ - mode: 'network', - port: 25418, // kept own port - }), + '127.0.0.1', + 25418, + expect.any(Function), ); }); }); - describe('handlePromotion (via onReconnectFailed)', () => { + describe('handlePromotion (via startDevToolsServer)', () => { it('caps promotion attempts at MAX_PROMOTION_ATTEMPTS', async () => { const config = createMockConfig(); mockDevToolsInstance.start.mockResolvedValue('http://127.0.0.1:25417'); mockDevToolsInstance.getPort.mockReturnValue(25417); // First: set up the logger so we can grab onReconnectFailed - const promise = registerActivityLogger(config); + const promise = startDevToolsServer(config); await vi.waitFor(() => { expect(MockWebSocket.instances.length).toBe(1); @@ -283,8 +409,8 @@ describe('devtoolsService', () => { await promise; // Extract onReconnectFailed callback - const initCall = mockInitActivityLogger.mock.calls[0]; - const onReconnectFailed = initCall[1].onReconnectFailed; + const initCall = mockAddNetworkTransport.mock.calls[0]; + const onReconnectFailed = initCall[3]; expect(onReconnectFailed).toBeDefined(); // Trigger promotion MAX_PROMOTION_ATTEMPTS + 1 times diff --git a/packages/cli/src/utils/devtoolsService.ts b/packages/cli/src/utils/devtoolsService.ts index 661cd1c0a9..ed5fb36387 100644 --- a/packages/cli/src/utils/devtoolsService.ts +++ b/packages/cli/src/utils/devtoolsService.ts @@ -7,7 +7,11 @@ import { debugLogger } from '@google/gemini-cli-core'; import type { Config } from '@google/gemini-cli-core'; import WebSocket from 'ws'; -import { initActivityLogger, addNetworkTransport } from './activityLogger.js'; +import { + initActivityLogger, + addNetworkTransport, + ActivityLogger, +} from './activityLogger.js'; interface IDevTools { start(): Promise; @@ -20,6 +24,8 @@ const DEFAULT_DEVTOOLS_PORT = 25417; const DEFAULT_DEVTOOLS_HOST = '127.0.0.1'; const MAX_PROMOTION_ATTEMPTS = 3; let promotionAttempts = 0; +let serverStartPromise: Promise | null = null; +let connectedUrl: string | null = null; /** * Probe whether a DevTools server is already listening on the given host:port. @@ -110,70 +116,103 @@ async function handlePromotion(config: Config) { } /** - * Registers the activity logger. - * Captures network and console logs via DevTools WebSocket or to a file. - * - * Environment variable GEMINI_CLI_ACTIVITY_LOG_TARGET controls the output: - * - file path (e.g., "/tmp/logs.jsonl") → file mode - * - not set → auto-start DevTools (reuses existing instance if already running) - * - * @param config The CLI configuration + * Initializes the activity logger. + * Interception starts immediately in buffering mode. + * If an existing DevTools server is found, attaches transport eagerly. */ -export async function registerActivityLogger(config: Config) { +export async function setupInitialActivityLogger(config: Config) { const target = process.env['GEMINI_CLI_ACTIVITY_LOG_TARGET']; - if (!target) { - // No explicit target: try connecting to existing DevTools, then start new one - const onReconnectFailed = () => handlePromotion(config); + if (target) { + if (!config.storage) return; + initActivityLogger(config, { mode: 'file', filePath: target }); + } else { + // Start in buffering mode (no transport attached yet) + initActivityLogger(config, { mode: 'buffer' }); - // Probe for an existing DevTools server - const existing = await probeDevTools( - DEFAULT_DEVTOOLS_HOST, - DEFAULT_DEVTOOLS_PORT, - ); - if (existing) { - debugLogger.log( - `DevTools (existing) at: http://${DEFAULT_DEVTOOLS_HOST}:${DEFAULT_DEVTOOLS_PORT}`, + // Eagerly probe for an existing DevTools server + try { + const existing = await probeDevTools( + DEFAULT_DEVTOOLS_HOST, + DEFAULT_DEVTOOLS_PORT, ); - initActivityLogger(config, { - mode: 'network', - host: DEFAULT_DEVTOOLS_HOST, - port: DEFAULT_DEVTOOLS_PORT, - onReconnectFailed, - }); - return; + if (existing) { + const onReconnectFailed = () => handlePromotion(config); + addNetworkTransport( + config, + DEFAULT_DEVTOOLS_HOST, + DEFAULT_DEVTOOLS_PORT, + onReconnectFailed, + ); + ActivityLogger.getInstance().enableNetworkLogging(); + connectedUrl = `http://localhost:${DEFAULT_DEVTOOLS_PORT}`; + debugLogger.log(`DevTools (existing) at startup: ${connectedUrl}`); + } + } catch { + // Probe failed silently — stay in buffer mode } + } +} +/** + * Starts the DevTools server and opens the UI in the browser. + * Returns the URL to the DevTools UI. + * Deduplicates concurrent calls — returns the same promise if already in flight. + */ +export function startDevToolsServer(config: Config): Promise { + if (connectedUrl) return Promise.resolve(connectedUrl); + if (serverStartPromise) return serverStartPromise; + serverStartPromise = startDevToolsServerImpl(config).catch((err) => { + serverStartPromise = null; + throw err; + }); + return serverStartPromise; +} + +async function startDevToolsServerImpl(config: Config): Promise { + const onReconnectFailed = () => handlePromotion(config); + + // Probe for an existing DevTools server + const existing = await probeDevTools( + DEFAULT_DEVTOOLS_HOST, + DEFAULT_DEVTOOLS_PORT, + ); + + let host = DEFAULT_DEVTOOLS_HOST; + let port = DEFAULT_DEVTOOLS_PORT; + + if (existing) { + debugLogger.log( + `DevTools (existing) at: http://${DEFAULT_DEVTOOLS_HOST}:${DEFAULT_DEVTOOLS_PORT}`, + ); + } else { // No existing server — start (or join if we lose the race) try { const result = await startOrJoinDevTools( DEFAULT_DEVTOOLS_HOST, DEFAULT_DEVTOOLS_PORT, ); - initActivityLogger(config, { - mode: 'network', - host: result.host, - port: result.port, - onReconnectFailed, - }); - return; + host = result.host; + port = result.port; } catch (err) { - debugLogger.debug( - 'Failed to start DevTools, falling back to file logging:', - err, - ); + debugLogger.debug('Failed to start DevTools:', err); + throw err; } } - // File mode fallback - if (!config.storage) { - return; - } + // Promote the activity logger to use the network transport + addNetworkTransport(config, host, port, onReconnectFailed); + const capture = ActivityLogger.getInstance(); + capture.enableNetworkLogging(); - initActivityLogger(config, { mode: 'file', filePath: target }); + const url = `http://localhost:${port}`; + connectedUrl = url; + return url; } /** Reset module-level state — test only. */ export function resetForTesting() { promotionAttempts = 0; + serverStartPromise = null; + connectedUrl = null; }