jcarr
/
gemini-cli
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

This commit is contained in:
HugoMurillo 2025-08-14 17:24:57 -06:00 committed by GitHub
parent 1a41ba7daf
commit 8c0c8d7770
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 232 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
packages/cli/src/ui/commands/chatCommand.ts
View File

@ -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)}'.`,
};
}
},

133
packages/core/src/core/logger.test.ts
View File

@ -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');

130
packages/core/src/core/logger.ts
View File

@ -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`);
}
return path.join(this.geminiDir, `checkpoint-${sanitizedTag}.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.
}
}
// 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;