refactor: decouple stored session deletion from ChatRecordingService (#22920) (#27039)

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<typeof SessionSelector>,
     );
-
-    // Mock ChatRecordingService
-    vi.mocked(ChatRecordingService).mockImplementation(
-      () =>
-        ({
-          deleteSession: mockDeleteSession,
-        }) as unknown as InstanceType<typeof ChatRecordingService>,
-    );
   });
 
   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'),
     );

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(

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<void> {
-    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<string[]> {
-    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<void> {
-    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);
   }
 
   /**

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<string> = 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<string[]> {
+  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<void> {
+  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<void> {
+  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;
+  }
+}