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

Address b/424256913 - fixed error in correctStringEscaping() and improved backslash handling (#1007)

This commit is contained in:
Bryan Morgan 2025-06-14 13:39:34 -04:00 committed by GitHub
parent 3bcb3c3666
commit 93909a2dd3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 219 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

186
packages/core/src/utils/editCorrector.test.ts
View File

@ -32,6 +32,7 @@ vi.mock('../core/client.js', () => ({
import {
countOccurrences,
ensureCorrectEdit,
ensureCorrectFileContent,
unescapeStringForGeminiBug,
resetEditCorrectorCaches_TEST_ONLY,
} from './editCorrector.js';
@ -102,7 +103,7 @@ describe('editCorrector', () => {
'First line\nSecond line',
);
});
it('should handle multiple backslashes before an escapable character', () => {
it('should handle multiple backslashes before an escapable character (aggressive unescaping)', () => {
expect(unescapeStringForGeminiBug('\\\\n')).toBe('\n');
expect(unescapeStringForGeminiBug('\\\\\\t')).toBe('\t');
expect(unescapeStringForGeminiBug('\\\\\\\\`')).toBe('`');
@ -126,6 +127,21 @@ describe('editCorrector', () => {
unescapeStringForGeminiBug('\\\\\\\nLine1\\\nLine2\\tTab\\\\`Tick\\"'),
).toBe('\nLine1\nLine2\tTab`Tick"');
});
it('should handle escaped backslashes', () => {
expect(unescapeStringForGeminiBug('\\\\')).toBe('\\');
expect(unescapeStringForGeminiBug('C:\\\\Users')).toBe('C:\\Users');
expect(unescapeStringForGeminiBug('path\\\\to\\\\file')).toBe(
'path\to\\file',
);
});
it('should handle escaped backslashes mixed with other escapes (aggressive unescaping)', () => {
expect(unescapeStringForGeminiBug('line1\\\\\\nline2')).toBe(
'line1\nline2',
);
expect(unescapeStringForGeminiBug('quote\\\\"text\\\\nline')).toBe(
'quote"text\nline',
);
});
});
describe('ensureCorrectEdit', () => {
@ -335,16 +351,13 @@ describe('editCorrector', () => {
old_string: 'find \\\\me',
new_string: 'replace with foobar',
};
mockResponses.push({
corrected_target_snippet: 'find \\me',
});
const result = await ensureCorrectEdit(
currentContent,
originalParams,
mockGeminiClientInstance,
abortSignal,
);
expect(mockGenerateJson).toHaveBeenCalledTimes(1);
expect(mockGenerateJson).toHaveBeenCalledTimes(0);
expect(result.params.new_string).toBe('replace with foobar');
expect(result.params.old_string).toBe('find \\me');
expect(result.occurrences).toBe(1);
@ -500,4 +513,167 @@ describe('editCorrector', () => {
});
});
});
describe('ensureCorrectFileContent', () => {
let mockGeminiClientInstance: Mocked<GeminiClient>;
let mockToolRegistry: Mocked<ToolRegistry>;
let mockConfigInstance: Config;
const abortSignal = new AbortController().signal;
beforeEach(() => {
mockToolRegistry = new ToolRegistry({} as Config) as Mocked<ToolRegistry>;
const configParams = {
apiKey: 'test-api-key',
model: 'test-model',
sandbox: false as boolean | string,
targetDir: '/test',
debugMode: false,
question: undefined as string | undefined,
fullContext: false,
coreTools: undefined as string[] | undefined,
toolDiscoveryCommand: undefined as string | undefined,
toolCallCommand: undefined as string | undefined,
mcpServerCommand: undefined as string | undefined,
mcpServers: undefined as Record<string, any> | undefined,
userAgent: 'test-agent',
userMemory: '',
geminiMdFileCount: 0,
alwaysSkipModificationConfirmation: false,
};
mockConfigInstance = {
...configParams,
getApiKey: vi.fn(() => configParams.apiKey),
getModel: vi.fn(() => configParams.model),
getSandbox: vi.fn(() => configParams.sandbox),
getTargetDir: vi.fn(() => configParams.targetDir),
getToolRegistry: vi.fn(() => mockToolRegistry),
getDebugMode: vi.fn(() => configParams.debugMode),
getQuestion: vi.fn(() => configParams.question),
getFullContext: vi.fn(() => configParams.fullContext),
getCoreTools: vi.fn(() => configParams.coreTools),
getToolDiscoveryCommand: vi.fn(() => configParams.toolDiscoveryCommand),
getToolCallCommand: vi.fn(() => configParams.toolCallCommand),
getMcpServerCommand: vi.fn(() => configParams.mcpServerCommand),
getMcpServers: vi.fn(() => configParams.mcpServers),
getUserAgent: vi.fn(() => configParams.userAgent),
getUserMemory: vi.fn(() => configParams.userMemory),
setUserMemory: vi.fn((mem: string) => {
configParams.userMemory = mem;
}),
getGeminiMdFileCount: vi.fn(() => configParams.geminiMdFileCount),
setGeminiMdFileCount: vi.fn((count: number) => {
configParams.geminiMdFileCount = count;
}),
getAlwaysSkipModificationConfirmation: vi.fn(
() => configParams.alwaysSkipModificationConfirmation,
),
setAlwaysSkipModificationConfirmation: vi.fn((skip: boolean) => {
configParams.alwaysSkipModificationConfirmation = skip;
}),
} as unknown as Config;
callCount = 0;
mockResponses.length = 0;
mockGenerateJson = vi
.fn()
.mockImplementation((_contents, _schema, signal) => {
if (signal && signal.aborted) {
return Promise.reject(new Error('Aborted'));
}
const response = mockResponses[callCount];
callCount++;
if (response === undefined) return Promise.resolve({});
return Promise.resolve(response);
});
mockStartChat = vi.fn();
mockSendMessageStream = vi.fn();
mockGeminiClientInstance = new GeminiClient(
mockConfigInstance,
) as Mocked<GeminiClient>;
resetEditCorrectorCaches_TEST_ONLY();
});
it('should return content unchanged if no escaping issues detected', async () => {
const content = 'This is normal content without escaping issues';
const result = await ensureCorrectFileContent(
content,
mockGeminiClientInstance,
abortSignal,
);
expect(result).toBe(content);
expect(mockGenerateJson).toHaveBeenCalledTimes(0);
});
it('should call correctStringEscaping for potentially escaped content', async () => {
const content = 'console.log(\\"Hello World\\");';
const correctedContent = 'console.log("Hello World");';
mockResponses.push({
corrected_string_escaping: correctedContent,
});
const result = await ensureCorrectFileContent(
content,
mockGeminiClientInstance,
abortSignal,
);
expect(result).toBe(correctedContent);
expect(mockGenerateJson).toHaveBeenCalledTimes(1);
});
it('should handle correctStringEscaping returning corrected content via correct property name', async () => {
// This test specifically verifies the property name fix
const content = 'const message = \\"Hello\\nWorld\\";';
const correctedContent = 'const message = "Hello\nWorld";';
// Mock the response with the correct property name
mockResponses.push({
corrected_string_escaping: correctedContent,
});
const result = await ensureCorrectFileContent(
content,
mockGeminiClientInstance,
abortSignal,
);
expect(result).toBe(correctedContent);
expect(mockGenerateJson).toHaveBeenCalledTimes(1);
});
it('should return original content if LLM correction fails', async () => {
const content = 'console.log(\\"Hello World\\");';
// Mock empty response to simulate LLM failure
mockResponses.push({});
const result = await ensureCorrectFileContent(
content,
mockGeminiClientInstance,
abortSignal,
);
expect(result).toBe(content);
expect(mockGenerateJson).toHaveBeenCalledTimes(1);
});
it('should handle various escape sequences that need correction', async () => {
const content =
'const obj = { name: \\"John\\", age: 30, bio: \\"Developer\\nEngineer\\" };';
const correctedContent =
'const obj = { name: "John", age: 30, bio: "Developer\nEngineer" };';
mockResponses.push({
corrected_string_escaping: correctedContent,
});
const result = await ensureCorrectFileContent(
content,
mockGeminiClientInstance,
abortSignal,
);
expect(result).toBe(correctedContent);
});
});
});

22
packages/core/src/utils/editCorrector.ts
View File

@ -511,10 +511,10 @@ Return ONLY the corrected string in the specified JSON format with the key 'corr
if (
result &&
typeof result.corrected_new_string_escaping === 'string' &&
result.corrected_new_string_escaping.length > 0
typeof result.corrected_string_escaping === 'string' &&
result.corrected_string_escaping.length > 0
) {
return result.corrected_new_string_escaping;
return result.corrected_string_escaping;
} else {
return potentiallyProblematicString;
}
@ -564,15 +564,18 @@ function trimPairIfPossible(
*/
export function unescapeStringForGeminiBug(inputString: string): string {
// Regex explanation:
// \\+ : Matches one or more literal backslash characters.
// (n|t|r|'|"|`|\n) : This is a capturing group. It matches one of the following:
// \\ : Matches exactly one literal backslash character.
// (n|t|r|'|"|`|\\|\n) : This is a capturing group. It matches one of the following:
// n, t, r, ', ", ` : These match the literal characters 'n', 't', 'r', single quote, double quote, or backtick.
// This handles cases like "\\n", "\\\\`", etc.
// This handles cases like "\\n", "\\`", etc.
// \\ : This matches a literal backslash. This handles cases like "\\\\" (escaped backslash).
// \n : This matches an actual newline character. This handles cases where the input
// string might have something like "\\\n" (a literal backslash followed by a newline).
// g : Global flag, to replace all occurrences.
return inputString.replace(/\\+(n|t|r|'|"|`|\n)/g, (match, capturedChar) => {
return inputString.replace(
/\\+(n|t|r|'|"|`|\\|\n)/g,
(match, capturedChar) => {
// 'match' is the entire erroneous sequence, e.g., if the input (in memory) was "\\\\`", match is "\\\\`".
// 'capturedChar' is the character that determines the true meaning, e.g., '`'.
@ -589,6 +592,8 @@ export function unescapeStringForGeminiBug(inputString: string): string {
return '"'; // Correctly escaped: " (quotation mark character)
case '`':
return '`'; // Correctly escaped: ` (backtick character)
case '\\': // This handles when 'capturedChar' is a literal backslash
return '\\'; // Replace escaped backslash (e.g., "\\\\") with single backslash
case '\n': // This handles when 'capturedChar' is an actual newline
return '\n'; // Replace the whole erroneous sequence (e.g., "\\\n" in memory) with a clean newline
default:
@ -596,7 +601,8 @@ export function unescapeStringForGeminiBug(inputString: string): string {
// It would return the original matched sequence if an unexpected character was captured.
return match;
}
});
},
);
}
/**