From 73466b626db85c993a1b350d0d5ac9367c456c4a Mon Sep 17 00:00:00 2001 From: fuyou Date: Sat, 13 Sep 2025 14:05:43 +0800 Subject: [PATCH] Fix dollar sign replacement bug in file editing (#7871) Co-authored-by: Jacob Richman --- integration-tests/replace.test.ts | 30 ++++++++ packages/a2a-server/src/agent/task.ts | 5 +- packages/core/src/tools/edit.test.ts | 80 ++++++++++++++++++++++ packages/core/src/tools/edit.ts | 5 +- packages/core/src/tools/smart-edit.test.ts | 18 ++++- packages/core/src/tools/smart-edit.ts | 25 ++----- packages/core/src/utils/textUtils.test.ts | 79 +++++++++++++++++++++ packages/core/src/utils/textUtils.ts | 21 ++++++ 8 files changed, 239 insertions(+), 24 deletions(-) create mode 100644 packages/core/src/utils/textUtils.test.ts diff --git a/integration-tests/replace.test.ts b/integration-tests/replace.test.ts index e7ed23a2dc..0de23a04c7 100644 --- a/integration-tests/replace.test.ts +++ b/integration-tests/replace.test.ts @@ -61,4 +61,34 @@ describe('replace', () => { console.log('File replaced successfully. New content:', newFileContent); } }); + + it('should handle $ literally when replacing text ending with $', async () => { + const rig = new TestRig(); + await rig.setup( + 'should handle $ literally when replacing text ending with $', + ); + + const fileName = 'regex.yml'; + const originalContent = "| select('match', '^[sv]d[a-z]$')\n"; + const expectedContent = "| select('match', '^[sv]d[a-z]$') # updated\n"; + + rig.createFile(fileName, originalContent); + + const prompt = + "Open regex.yml and append ' # updated' after the line containing ^[sv]d[a-z]$ without breaking the $ character."; + + const result = await rig.run(prompt); + const foundToolCall = await rig.waitForToolCall('replace'); + + if (!foundToolCall) { + printDebugInfo(rig, result); + } + + expect(foundToolCall, 'Expected to find a replace tool call').toBeTruthy(); + + validateModelOutput(result, ['regex.yml'], 'Replace $ literal test'); + + const newFileContent = rig.readFile(fileName); + expect(newFileContent).toBe(expectedContent); + }); }); diff --git a/packages/a2a-server/src/agent/task.ts b/packages/a2a-server/src/agent/task.ts index e2aae8fece..b75205fa69 100644 --- a/packages/a2a-server/src/agent/task.ts +++ b/packages/a2a-server/src/agent/task.ts @@ -14,6 +14,7 @@ import { MCPServerStatus, isNodeError, parseAndFormatApiError, + safeLiteralReplace, } from '@google/gemini-cli-core'; import type { ToolConfirmationPayload, @@ -518,7 +519,9 @@ export class Task { if (oldString === '' && !isNewFile) { return currentContent; } - return currentContent.replaceAll(oldString, newString); + + // Use intelligent replacement that handles $ sequences safely + return safeLiteralReplace(currentContent, oldString, newString); } async scheduleToolCalls( diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 49ed12e5a6..cd5d559842 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -194,6 +194,86 @@ describe('EditTool', () => { 'hello world', ); }); + + it('should treat $ literally and not as replacement pattern', () => { + const current = "price is $100 and pattern end is ' '"; + const oldStr = 'price is $100'; + const newStr = 'price is $200'; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe("price is $200 and pattern end is ' '"); + }); + + it("should treat $' literally and not as a replacement pattern", () => { + const current = 'foo'; + const oldStr = 'foo'; + const newStr = "bar$'baz"; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe("bar$'baz"); + }); + + it('should treat $& literally and not as a replacement pattern', () => { + const current = 'hello world'; + const oldStr = 'hello'; + const newStr = '$&-replacement'; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe('$&-replacement world'); + }); + + it('should treat $` literally and not as a replacement pattern', () => { + const current = 'prefix-middle-suffix'; + const oldStr = 'middle'; + const newStr = 'new$`content'; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe('prefix-new$`content-suffix'); + }); + + it('should treat $1, $2 capture groups literally', () => { + const current = 'test string'; + const oldStr = 'test'; + const newStr = '$1$2replacement'; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe('$1$2replacement string'); + }); + + it('should use replaceAll for normal strings without problematic $ sequences', () => { + const current = 'normal text replacement'; + const oldStr = 'text'; + const newStr = 'string'; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe('normal string replacement'); + }); + + it('should handle multiple occurrences with problematic $ sequences', () => { + const current = 'foo bar foo baz'; + const oldStr = 'foo'; + const newStr = "test$'end"; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe("test$'end bar test$'end baz"); + }); + + it('should handle complex regex patterns with $ at end', () => { + const current = "| select('match', '^[sv]d[a-z]$')"; + const oldStr = "'^[sv]d[a-z]$'"; + const newStr = "'^[sv]d[a-z]$' # updated"; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe("| select('match', '^[sv]d[a-z]$' # updated)"); + }); + + it('should handle empty replacement with problematic $ in newString', () => { + const current = 'test content'; + const oldStr = 'nothing'; + const newStr = "replacement$'text"; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe('test content'); // No replacement because oldStr not found + }); + + it('should handle $$ (escaped dollar) correctly', () => { + const current = 'price value'; + const oldStr = 'value'; + const newStr = '$$100'; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe('price $$100'); + }); }); describe('validateToolParams', () => { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 2c17fe86f0..24f9332042 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -33,6 +33,7 @@ import type { ModifyContext, } from './modifiable-tool.js'; import { IdeClient } from '../ide/ide-client.js'; +import { safeLiteralReplace } from '../utils/textUtils.js'; export function applyReplacement( currentContent: string | null, @@ -51,7 +52,9 @@ export function applyReplacement( if (oldString === '' && !isNewFile) { return currentContent; } - return currentContent.replaceAll(oldString, newString); + + // Use intelligent replacement that handles $ sequences safely + return safeLiteralReplace(currentContent, oldString, newString); } /** diff --git a/packages/core/src/tools/smart-edit.test.ts b/packages/core/src/tools/smart-edit.test.ts index 9ce42c506e..7a60614293 100644 --- a/packages/core/src/tools/smart-edit.test.ts +++ b/packages/core/src/tools/smart-edit.test.ts @@ -42,11 +42,11 @@ import { type Mock, } from 'vitest'; import { - applyReplacement, SmartEditTool, type EditToolParams, calculateReplacement, } from './smart-edit.js'; +import { applyReplacement } from './edit.js'; import { type FileDiff, ToolConfirmationOutcome } from './tools.js'; import { ToolErrorType } from './tool-error.js'; import path from 'node:path'; @@ -172,6 +172,22 @@ describe('SmartEditTool', () => { 'hello new world new', ); }); + + it('should treat $ literally and not as replacement pattern', () => { + const current = 'regex end is $ and more'; + const oldStr = 'regex end is $'; + const newStr = 'regex end is $ and correct'; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe('regex end is $ and correct and more'); + }); + + it("should treat $' literally and not as a replacement pattern", () => { + const current = 'foo'; + const oldStr = 'foo'; + const newStr = "bar$'baz"; + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe("bar$'baz"); + }); }); describe('calculateReplacement', () => { diff --git a/packages/core/src/tools/smart-edit.ts b/packages/core/src/tools/smart-edit.ts index a39d906d2e..f1537958a2 100644 --- a/packages/core/src/tools/smart-edit.ts +++ b/packages/core/src/tools/smart-edit.ts @@ -30,26 +30,8 @@ import { } from './modifiable-tool.js'; import { IdeClient } from '../ide/ide-client.js'; import { FixLLMEditWithInstruction } from '../utils/llm-edit-fixer.js'; - -export function applyReplacement( - currentContent: string | null, - oldString: string, - newString: string, - isNewFile: boolean, -): string { - if (isNewFile) { - return newString; - } - if (currentContent === null) { - // Should not happen if not a new file, but defensively return empty or newString if oldString is also empty - return oldString === '' ? newString : ''; - } - // If oldString is empty and it's not a new file, do not modify the content. - if (oldString === '' && !isNewFile) { - return currentContent; - } - return currentContent.replaceAll(oldString, newString); -} +import { applyReplacement } from './edit.js'; +import { safeLiteralReplace } from '../utils/textUtils.js'; interface ReplacementContext { params: EditToolParams; @@ -89,7 +71,8 @@ async function calculateExactReplacement( const exactOccurrences = normalizedCode.split(normalizedSearch).length - 1; if (exactOccurrences > 0) { - let modifiedCode = normalizedCode.replaceAll( + let modifiedCode = safeLiteralReplace( + normalizedCode, normalizedSearch, normalizedReplace, ); diff --git a/packages/core/src/utils/textUtils.test.ts b/packages/core/src/utils/textUtils.test.ts new file mode 100644 index 0000000000..c1468c111b --- /dev/null +++ b/packages/core/src/utils/textUtils.test.ts @@ -0,0 +1,79 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { safeLiteralReplace } from './textUtils.js'; + +describe('safeLiteralReplace', () => { + it('returns original string when oldString empty or not found', () => { + expect(safeLiteralReplace('abc', '', 'X')).toBe('abc'); + expect(safeLiteralReplace('abc', 'z', 'X')).toBe('abc'); + }); + + it('fast path when newString has no $', () => { + expect(safeLiteralReplace('abc', 'b', 'X')).toBe('aXc'); + }); + + it('treats $ literally', () => { + expect(safeLiteralReplace('foo', 'foo', "bar$'baz")).toBe("bar$'baz"); + }); + + it("does not interpret replacement patterns like $&, $', $` and $1", () => { + expect(safeLiteralReplace('hello', 'hello', '$&-replacement')).toBe( + '$&-replacement', + ); + expect(safeLiteralReplace('mid', 'mid', 'new$`content')).toBe( + 'new$`content', + ); + expect(safeLiteralReplace('test', 'test', '$1$2value')).toBe('$1$2value'); + }); + + it('preserves end-of-line $ in regex-like text', () => { + const current = "| select('match', '^[sv]d[a-z]$')"; + const oldStr = "'^[sv]d[a-z]$'"; + const newStr = "'^[sv]d[a-z]$' # updated"; + const expected = "| select('match', '^[sv]d[a-z]$' # updated)"; + expect(safeLiteralReplace(current, oldStr, newStr)).toBe(expected); + }); + + it('handles multiple $ characters', () => { + expect(safeLiteralReplace('x', 'x', '$$$')).toBe('$$$'); + }); + + it('preserves pre-escaped $$ literally', () => { + expect(safeLiteralReplace('x', 'x', '$$value')).toBe('$$value'); + }); + + it('handles complex malicious patterns from PR #7871', () => { + const original = 'The price is PRICE.'; + const result = safeLiteralReplace( + original, + 'PRICE', + "$& Wow, that's a lot! $'", + ); + expect(result).toBe("The price is $& Wow, that's a lot! $'."); + }); + + it('handles multiple replacements correctly', () => { + const text = 'Replace FOO and FOO again'; + const result = safeLiteralReplace(text, 'FOO', '$100'); + expect(result).toBe('Replace $100 and $100 again'); + }); + + it('preserves $ at different positions', () => { + expect(safeLiteralReplace('test', 'test', '$')).toBe('$'); + expect(safeLiteralReplace('test', 'test', 'prefix$')).toBe('prefix$'); + expect(safeLiteralReplace('test', 'test', '$suffix')).toBe('$suffix'); + }); + + it('handles edge case with $$$$', () => { + expect(safeLiteralReplace('x', 'x', '$$$$')).toBe('$$$$'); + }); + + it('handles newString with only dollar signs', () => { + expect(safeLiteralReplace('abc', 'b', '$$')).toBe('a$$c'); + }); +}); diff --git a/packages/core/src/utils/textUtils.ts b/packages/core/src/utils/textUtils.ts index 3bcc475938..693ab48fe5 100644 --- a/packages/core/src/utils/textUtils.ts +++ b/packages/core/src/utils/textUtils.ts @@ -4,6 +4,27 @@ * SPDX-License-Identifier: Apache-2.0 */ +/** + * Safely replaces text with literal strings, avoiding ECMAScript GetSubstitution issues. + * Escapes $ characters to prevent template interpretation. + */ +export function safeLiteralReplace( + str: string, + oldString: string, + newString: string, +): string { + if (oldString === '' || !str.includes(oldString)) { + return str; + } + + if (!newString.includes('$')) { + return str.replaceAll(oldString, newString); + } + + const escapedNewString = newString.replaceAll('$', '$$$$'); + return str.replaceAll(oldString, escapedNewString); +} + /** * Checks if a Buffer is likely binary by testing for the presence of a NULL byte. * The presence of a NULL byte is a strong indicator that the data is not plain text.