diff --git a/packages/cli/src/utils/sessions.test.ts b/packages/cli/src/utils/sessions.test.ts index 5c91bf0d50..e457234d42 100644 --- a/packages/cli/src/utils/sessions.test.ts +++ b/packages/cli/src/utils/sessions.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { ChatRecordingService, type Config } from '@google/gemini-cli-core'; +import { deleteStoredSession, type Config } from '@google/gemini-cli-core'; import { listSessions, deleteSession } from './sessions.js'; import { SessionSelector, type SessionInfo } from './sessionUtils.js'; @@ -14,7 +14,7 @@ const mocks = vi.hoisted(() => ({ writeToStderr: vi.fn(), })); -// Mock the SessionSelector and ChatRecordingService +// Mock the SessionSelector and deleteStoredSession. vi.mock('./sessionUtils.js', () => ({ SessionSelector: vi.fn(), formatRelativeTime: vi.fn(() => 'some time ago'), @@ -24,7 +24,7 @@ vi.mock('@google/gemini-cli-core', async () => { const actual = await vi.importActual('@google/gemini-cli-core'); return { ...actual, - ChatRecordingService: vi.fn(), + deleteStoredSession: vi.fn(), generateSummary: vi.fn().mockResolvedValue(undefined), writeToStdout: mocks.writeToStdout, writeToStderr: mocks.writeToStderr, @@ -347,7 +347,8 @@ describe('deleteSession', () => { // Create mock methods mockListSessions = vi.fn(); - mockDeleteSession = vi.fn(); + mockDeleteSession = vi.mocked(deleteStoredSession); + mockDeleteSession.mockReset(); // Mock SessionSelector constructor vi.mocked(SessionSelector).mockImplementation( @@ -356,14 +357,6 @@ describe('deleteSession', () => { listSessions: mockListSessions, }) as unknown as InstanceType, ); - - // Mock ChatRecordingService - vi.mocked(ChatRecordingService).mockImplementation( - () => - ({ - deleteSession: mockDeleteSession, - }) as unknown as InstanceType, - ); }); afterEach(() => { @@ -411,7 +404,10 @@ describe('deleteSession', () => { // Assert expect(mockListSessions).toHaveBeenCalledOnce(); - expect(mockDeleteSession).toHaveBeenCalledWith('session-file-123'); + expect(mockDeleteSession).toHaveBeenCalledWith( + mockConfig, + 'session-file-123', + ); expect(mocks.writeToStdout).toHaveBeenCalledWith( 'Deleted session 1: Test session (some time ago)', ); @@ -458,7 +454,10 @@ describe('deleteSession', () => { // Assert expect(mockListSessions).toHaveBeenCalledOnce(); - expect(mockDeleteSession).toHaveBeenCalledWith('session-file-2'); + expect(mockDeleteSession).toHaveBeenCalledWith( + mockConfig, + 'session-file-2', + ); expect(mocks.writeToStdout).toHaveBeenCalledWith( 'Deleted session 2: Second session (some time ago)', ); @@ -641,7 +640,10 @@ describe('deleteSession', () => { await deleteSession(mockConfig, '1'); // Assert - expect(mockDeleteSession).toHaveBeenCalledWith('session-file-1'); + expect(mockDeleteSession).toHaveBeenCalledWith( + mockConfig, + 'session-file-1', + ); expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Failed to delete session: File deletion failed', ); @@ -732,7 +734,10 @@ describe('deleteSession', () => { await deleteSession(mockConfig, '1'); // Assert - expect(mockDeleteSession).toHaveBeenCalledWith('session-file-1'); + expect(mockDeleteSession).toHaveBeenCalledWith( + mockConfig, + 'session-file-1', + ); expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('Oldest session'), ); diff --git a/packages/cli/src/utils/sessions.ts b/packages/cli/src/utils/sessions.ts index 8b62376ff8..96674466d8 100644 --- a/packages/cli/src/utils/sessions.ts +++ b/packages/cli/src/utils/sessions.ts @@ -5,7 +5,7 @@ */ import { - ChatRecordingService, + deleteStoredSession, generateSummary, writeToStderr, writeToStdout, @@ -95,9 +95,7 @@ export async function deleteSession( } try { - // Use ChatRecordingService to delete the session - const chatRecordingService = new ChatRecordingService(config); - await chatRecordingService.deleteSession(sessionToDelete.file); + await deleteStoredSession(config, sessionToDelete.file); const time = formatRelativeTime(sessionToDelete.lastUpdated); writeToStdout( diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index ca76a0e499..d52d88bd7f 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -12,7 +12,7 @@ import { sanitizeFilenamePart } from '../utils/fileUtils.js'; import { isNodeError } from '../utils/errors.js'; import { deleteSessionArtifactsAsync, - deleteSubagentSessionDirAndArtifactsAsync, + deleteStoredSession, } from '../utils/sessionOperations.js'; import readline from 'node:readline'; import { randomUUID } from 'node:crypto'; @@ -98,10 +98,6 @@ function isTextPart(part: unknown): part is { text: string } { return isStringProperty(part, 'text'); } -function isSessionIdRecord(record: unknown): record is { sessionId: string } { - return isStringProperty(record, 'sessionId'); -} - export async function loadConversationRecord( filePath: string, options?: LoadConversationOptions, @@ -702,140 +698,7 @@ export class ChatRecordingService { * @throws {Error} If shortId validation fails. */ async deleteSession(sessionIdOrBasename: string): Promise { - try { - const tempDir = this.context.config.storage.getProjectTempDir(); - const chatsDir = path.join(tempDir, 'chats'); - const shortId = this.deriveShortId(sessionIdOrBasename); - - // Using stat instead of existsSync for async sanity - if (!(await fs.promises.stat(chatsDir).catch(() => null))) { - return; // Nothing to delete - } - - const matchingFiles = await this.getMatchingSessionFiles( - chatsDir, - shortId, - ); - for (const file of matchingFiles) { - await this.deleteSessionAndArtifacts(chatsDir, file, tempDir); - } - } catch (error) { - debugLogger.error('Error deleting session file.', error); - throw error; - } - } - - private deriveShortId(sessionIdOrBasename: string): string { - let shortId = sessionIdOrBasename; - if (sessionIdOrBasename.startsWith(SESSION_FILE_PREFIX)) { - const withoutExt = sessionIdOrBasename.replace(/\.jsonl?$/, ''); - const parts = withoutExt.split('-'); - shortId = parts[parts.length - 1]; - } else if (sessionIdOrBasename.length >= 8) { - shortId = sessionIdOrBasename.slice(0, 8); - } else { - throw new Error('Invalid sessionId or basename provided for deletion'); - } - - if (shortId.length !== 8) { - throw new Error('Derived shortId must be exactly 8 characters'); - } - - return shortId; - } - - private async getMatchingSessionFiles( - chatsDir: string, - shortId: string, - ): Promise { - const files = await fs.promises.readdir(chatsDir); - return files.filter( - (f) => - f.startsWith(SESSION_FILE_PREFIX) && - (f.endsWith(`-${shortId}.json`) || f.endsWith(`-${shortId}.jsonl`)), - ); - } - - /** - * Deletes a single session file and its associated logs, tool-outputs, and directory. - */ - private async deleteSessionAndArtifacts( - chatsDir: string, - file: string, - tempDir: string, - ): Promise { - const filePath = path.join(chatsDir, file); - let fullSessionId: string | undefined; - - try { - const CHUNK_SIZE = 4096; - const buffer = Buffer.alloc(CHUNK_SIZE); - let firstLine: string; - let fd: fs.promises.FileHandle | undefined; - try { - fd = await fs.promises.open(filePath, 'r'); - const { bytesRead } = await fd.read(buffer, 0, CHUNK_SIZE, 0); - if (bytesRead > 0) { - const contentChunk = buffer.toString('utf8', 0, bytesRead); - const newlineIndex = contentChunk.indexOf('\n'); - firstLine = - newlineIndex !== -1 - ? contentChunk.substring(0, newlineIndex) - : contentChunk; - - try { - const content = JSON.parse(firstLine) as unknown; - if (isSessionIdRecord(content)) { - fullSessionId = content.sessionId; - } - } catch { - // If first line parse fails, it might be a legacy pretty-printed JSON. - // We'll fall back to full file read below. - } - } - } finally { - if (fd !== undefined) { - await fd.close(); - } - } - - // Fallback for legacy JSON files if we couldn't get sessionId from first line - if (!fullSessionId) { - try { - const fileContent = await fs.promises.readFile(filePath, 'utf8'); - const parsed = JSON.parse(fileContent) as unknown; - if (isSessionIdRecord(parsed)) { - fullSessionId = parsed.sessionId; - } - } catch { - // Ignore parse errors, we'll still try to unlink the file - } - } - - if (fullSessionId) { - // Delegate to shared utility! - await deleteSessionArtifactsAsync(fullSessionId, tempDir); - await deleteSubagentSessionDirAndArtifactsAsync( - fullSessionId, - chatsDir, - tempDir, - ); - } - } catch (error) { - debugLogger.error( - `Error deleting artifacts for session file ${file}:`, - error, - ); - } finally { - // ALWAYS try to delete the session file itself - try { - await fs.promises.unlink(filePath); - } catch (error) { - if (isNodeError(error) && error.code !== 'ENOENT') { - debugLogger.error(`Error unlinking session file ${file}:`, error); - } - } - } + return deleteStoredSession(this.context.config, sessionIdOrBasename); } /** diff --git a/packages/core/src/utils/sessionOperations.ts b/packages/core/src/utils/sessionOperations.ts index 8a6da85d8e..a6ba6bdda9 100644 --- a/packages/core/src/utils/sessionOperations.ts +++ b/packages/core/src/utils/sessionOperations.ts @@ -8,9 +8,35 @@ import * as fs from 'node:fs/promises'; import path from 'node:path'; import { sanitizeFilenamePart } from './fileUtils.js'; import { debugLogger } from './debugLogger.js'; +import { isNodeError } from './errors.js'; +import type { Config } from '../config/config.js'; +import { SESSION_FILE_PREFIX } from '../services/chatRecordingTypes.js'; const LOGS_DIR = 'logs'; const TOOL_OUTPUTS_DIR = 'tool-outputs'; +const CHATS_DIR = 'chats'; + +/** + * Reserved directory names that must never be treated as a session id. A + * crafted JSONL session file whose first record had `sessionId: "chats"` + * (or any other reserved name) could otherwise resolve to the project's + * top-level chats/logs/tool-outputs directory and have it deleted by + * `deleteSessionArtifactsAsync`. + */ +const RESERVED_SESSION_DIR_NAMES: ReadonlySet = new Set([ + CHATS_DIR, + LOGS_DIR, + TOOL_OUTPUTS_DIR, +]); + +function isSessionIdRecord(record: unknown): record is { sessionId: string } { + return ( + record !== null && + typeof record === 'object' && + 'sessionId' in record && + typeof (record as { sessionId: unknown }).sessionId === 'string' + ); +} /** * Validates a sessionId and returns a sanitized version. @@ -57,13 +83,31 @@ export async function deleteSessionArtifactsAsync( if (err.code !== 'ENOENT') throw err; }); - // Top-level session directory (e.g., tempDir/safeSessionId) - const sessionDir = path.join(tempDir, safeSessionId); - await fs - .rm(sessionDir, { recursive: true, force: true }) - .catch((err: NodeJS.ErrnoException) => { - if (err.code !== 'ENOENT') throw err; - }); + // Top-level session directory (e.g., tempDir/safeSessionId). Reserved + // directory names (chats, logs, tool-outputs) are skipped here to prevent + // a crafted session file from causing one of the project's top-level + // temp directories to be deleted. Case-insensitive because macOS and + // Windows resolve `Chats` and `chats` to the same path. + // + // `safeSessionId` should never contain path separators (sanitizeFilenamePart + // replaces every non-`[a-zA-Z0-9_-]` character with `_`), but we re-assert + // it here so this deletion path is internally defended against future + // changes to the sanitizer. + const hasSeparator = + safeSessionId.includes(path.sep) || + safeSessionId.includes('/') || + safeSessionId.includes('\\'); + if ( + !hasSeparator && + !RESERVED_SESSION_DIR_NAMES.has(safeSessionId.toLowerCase()) + ) { + const sessionDir = path.join(tempDir, safeSessionId); + await fs + .rm(sessionDir, { recursive: true, force: true }) + .catch((err: NodeJS.ErrnoException) => { + if (err.code !== 'ENOENT') throw err; + }); + } } catch (error) { debugLogger.error( `Error deleting session artifacts for ${sessionId}:`, @@ -123,3 +167,159 @@ export async function deleteSubagentSessionDirAndArtifactsAsync( await fs.rm(subagentDir, { recursive: true, force: true }).catch(() => {}); } } + +/** + * Derives an 8-character short id from either a raw session id or a stored + * session file basename. Throws if the input cannot produce a valid short id. + */ +export function deriveSessionShortId(sessionIdOrBasename: string): string { + let shortId = sessionIdOrBasename; + if (sessionIdOrBasename.startsWith(SESSION_FILE_PREFIX)) { + const withoutExt = sessionIdOrBasename.replace(/\.jsonl?$/, ''); + const parts = withoutExt.split('-'); + shortId = parts[parts.length - 1]; + } else if (sessionIdOrBasename.length >= 8) { + shortId = sessionIdOrBasename.slice(0, 8); + } else { + throw new Error('Invalid sessionId or basename provided for deletion'); + } + + if (shortId.length !== 8) { + throw new Error('Derived shortId must be exactly 8 characters'); + } + + return shortId; +} + +/** + * Returns the list of stored session file names in `chatsDir` whose suffix + * matches the given 8-character short id. + */ +export async function getMatchingSessionFiles( + chatsDir: string, + shortId: string, +): Promise { + const files = await fs.readdir(chatsDir); + return files.filter( + (f) => + f.startsWith(SESSION_FILE_PREFIX) && + (f.endsWith(`-${shortId}.json`) || f.endsWith(`-${shortId}.jsonl`)), + ); +} + +/** + * Deletes a single session file and its associated logs, tool outputs, and + * any subagent directory. Reads the file's first line (or full body as a + * fallback) to recover the full session id required for artifact cleanup. + */ +export async function deleteSessionFileAndArtifacts( + chatsDir: string, + file: string, + tempDir: string, +): Promise { + const filePath = path.join(chatsDir, file); + let fullSessionId: string | undefined; + + try { + const CHUNK_SIZE = 4096; + const buffer = Buffer.alloc(CHUNK_SIZE); + let firstLine: string; + let fd: fs.FileHandle | undefined; + try { + fd = await fs.open(filePath, 'r'); + const { bytesRead } = await fd.read(buffer, 0, CHUNK_SIZE, 0); + if (bytesRead > 0) { + const contentChunk = buffer.toString('utf8', 0, bytesRead); + const newlineIndex = contentChunk.indexOf('\n'); + firstLine = + newlineIndex !== -1 + ? contentChunk.substring(0, newlineIndex) + : contentChunk; + + try { + const content = JSON.parse(firstLine) as unknown; + if (isSessionIdRecord(content)) { + fullSessionId = content.sessionId; + } + } catch { + // First line wasn't a parseable JSON record; fall back to full read. + } + } + } finally { + if (fd !== undefined) { + await fd.close(); + } + } + + if (!fullSessionId) { + try { + const fileContent = await fs.readFile(filePath, 'utf8'); + const parsed = JSON.parse(fileContent) as unknown; + if (isSessionIdRecord(parsed)) { + fullSessionId = parsed.sessionId; + } + } catch { + // Ignore parse errors, we'll still try to unlink the file below. + } + } + + if (fullSessionId) { + await deleteSessionArtifactsAsync(fullSessionId, tempDir); + await deleteSubagentSessionDirAndArtifactsAsync( + fullSessionId, + chatsDir, + tempDir, + ); + } + } catch (error) { + // ENOENT here is most likely a concurrent deletion race (another caller + // unlinked the file between `getMatchingSessionFiles` returning it and + // our `fs.open`). Don't log that as an error to avoid noise. + if (!isNodeError(error) || error.code !== 'ENOENT') { + debugLogger.error( + `Error deleting artifacts for session file ${file}:`, + error, + ); + } + } finally { + try { + await fs.unlink(filePath); + } catch (error) { + if (isNodeError(error) && error.code !== 'ENOENT') { + debugLogger.error(`Error unlinking session file ${file}:`, error); + } + } + } +} + +/** + * Deletes a stored chat session and all of its on-disk artifacts. Accepts + * either a raw session id (UUID-style) or a stored session file basename. + * + * This is the storage-only counterpart to `ChatRecordingService.deleteSession` + * and can be called from contexts that only have a `Config` (e.g. the CLI's + * one-shot session-delete utility) without constructing an `AgentLoopContext`. + */ +export async function deleteStoredSession( + config: Config, + sessionIdOrBasename: string, +): Promise { + try { + const tempDir = config.storage.getProjectTempDir(); + const chatsDir = path.join(tempDir, 'chats'); + const shortId = deriveSessionShortId(sessionIdOrBasename); + + const chatsDirStat = await fs.stat(chatsDir).catch(() => null); + if (!chatsDirStat || !chatsDirStat.isDirectory()) { + return; + } + + const matchingFiles = await getMatchingSessionFiles(chatsDir, shortId); + for (const file of matchingFiles) { + await deleteSessionFileAndArtifacts(chatsDir, file, tempDir); + } + } catch (error) { + debugLogger.error('Error deleting session file.', error); + throw error; + } +}