Fix dollar sign replacement bug in file editing (#7871)

Co-authored-by: Jacob Richman <jacob314@gmail.com>

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);
+  });
 });

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(

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', () => {

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);
 }
 
 /**

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', () => {

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,
     );

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');
+  });
+});

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.