fix(#5340): unable to load chats with weird characters (#5969)

3 files changed, 232 insertions, 43 deletions

diff --git a/packages/cli/src/ui/commands/chatCommand.ts b/packages/cli/src/ui/commands/chatCommand.ts
index 14714df3..1c9029a9 100644
--- a/packages/cli/src/ui/commands/chatCommand.ts
+++ b/packages/cli/src/ui/commands/chatCommand.ts
@@ -15,6 +15,7 @@ import {
   CommandKind,
   SlashCommandActionReturn,
 } from './types.js';
+import { decodeTagName } from '@google/gemini-cli-core';
 import path from 'path';
 import { HistoryItemWithoutId, MessageType } from '../types.js';
 
@@ -41,8 +42,9 @@ const getSavedChatTags = async (
       if (file.startsWith(file_head) && file.endsWith(file_tail)) {
         const filePath = path.join(geminiDir, file);
         const stats = await fsPromises.stat(filePath);
+        const tagName = file.slice(file_head.length, -file_tail.length);
         chatDetails.push({
-          name: file.slice(file_head.length, -file_tail.length),
+          name: decodeTagName(tagName),
           mtime: stats.mtime,
         });
       }
@@ -147,7 +149,7 @@ const saveCommand: SlashCommand = {
       return {
         type: 'message',
         messageType: 'info',
-        content: `Conversation checkpoint saved with tag: ${tag}.`,
+        content: `Conversation checkpoint saved with tag: ${decodeTagName(tag)}.`,
       };
     } else {
       return {
@@ -183,7 +185,7 @@ const resumeCommand: SlashCommand = {
       return {
         type: 'message',
         messageType: 'info',
-        content: `No saved checkpoint found with tag: ${tag}.`,
+        content: `No saved checkpoint found with tag: ${decodeTagName(tag)}.`,
       };
     }
 
@@ -252,13 +254,13 @@ const deleteCommand: SlashCommand = {
       return {
         type: 'message',
         messageType: 'info',
-        content: `Conversation checkpoint '${tag}' has been deleted.`,
+        content: `Conversation checkpoint '${decodeTagName(tag)}' has been deleted.`,
       };
     } else {
       return {
         type: 'message',
         messageType: 'error',
-        content: `Error: No checkpoint found with tag '${tag}'.`,
+        content: `Error: No checkpoint found with tag '${decodeTagName(tag)}'.`,
       };
     }
   },
diff --git a/packages/core/src/core/logger.test.ts b/packages/core/src/core/logger.test.ts
index d032e2d4..0ad1cea9 100644
--- a/packages/core/src/core/logger.test.ts
+++ b/packages/core/src/core/logger.test.ts
@@ -13,8 +13,14 @@ import {
   afterEach,
   afterAll,
 } from 'vitest';
-import { Logger, MessageSenderType, LogEntry } from './logger.js';
-import { promises as fs } from 'node:fs';
+import {
+  Logger,
+  MessageSenderType,
+  LogEntry,
+  encodeTagName,
+  decodeTagName,
+} from './logger.js';
+import { promises as fs, existsSync } from 'node:fs';
 import path from 'node:path';
 import { Content } from '@google/genai';
 
@@ -394,15 +400,28 @@ describe('Logger', () => {
     ];
 
     it.each([
-      { tag: 'test-tag', sanitizedTag: 'test-tag' },
-      { tag: 'invalid/?*!', sanitizedTag: 'invalid' },
-      { tag: '/?*!', sanitizedTag: 'default' },
-      { tag: '../../secret', sanitizedTag: 'secret' },
-    ])('should save a checkpoint', async ({ tag, sanitizedTag }) => {
+      {
+        tag: 'test-tag',
+        encodedTag: 'test-tag',
+      },
+      {
+        tag: '你好世界',
+        encodedTag: '%E4%BD%A0%E5%A5%BD%E4%B8%96%E7%95%8C',
+      },
+      {
+        tag: 'japanese-ひらがなひらがな形声',
+        encodedTag:
+          'japanese-%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E5%BD%A2%E5%A3%B0',
+      },
+      {
+        tag: '../../secret',
+        encodedTag: '..%2F..%2Fsecret',
+      },
+    ])('should save a checkpoint', async ({ tag, encodedTag }) => {
       await logger.saveCheckpoint(conversation, tag);
       const taggedFilePath = path.join(
         TEST_GEMINI_DIR,
-        `checkpoint-${sanitizedTag}.json`,
+        `checkpoint-${encodedTag}.json`,
       );
       const fileContent = await fs.readFile(taggedFilePath, 'utf-8');
       expect(JSON.parse(fileContent)).toEqual(conversation);
@@ -438,18 +457,31 @@ describe('Logger', () => {
     });
 
     it.each([
-      { tag: 'load-tag', sanitizedTag: 'load-tag' },
-      { tag: 'inv/load?*!', sanitizedTag: 'invload' },
-      { tag: '/?*!', sanitizedTag: 'default' },
-      { tag: '../../secret', sanitizedTag: 'secret' },
-    ])('should load from a checkpoint', async ({ tag, sanitizedTag }) => {
+      {
+        tag: 'test-tag',
+        encodedTag: 'test-tag',
+      },
+      {
+        tag: '你好世界',
+        encodedTag: '%E4%BD%A0%E5%A5%BD%E4%B8%96%E7%95%8C',
+      },
+      {
+        tag: 'japanese-ひらがなひらがな形声',
+        encodedTag:
+          'japanese-%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E5%BD%A2%E5%A3%B0',
+      },
+      {
+        tag: '../../secret',
+        encodedTag: '..%2F..%2Fsecret',
+      },
+    ])('should load from a checkpoint', async ({ tag, encodedTag }) => {
       const taggedConversation = [
         ...conversation,
         { role: 'user', parts: [{ text: 'hello' }] },
       ];
       const taggedFilePath = path.join(
         TEST_GEMINI_DIR,
-        `checkpoint-${sanitizedTag}.json`,
+        `checkpoint-${encodedTag}.json`,
       );
       await fs.writeFile(
         taggedFilePath,
@@ -458,6 +490,8 @@ describe('Logger', () => {
 
       const loaded = await logger.loadCheckpoint(tag);
       expect(loaded).toEqual(taggedConversation);
+      expect(encodeTagName(tag)).toBe(encodedTag);
+      expect(decodeTagName(encodedTag)).toBe(tag);
     });
 
     it('should return an empty array if a tagged checkpoint file does not exist', async () => {
@@ -473,9 +507,10 @@ describe('Logger', () => {
 
     it('should return an empty array if the file contains invalid JSON', async () => {
       const tag = 'invalid-json-tag';
+      const encodedTag = 'invalid-json-tag';
       const taggedFilePath = path.join(
         TEST_GEMINI_DIR,
-        `checkpoint-${tag}.json`,
+        `checkpoint-${encodedTag}.json`,
       );
       await fs.writeFile(taggedFilePath, 'invalid json');
       const consoleErrorSpy = vi
@@ -508,12 +543,13 @@ describe('Logger', () => {
       { role: 'user', parts: [{ text: 'Content to be deleted' }] },
     ];
     const tag = 'delete-me';
+    const encodedTag = 'delete-me';
     let taggedFilePath: string;
 
     beforeEach(async () => {
       taggedFilePath = path.join(
         TEST_GEMINI_DIR,
-        `${CHECKPOINT_FILE_NAME.replace('.json', '')}-${tag}.json`,
+        `checkpoint-${encodedTag}.json`,
       );
       // Create a file to be deleted
       await fs.writeFile(taggedFilePath, JSON.stringify(conversation));
@@ -527,6 +563,30 @@ describe('Logger', () => {
       await expect(fs.access(taggedFilePath)).rejects.toThrow(/ENOENT/);
     });
 
+    it('should delete both new and old checkpoint files if they exist', async () => {
+      const oldTag = 'delete-me(old)';
+      const oldStylePath = path.join(
+        TEST_GEMINI_DIR,
+        `checkpoint-${oldTag}.json`,
+      );
+      const newStylePath = logger['_checkpointPath'](oldTag);
+
+      // Create both files
+      await fs.writeFile(oldStylePath, '{}');
+      await fs.writeFile(newStylePath, '{}');
+
+      // Verify both files exist before deletion
+      expect(existsSync(oldStylePath)).toBe(true);
+      expect(existsSync(newStylePath)).toBe(true);
+
+      const result = await logger.deleteCheckpoint(oldTag);
+      expect(result).toBe(true);
+
+      // Verify both are gone
+      expect(existsSync(oldStylePath)).toBe(false);
+      expect(existsSync(newStylePath)).toBe(false);
+    });
+
     it('should return false if the checkpoint file does not exist', async () => {
       const result = await logger.deleteCheckpoint('non-existent-tag');
       expect(result).toBe(false);
@@ -535,7 +595,9 @@ describe('Logger', () => {
     it('should re-throw an error if file deletion fails for reasons other than not existing', async () => {
       // Simulate a different error (e.g., permission denied)
       vi.spyOn(fs, 'unlink').mockRejectedValueOnce(
-        new Error('EACCES: permission denied'),
+        Object.assign(new Error('EACCES: permission denied'), {
+          code: 'EACCES',
+        }),
       );
       const consoleErrorSpy = vi
         .spyOn(console, 'error')
@@ -567,10 +629,14 @@ describe('Logger', () => {
 
   describe('checkpointExists', () => {
     const tag = 'exists-test';
+    const encodedTag = 'exists-test';
     let taggedFilePath: string;
 
     beforeEach(() => {
-      taggedFilePath = path.join(TEST_GEMINI_DIR, `checkpoint-${tag}.json`);
+      taggedFilePath = path.join(
+        TEST_GEMINI_DIR,
+        `checkpoint-${encodedTag}.json`,
+      );
     });
 
     it('should return true if the checkpoint file exists', async () => {
@@ -595,7 +661,9 @@ describe('Logger', () => {
 
     it('should re-throw an error if fs.access fails for reasons other than not existing', async () => {
       vi.spyOn(fs, 'access').mockRejectedValueOnce(
-        new Error('EACCES: permission denied'),
+        Object.assign(new Error('EACCES: permission denied'), {
+          code: 'EACCES',
+        }),
       );
       const consoleErrorSpy = vi
         .spyOn(console, 'error')
@@ -605,12 +673,37 @@ describe('Logger', () => {
         'EACCES: permission denied',
       );
       expect(consoleErrorSpy).toHaveBeenCalledWith(
-        `Failed to check checkpoint existence for ${taggedFilePath}:`,
+        `Failed to check checkpoint existence for path for tag "${tag}":`,
         expect.any(Error),
       );
     });
   });
 
+  describe('Backward compatibility', () => {
+    const conversation: Content[] = [
+      { role: 'user', parts: [{ text: 'Hello' }] },
+      { role: 'model', parts: [{ text: 'Hi there' }] },
+    ];
+    it('should load from a checkpoint with a raw special character tag', async () => {
+      const taggedConversation = [
+        ...conversation,
+        { role: 'user', parts: [{ text: 'hello' }] },
+      ];
+      const tag = 'special(char)';
+      const taggedFilePath = path.join(
+        TEST_GEMINI_DIR,
+        `checkpoint-${tag}.json`,
+      );
+      await fs.writeFile(
+        taggedFilePath,
+        JSON.stringify(taggedConversation, null, 2),
+      );
+
+      const loaded = await logger.loadCheckpoint(tag);
+      expect(loaded).toEqual(taggedConversation);
+    });
+  });
+
   describe('close', () => {
     it('should reset logger state', async () => {
       await logger.logMessage(MessageSenderType.USER, 'A message');
diff --git a/packages/core/src/core/logger.ts b/packages/core/src/core/logger.ts
index f4857f47..57b5bdf2 100644
--- a/packages/core/src/core/logger.ts
+++ b/packages/core/src/core/logger.ts
@@ -23,6 +23,42 @@ export interface LogEntry {
   message: string;
 }
 
+// This regex matches any character that is NOT a letter (a-z, A-Z),
+// a number (0-9), a hyphen (-), an underscore (_), or a dot (.).
+
+/**
+ * Encodes a string to be safe for use as a filename.
+ *
+ * It replaces any characters that are not alphanumeric or one of `_`, `-`, `.`
+ * with a URL-like percent-encoding (`%` followed by the 2-digit hex code).
+ *
+ * @param str The input string to encode.
+ * @returns The encoded, filename-safe string.
+ */
+export function encodeTagName(str: string): string {
+  return encodeURIComponent(str);
+}
+
+/**
+ * Decodes a string that was encoded with the `encode` function.
+ *
+ * It finds any percent-encoded characters and converts them back to their
+ * original representation.
+ *
+ * @param str The encoded string to decode.
+ * @returns The decoded, original string.
+ */
+export function decodeTagName(str: string): string {
+  try {
+    return decodeURIComponent(str);
+  } catch (_e) {
+    // Fallback for old, potentially malformed encoding
+    return str.replace(/%([0-9A-F]{2})/g, (_, hex) =>
+      String.fromCharCode(parseInt(hex, 16)),
+    );
+  }
+}
+
 export class Logger {
   private geminiDir: string | undefined;
   private logFilePath: string | undefined;
@@ -231,19 +267,46 @@ export class Logger {
     }
   }
 
-  _checkpointPath(tag: string): string {
+  private _checkpointPath(tag: string): string {
     if (!tag.length) {
       throw new Error('No checkpoint tag specified.');
     }
     if (!this.geminiDir) {
       throw new Error('Checkpoint file path not set.');
     }
-    // Sanitize tag to prevent directory traversal attacks
-    let sanitizedTag = tag.replace(/[^a-zA-Z0-9-_]/g, '');
-    if (!sanitizedTag) {
-      sanitizedTag = 'default';
+    // Encode the tag to handle all special characters safely.
+    const encodedTag = encodeTagName(tag);
+    return path.join(this.geminiDir, `checkpoint-${encodedTag}.json`);
+  }
+
+  private async _getCheckpointPath(tag: string): Promise<string> {
+    // 1. Check for the new encoded path first.
+    const newPath = this._checkpointPath(tag);
+    try {
+      await fs.access(newPath);
+      return newPath; // Found it, use the new path.
+    } catch (error) {
+      const nodeError = error as NodeJS.ErrnoException;
+      if (nodeError.code !== 'ENOENT') {
+        throw error; // A real error occurred, rethrow it.
+      }
+      // It was not found, so we'll check the old path next.
+    }
+
+    // 2. Fallback for backward compatibility: check for the old raw path.
+    const oldPath = path.join(this.geminiDir!, `checkpoint-${tag}.json`);
+    try {
+      await fs.access(oldPath);
+      return oldPath; // Found it, use the old path.
+    } catch (error) {
+      const nodeError = error as NodeJS.ErrnoException;
+      if (nodeError.code !== 'ENOENT') {
+        throw error; // A real error occurred, rethrow it.
+      }
     }
-    return path.join(this.geminiDir, `checkpoint-${sanitizedTag}.json`);
+
+    // 3. If neither path exists, return the new encoded path as the canonical one.
+    return newPath;
   }
 
   async saveCheckpoint(conversation: Content[], tag: string): Promise<void> {
@@ -253,6 +316,7 @@ export class Logger {
       );
       return;
     }
+    // Always save with the new encoded path.
     const path = this._checkpointPath(tag);
     try {
       await fs.writeFile(path, JSON.stringify(conversation, null, 2), 'utf-8');
@@ -269,7 +333,7 @@ export class Logger {
       return [];
     }
 
-    const path = this._checkpointPath(tag);
+    const path = await this._getCheckpointPath(tag);
     try {
       const fileContent = await fs.readFile(path, 'utf-8');
       const parsedContent = JSON.parse(fileContent);
@@ -281,6 +345,11 @@ export class Logger {
       }
       return parsedContent as Content[];
     } catch (error) {
+      const nodeError = error as NodeJS.ErrnoException;
+      if (nodeError.code === 'ENOENT') {
+        // This is okay, it just means the checkpoint doesn't exist in either format.
+        return [];
+      }
       console.error(`Failed to read or parse checkpoint file ${path}:`, error);
       return [];
     }
@@ -294,20 +363,39 @@ export class Logger {
       return false;
     }
 
-    const path = this._checkpointPath(tag);
+    let deletedSomething = false;
 
+    // 1. Attempt to delete the new encoded path.
+    const newPath = this._checkpointPath(tag);
     try {
-      await fs.unlink(path);
-      return true;
+      await fs.unlink(newPath);
+      deletedSomething = true;
     } catch (error) {
       const nodeError = error as NodeJS.ErrnoException;
-      if (nodeError.code === 'ENOENT') {
-        // File doesn't exist, which is fine.
-        return false;
+      if (nodeError.code !== 'ENOENT') {
+        console.error(`Failed to delete checkpoint file ${newPath}:`, error);
+        throw error; // Rethrow unexpected errors
       }
-      console.error(`Failed to delete checkpoint file ${path}:`, error);
-      throw error;
+      // It's okay if it doesn't exist.
     }
+
+    // 2. Attempt to delete the old raw path for backward compatibility.
+    const oldPath = path.join(this.geminiDir!, `checkpoint-${tag}.json`);
+    if (newPath !== oldPath) {
+      try {
+        await fs.unlink(oldPath);
+        deletedSomething = true;
+      } catch (error) {
+        const nodeError = error as NodeJS.ErrnoException;
+        if (nodeError.code !== 'ENOENT') {
+          console.error(`Failed to delete checkpoint file ${oldPath}:`, error);
+          throw error; // Rethrow unexpected errors
+        }
+        // It's okay if it doesn't exist.
+      }
+    }
+
+    return deletedSomething;
   }
 
   async checkpointExists(tag: string): Promise<boolean> {
@@ -316,17 +404,23 @@ export class Logger {
         'Logger not initialized. Cannot check for checkpoint existence.',
       );
     }
-    const filePath = this._checkpointPath(tag);
+    let filePath: string | undefined;
     try {
+      filePath = await this._getCheckpointPath(tag);
+      // We need to check for existence again, because _getCheckpointPath
+      // returns a canonical path even if it doesn't exist yet.
       await fs.access(filePath);
       return true;
     } catch (error) {
       const nodeError = error as NodeJS.ErrnoException;
       if (nodeError.code === 'ENOENT') {
-        return false;
+        return false; // It truly doesn't exist in either format.
       }
+      // A different error occurred.
       console.error(
-        `Failed to check checkpoint existence for ${filePath}:`,
+        `Failed to check checkpoint existence for ${
+          filePath ?? `path for tag "${tag}"`
+        }:`,
         error,
       );
       throw error;