fix(core): ensure tool output cleanup on session deletion for legacy files (#26263)

packages/cli/src/utils/sessionCleanup.test.ts

@@ -280,6 +280,100 @@ describe('Session Cleanup (Refactored)', () => {
       expect(existsSync(path.join(chatsDir, sessions[1].id))).toBe(false); // Subagent chats directory should be deleted
     });
 
+    it('should delete legacy pretty-printed session files and their artifacts', async () => {
+      const twoWeeksAgo = new Date(Date.now() - 14 * 24 * 60 * 60 * 1000);
+      const sessionId = 'legacy-uuid';
+      const shortId = 'legacy12';
+      const fileName = `session-20250110-${shortId}.json`;
+      const filePath = path.join(chatsDir, fileName);
+
+      // Write pretty-printed JSON
+      await fs.writeFile(
+        filePath,
+        JSON.stringify(
+          {
+            sessionId,
+            lastUpdated: twoWeeksAgo.toISOString(),
+            startTime: twoWeeksAgo.toISOString(),
+            messages: [{ type: 'user', content: 'hello legacy' }],
+          },
+          null,
+          2,
+        ),
+      );
+
+      await writeArtifacts(sessionId);
+
+      const config = createMockConfig();
+      const settings: Settings = {
+        general: { sessionRetention: { enabled: true, maxAge: '10d' } },
+      };
+
+      const result = await cleanupExpiredSessions(config, settings);
+
+      expect(result.deleted).toBe(1);
+      expect(existsSync(filePath)).toBe(false);
+      // Artifacts should be gone because we extracted sessionId from legacy JSON
+      expect(
+        existsSync(path.join(toolOutputsDir, `session-${sessionId}`)),
+      ).toBe(false);
+    });
+
+    it('should delete expired JSONL session files and their artifacts', async () => {
+      const twoWeeksAgo = new Date(Date.now() - 14 * 24 * 60 * 60 * 1000);
+      const sessionId = 'jsonl-uuid';
+      const shortId = 'jsonl123';
+      const fileName = `session-20250110-${shortId}.jsonl`;
+      const filePath = path.join(chatsDir, fileName);
+
+      // Write JSONL
+      const metadata = {
+        sessionId,
+        lastUpdated: twoWeeksAgo.toISOString(),
+        startTime: twoWeeksAgo.toISOString(),
+        kind: 'main',
+      };
+      const message = { id: '1', type: 'user', content: 'hello jsonl' };
+      await fs.writeFile(
+        filePath,
+        JSON.stringify(metadata) + '\n' + JSON.stringify(message) + '\n',
+      );
+
+      await writeArtifacts(sessionId);
+
+      const config = createMockConfig();
+      const settings: Settings = {
+        general: { sessionRetention: { enabled: true, maxAge: '10d' } },
+      };
+
+      const result = await cleanupExpiredSessions(config, settings);
+
+      expect(result.deleted).toBe(1);
+      expect(existsSync(filePath)).toBe(false);
+      // Artifacts should be gone because we extracted sessionId from JSONL first line
+      expect(
+        existsSync(path.join(toolOutputsDir, `session-${sessionId}`)),
+      ).toBe(false);
+    });
+
+    it('should delete corrupted session files even if sessionId cannot be extracted', async () => {
+      const shortId = 'corrupt1';
+      const fileName = `session-20250110-${shortId}.json`;
+      const filePath = path.join(chatsDir, fileName);
+
+      await fs.writeFile(filePath, 'completely invalid json');
+
+      const config = createMockConfig();
+      const settings: Settings = {
+        general: { sessionRetention: { enabled: true, maxAge: '10d' } },
+      };
+
+      const result = await cleanupExpiredSessions(config, settings);
+
+      expect(result.deleted).toBe(1);
+      expect(existsSync(filePath)).toBe(false);
+    });
+
     it('should NOT delete sessions within the cutoff date', async () => {
       const sessions = await seedSessions(); // [current, 14d, 30d]
       const config = createMockConfig();

packages/cli/src/utils/sessionCleanup.ts

@@ -30,10 +30,28 @@ const MULTIPLIERS = {
 };
 
 /**
- * Matches a trailing hyphen followed by exactly 8 alphanumeric characters before the .json extension.
+ * Matches a trailing hyphen followed by exactly 8 alphanumeric characters before the .json or .jsonl extension.
  * Example: session-20250110-abcdef12.json -> captures "abcdef12"
  */
-const SHORT_ID_REGEX = /-([a-zA-Z0-9]{8})\.json$/;
+const SHORT_ID_REGEX = /-([a-zA-Z0-9]{8})\.jsonl?$/;
+
+function hasProperty<T extends string>(
+  obj: unknown,
+  prop: T,
+): obj is { [key in T]: unknown } {
+  return obj !== null && typeof obj === 'object' && prop in obj;
+}
+
+function isStringProperty<T extends string>(
+  obj: unknown,
+  prop: T,
+): obj is { [key in T]: string } {
+  return hasProperty(obj, prop) && typeof obj[prop] === 'string';
+}
+
+function isSessionIdRecord(record: unknown): record is { sessionId: string } {
+  return isStringProperty(record, 'sessionId');
+}
 
 /**
  * Result of session cleanup operation
@@ -54,7 +72,10 @@ export interface CleanupResult {
  * Derives an 8-character shortId from a session filename.
  */
 function deriveShortIdFromFileName(fileName: string): string | null {
-  if (fileName.startsWith(SESSION_FILE_PREFIX) && fileName.endsWith('.json')) {
+  if (
+    fileName.startsWith(SESSION_FILE_PREFIX) &&
+    (fileName.endsWith('.json') || fileName.endsWith('.jsonl'))
+  ) {
     const match = fileName.match(SHORT_ID_REGEX);
     return match ? match[1] : null;
   }
@@ -141,7 +162,8 @@ export async function cleanupExpiredSessions(
             .filter(
               (f) =>
                 f.startsWith(SESSION_FILE_PREFIX) &&
-                f.endsWith(`-${shortId}.json`),
+                (f.endsWith(`-${shortId}.json`) ||
+                  f.endsWith(`-${shortId}.jsonl`)),
             );
 
           for (const file of matchingFiles) {
@@ -151,21 +173,44 @@ export async function cleanupExpiredSessions(
             try {
               // Try to read file to get full sessionId
               try {
-                const fileContent = await fs.readFile(filePath, 'utf8');
-                const content: unknown = JSON.parse(fileContent);
-                if (
-                  content &&
-                  typeof content === 'object' &&
-                  'sessionId' in content
-                ) {
-                  const record = content as Record<string, unknown>;
-                  const id = record['sessionId'];
-                  if (typeof id === 'string') {
-                    fullSessionId = id;
+                const CHUNK_SIZE = 4096;
+                const buffer = Buffer.alloc(CHUNK_SIZE);
+                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');
+                    const firstLine =
+                      newlineIndex !== -1
+                        ? contentChunk.substring(0, newlineIndex)
+                        : contentChunk;
+
+                    try {
+                      const record: unknown = JSON.parse(firstLine);
+                      if (isSessionIdRecord(record)) {
+                        fullSessionId = record.sessionId;
+                      }
+                    } catch {
+                      // Ignore first line parse error, try full parse for legacy pretty-printed JSON
+                    }
+                  }
+                } finally {
+                  if (fd !== undefined) {
+                    await fd.close();
+                  }
+                }
+
+                if (!fullSessionId) {
+                  const fileContent = await fs.readFile(filePath, 'utf8');
+                  const content: unknown = JSON.parse(fileContent);
+                  if (isSessionIdRecord(content)) {
+                    fullSessionId = content.sessionId;
                   }
                 }
               } catch {
-                // If read/parse fails, skip getting sessionId, just delete the file
+                // If read/parse fails, skip getting sessionId, just delete the file below
               }
 
               // Delete the session file

packages/core/src/services/chatRecordingService.test.ts

@@ -602,6 +602,52 @@ describe('ChatRecordingService', () => {
       expect(fs.existsSync(sessionDir)).toBe(false);
     });
 
+    it('should delete legacy pretty-printed session files and their artifacts', async () => {
+      const sessionId = 'legacy-uuid';
+      const shortId = 'legacy12';
+      const chatsDir = path.join(testTempDir, 'chats');
+      const toolOutputsDir = path.join(testTempDir, 'tool-outputs');
+
+      fs.mkdirSync(chatsDir, { recursive: true });
+      fs.mkdirSync(toolOutputsDir, { recursive: true });
+
+      const sessionFile = path.join(
+        chatsDir,
+        `session-2023-01-01T00-00-${shortId}.json`,
+      );
+      // Pretty-printed JSON (not JSONL)
+      fs.writeFileSync(
+        sessionFile,
+        JSON.stringify({ sessionId, messages: [] }, null, 2),
+      );
+
+      const toolOutputDir = path.join(toolOutputsDir, `session-${sessionId}`);
+      fs.mkdirSync(toolOutputDir, { recursive: true });
+      fs.writeFileSync(path.join(toolOutputDir, 'output.txt'), 'data');
+
+      await chatRecordingService.deleteSession(shortId);
+
+      expect(fs.existsSync(sessionFile)).toBe(false);
+      expect(fs.existsSync(toolOutputDir)).toBe(false);
+    });
+
+    it('should delete the session file even if it is corrupted (invalid JSON)', async () => {
+      const shortId = 'corrupt1';
+      const chatsDir = path.join(testTempDir, 'chats');
+
+      fs.mkdirSync(chatsDir, { recursive: true });
+
+      const sessionFile = path.join(
+        chatsDir,
+        `session-2023-01-01T00-00-${shortId}.jsonl`,
+      );
+      fs.writeFileSync(sessionFile, 'not-json');
+
+      await chatRecordingService.deleteSession(shortId);
+
+      expect(fs.existsSync(sessionFile)).toBe(false);
+    });
+
     it('should delete subagent files and their logs when parent is deleted', async () => {
       const parentSessionId = '12345678-session-id';
       const shortId = '12345678';

packages/core/src/services/chatRecordingService.ts

@@ -744,6 +744,8 @@ export class ChatRecordingService {
     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);
@@ -752,32 +754,43 @@ export class ChatRecordingService {
       try {
         fd = await fs.promises.open(filePath, 'r');
         const { bytesRead } = await fd.read(buffer, 0, CHUNK_SIZE, 0);
-        if (bytesRead === 0) {
-          await fd.close();
-          await fs.promises.unlink(filePath);
-          return;
+        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.
+          }
         }
-        const contentChunk = buffer.toString('utf8', 0, bytesRead);
-        const newlineIndex = contentChunk.indexOf('\n');
-        firstLine =
-          newlineIndex !== -1
-            ? contentChunk.substring(0, newlineIndex)
-            : contentChunk;
       } finally {
         if (fd !== undefined) {
           await fd.close();
         }
       }
-      const content = JSON.parse(firstLine) as unknown;
 
-      let fullSessionId: string | undefined;
-      if (isSessionIdRecord(content)) {
-        fullSessionId = content['sessionId'];
+      // 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
+        }
       }
 
-      // Delete the session file
-      await fs.promises.unlink(filePath);
-
       if (fullSessionId) {
         // Delegate to shared utility!
         await deleteSessionArtifactsAsync(fullSessionId, tempDir);
@@ -788,7 +801,19 @@ export class ChatRecordingService {
         );
       }
     } catch (error) {
-      debugLogger.error(`Error deleting associated file ${file}:`, 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);
+        }
+      }
     }
   }