diff options
| author | Keith Ballinger <[email protected]> | 2025-06-06 22:54:37 -0700 |
|---|---|---|
| committer | GitHub <[email protected]> | 2025-06-06 22:54:37 -0700 |
| commit | 0c868746777e95255ce870aff4a61fb584d60a62 (patch) | |
| tree | 07fd51b91eee0df77d7014828308facaea03778f /packages/core/src/utils | |
| parent | 76ec9122c0dd36f0535a74c65811c0f7bd138f4d (diff) | |
Add batch editing capabilities to Edit Tool (#648)
Co-authored-by: N. Taylor Mullen <[email protected]>
Diffstat (limited to 'packages/core/src/utils')
| -rw-r--r-- | packages/core/src/utils/editCorrector.test.ts | 63 | ||||
| -rw-r--r-- | packages/core/src/utils/editCorrector.test.ts.backup | 503 | ||||
| -rw-r--r-- | packages/core/src/utils/editCorrector.ts | 38 | ||||
| -rw-r--r-- | packages/core/src/utils/positionBasedEditProcessor.ts | 239 |
4 files changed, 835 insertions, 8 deletions
diff --git a/packages/core/src/utils/editCorrector.test.ts b/packages/core/src/utils/editCorrector.test.ts index 7d6f5a53..53872253 100644 --- a/packages/core/src/utils/editCorrector.test.ts +++ b/packages/core/src/utils/editCorrector.test.ts @@ -214,6 +214,9 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to find me.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { old_string: 'find me', new_string: 'replace with \\"this\\"' }, + ], old_string: 'find me', new_string: 'replace with \\"this\\"', }; @@ -235,6 +238,7 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to find me.'; const originalParams = { file_path: '/test/file.txt', + edits: [{ old_string: 'find me', new_string: 'replace with this' }], old_string: 'find me', new_string: 'replace with this', }; @@ -253,6 +257,9 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to find\\me.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { old_string: 'find\\me', new_string: 'replace with \\"this\\"' }, + ], old_string: 'find\\me', new_string: 'replace with \\"this\\"', }; @@ -274,6 +281,7 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to find\\me.'; const originalParams = { file_path: '/test/file.txt', + edits: [{ old_string: 'find\\me', new_string: 'replace with this' }], old_string: 'find\\me', new_string: 'replace with this', }; @@ -295,6 +303,12 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to find "me".'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { + old_string: 'find \\"me\\"', + new_string: 'replace with \\"this\\"', + }, + ], old_string: 'find \\"me\\"', new_string: 'replace with \\"this\\"', }; @@ -314,6 +328,9 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to find "me".'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { old_string: 'find \\"me\\"', new_string: 'replace with this' }, + ], old_string: 'find \\"me\\"', new_string: 'replace with this', }; @@ -332,6 +349,9 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to find \\me.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { old_string: 'find \\\\me', new_string: 'replace with foobar' }, + ], old_string: 'find \\\\me', new_string: 'replace with foobar', }; @@ -356,6 +376,12 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to corrected find me.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { + old_string: 'find me', + new_string: 'replace with \\\\"this\\\\"', + }, + ], old_string: 'find me', new_string: 'replace with \\\\"this\\\\"', }; @@ -376,6 +402,12 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to corrected find me.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { + old_string: 'find\\me', + new_string: 'replace with \\\\"this\\\\"', + }, + ], old_string: 'find\\me', new_string: 'replace with \\\\"this\\\\"', }; @@ -398,6 +430,9 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to be corrected.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { old_string: 'fiiind me', new_string: 'replace with "this"' }, + ], old_string: 'fiiind me', new_string: 'replace with "this"', }; @@ -418,6 +453,12 @@ describe('editCorrector', () => { const currentContent = 'This is a test string to corrected find me.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { + old_string: 'find me', + new_string: 'replace with \\\\"this\\\\"', + }, + ], old_string: 'find me', new_string: 'replace with \\\\"this\\\\"', }; @@ -442,6 +483,9 @@ describe('editCorrector', () => { const currentContent = 'This content has nothing to find.'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { old_string: 'nonexistent string', new_string: 'some new string' }, + ], old_string: 'nonexistent string', new_string: 'some new string', }; @@ -453,7 +497,11 @@ describe('editCorrector', () => { abortSignal, ); expect(mockGenerateJson).toHaveBeenCalledTimes(1); - expect(result.params).toEqual(originalParams); + expect(result.params).toEqual({ + file_path: originalParams.file_path, + old_string: originalParams.old_string, + new_string: originalParams.new_string, + }); expect(result.occurrences).toBe(0); }); it('Test 4.2: unescapedOldStringAttempt results in >1 occurrences -> returns original params, count occurrences', async () => { @@ -461,6 +509,7 @@ describe('editCorrector', () => { 'This content has find "me" and also find "me" again.'; const originalParams = { file_path: '/test/file.txt', + edits: [{ old_string: 'find "me"', new_string: 'some new string' }], old_string: 'find "me"', new_string: 'some new string', }; @@ -471,7 +520,11 @@ describe('editCorrector', () => { abortSignal, ); expect(mockGenerateJson).toHaveBeenCalledTimes(0); - expect(result.params).toEqual(originalParams); + expect(result.params).toEqual({ + file_path: originalParams.file_path, + old_string: originalParams.old_string, + new_string: originalParams.new_string, + }); expect(result.occurrences).toBe(2); }); }); @@ -481,6 +534,12 @@ describe('editCorrector', () => { const currentContent = 'const x = "a\\nbc\\\\"def\\\\"'; const originalParams = { file_path: '/test/file.txt', + edits: [ + { + old_string: 'const x = \\\\"a\\\\nbc\\\\\\\\"def\\\\\\\\"', + new_string: 'const y = \\\\"new\\\\nval\\\\\\\\"content\\\\\\\\"', + }, + ], old_string: 'const x = \\\\"a\\\\nbc\\\\\\\\"def\\\\\\\\"', new_string: 'const y = \\\\"new\\\\nval\\\\\\\\"content\\\\\\\\"', }; diff --git a/packages/core/src/utils/editCorrector.test.ts.backup b/packages/core/src/utils/editCorrector.test.ts.backup new file mode 100644 index 00000000..7d6f5a53 --- /dev/null +++ b/packages/core/src/utils/editCorrector.test.ts.backup @@ -0,0 +1,503 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { vi, describe, it, expect, beforeEach, type Mocked } from 'vitest'; + +// MOCKS +let callCount = 0; +const mockResponses: any[] = []; + +let mockGenerateJson: any; +let mockStartChat: any; +let mockSendMessageStream: any; + +vi.mock('../core/client.js', () => ({ + GeminiClient: vi.fn().mockImplementation(function ( + this: any, + _config: Config, + ) { + this.generateJson = (...params: any[]) => mockGenerateJson(...params); // Corrected: use mockGenerateJson + this.startChat = (...params: any[]) => mockStartChat(...params); // Corrected: use mockStartChat + this.sendMessageStream = (...params: any[]) => + mockSendMessageStream(...params); // Corrected: use mockSendMessageStream + return this; + }), +})); +// END MOCKS + +import { + countOccurrences, + ensureCorrectEdit, + unescapeStringForGeminiBug, + resetEditCorrectorCaches_TEST_ONLY, +} from './editCorrector.js'; +import { GeminiClient } from '../core/client.js'; +import type { Config } from '../config/config.js'; +import { ToolRegistry } from '../tools/tool-registry.js'; + +vi.mock('../tools/tool-registry.js'); + +describe('editCorrector', () => { + describe('countOccurrences', () => { + it('should return 0 for empty string', () => { + expect(countOccurrences('', 'a')).toBe(0); + }); + it('should return 0 for empty substring', () => { + expect(countOccurrences('abc', '')).toBe(0); + }); + it('should return 0 if substring is not found', () => { + expect(countOccurrences('abc', 'd')).toBe(0); + }); + it('should return 1 if substring is found once', () => { + expect(countOccurrences('abc', 'b')).toBe(1); + }); + it('should return correct count for multiple occurrences', () => { + expect(countOccurrences('ababa', 'a')).toBe(3); + expect(countOccurrences('ababab', 'ab')).toBe(3); + }); + it('should count non-overlapping occurrences', () => { + expect(countOccurrences('aaaaa', 'aa')).toBe(2); + expect(countOccurrences('ababab', 'aba')).toBe(1); + }); + it('should correctly count occurrences when substring is longer', () => { + expect(countOccurrences('abc', 'abcdef')).toBe(0); + }); + it('should be case sensitive', () => { + expect(countOccurrences('abcABC', 'a')).toBe(1); + expect(countOccurrences('abcABC', 'A')).toBe(1); + }); + }); + + describe('unescapeStringForGeminiBug', () => { + it('should unescape common sequences', () => { + expect(unescapeStringForGeminiBug('\\n')).toBe('\n'); + expect(unescapeStringForGeminiBug('\\t')).toBe('\t'); + expect(unescapeStringForGeminiBug("\\'")).toBe("'"); + expect(unescapeStringForGeminiBug('\\"')).toBe('"'); + expect(unescapeStringForGeminiBug('\\`')).toBe('`'); + }); + it('should handle multiple escaped sequences', () => { + expect(unescapeStringForGeminiBug('Hello\\nWorld\\tTest')).toBe( + 'Hello\nWorld\tTest', + ); + }); + it('should not alter already correct sequences', () => { + expect(unescapeStringForGeminiBug('\n')).toBe('\n'); + expect(unescapeStringForGeminiBug('Correct string')).toBe( + 'Correct string', + ); + }); + it('should handle mixed correct and incorrect sequences', () => { + expect(unescapeStringForGeminiBug('\\nCorrect\t\\`')).toBe( + '\nCorrect\t`', + ); + }); + it('should handle backslash followed by actual newline character', () => { + expect(unescapeStringForGeminiBug('\\\n')).toBe('\n'); + expect(unescapeStringForGeminiBug('First line\\\nSecond line')).toBe( + 'First line\nSecond line', + ); + }); + it('should handle multiple backslashes before an escapable character', () => { + expect(unescapeStringForGeminiBug('\\\\n')).toBe('\n'); + expect(unescapeStringForGeminiBug('\\\\\\t')).toBe('\t'); + expect(unescapeStringForGeminiBug('\\\\\\\\`')).toBe('`'); + }); + it('should return empty string for empty input', () => { + expect(unescapeStringForGeminiBug('')).toBe(''); + }); + it('should not alter strings with no targeted escape sequences', () => { + expect(unescapeStringForGeminiBug('abc def')).toBe('abc def'); + expect(unescapeStringForGeminiBug('C:\\Folder\\File')).toBe( + 'C:\\Folder\\File', + ); + }); + it('should correctly process strings with some targeted escapes', () => { + expect(unescapeStringForGeminiBug('C:\\Users\\name')).toBe( + 'C:\\Users\name', + ); + }); + it('should handle complex cases with mixed slashes and characters', () => { + expect( + unescapeStringForGeminiBug('\\\\\\\nLine1\\\nLine2\\tTab\\\\`Tick\\"'), + ).toBe('\nLine1\nLine2\tTab`Tick"'); + }); + }); + + describe('ensureCorrectEdit', () => { + 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) => { + // Check if the signal is aborted. If so, throw an error or return a specific response. + if (signal && signal.aborted) { + return Promise.reject(new Error('Aborted')); // Or some other specific error/response + } + 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(); + }); + + describe('Scenario Group 1: originalParams.old_string matches currentContent directly', () => { + it('Test 1.1: old_string (no literal \\), new_string (escaped by Gemini) -> new_string unescaped', async () => { + const currentContent = 'This is a test string to find me.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find me', + new_string: 'replace with \\"this\\"', + }; + mockResponses.push({ + corrected_new_string_escaping: 'replace with "this"', + }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(1); + expect(result.params.new_string).toBe('replace with "this"'); + expect(result.params.old_string).toBe('find me'); + expect(result.occurrences).toBe(1); + }); + it('Test 1.2: old_string (no literal \\), new_string (correctly formatted) -> new_string unchanged', async () => { + const currentContent = 'This is a test string to find me.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find me', + new_string: 'replace with this', + }; + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(0); + expect(result.params.new_string).toBe('replace with this'); + expect(result.params.old_string).toBe('find me'); + expect(result.occurrences).toBe(1); + }); + it('Test 1.3: old_string (with literal \\), new_string (escaped by Gemini) -> new_string unchanged (still escaped)', async () => { + const currentContent = 'This is a test string to find\\me.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find\\me', + new_string: 'replace with \\"this\\"', + }; + mockResponses.push({ + corrected_new_string_escaping: 'replace with "this"', + }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(1); + expect(result.params.new_string).toBe('replace with "this"'); + expect(result.params.old_string).toBe('find\\me'); + expect(result.occurrences).toBe(1); + }); + it('Test 1.4: old_string (with literal \\), new_string (correctly formatted) -> new_string unchanged', async () => { + const currentContent = 'This is a test string to find\\me.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find\\me', + new_string: 'replace with this', + }; + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(0); + expect(result.params.new_string).toBe('replace with this'); + expect(result.params.old_string).toBe('find\\me'); + expect(result.occurrences).toBe(1); + }); + }); + + describe('Scenario Group 2: originalParams.old_string does NOT match, but unescapeStringForGeminiBug(originalParams.old_string) DOES match', () => { + it('Test 2.1: old_string (over-escaped, no intended literal \\), new_string (escaped by Gemini) -> new_string unescaped', async () => { + const currentContent = 'This is a test string to find "me".'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find \\"me\\"', + new_string: 'replace with \\"this\\"', + }; + mockResponses.push({ corrected_new_string: 'replace with "this"' }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(1); + expect(result.params.new_string).toBe('replace with "this"'); + expect(result.params.old_string).toBe('find "me"'); + expect(result.occurrences).toBe(1); + }); + it('Test 2.2: old_string (over-escaped, no intended literal \\), new_string (correctly formatted) -> new_string unescaped (harmlessly)', async () => { + const currentContent = 'This is a test string to find "me".'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find \\"me\\"', + new_string: 'replace with this', + }; + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(0); + expect(result.params.new_string).toBe('replace with this'); + expect(result.params.old_string).toBe('find "me"'); + expect(result.occurrences).toBe(1); + }); + it('Test 2.3: old_string (over-escaped, with intended literal \\), new_string (simple) -> new_string corrected', async () => { + const currentContent = 'This is a test string to find \\me.'; + const originalParams = { + file_path: '/test/file.txt', + 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(result.params.new_string).toBe('replace with foobar'); + expect(result.params.old_string).toBe('find \\me'); + expect(result.occurrences).toBe(1); + }); + }); + + describe('Scenario Group 3: LLM Correction Path', () => { + it('Test 3.1: old_string (no literal \\), new_string (escaped by Gemini), LLM re-escapes new_string -> final new_string is double unescaped', async () => { + const currentContent = 'This is a test string to corrected find me.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find me', + new_string: 'replace with \\\\"this\\\\"', + }; + const llmNewString = 'LLM says replace with "that"'; + mockResponses.push({ corrected_new_string_escaping: llmNewString }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(1); + expect(result.params.new_string).toBe(llmNewString); + expect(result.params.old_string).toBe('find me'); + expect(result.occurrences).toBe(1); + }); + it('Test 3.2: old_string (with literal \\), new_string (escaped by Gemini), LLM re-escapes new_string -> final new_string is unescaped once', async () => { + const currentContent = 'This is a test string to corrected find me.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find\\me', + new_string: 'replace with \\\\"this\\\\"', + }; + const llmCorrectedOldString = 'corrected find me'; + const llmNewString = 'LLM says replace with "that"'; + mockResponses.push({ corrected_target_snippet: llmCorrectedOldString }); + mockResponses.push({ corrected_new_string: llmNewString }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(2); + expect(result.params.new_string).toBe(llmNewString); + expect(result.params.old_string).toBe(llmCorrectedOldString); + expect(result.occurrences).toBe(1); + }); + it('Test 3.3: old_string needs LLM, new_string is fine -> old_string corrected, new_string original', async () => { + const currentContent = 'This is a test string to be corrected.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'fiiind me', + new_string: 'replace with "this"', + }; + const llmCorrectedOldString = 'to be corrected'; + mockResponses.push({ corrected_target_snippet: llmCorrectedOldString }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(1); + expect(result.params.new_string).toBe('replace with "this"'); + expect(result.params.old_string).toBe(llmCorrectedOldString); + expect(result.occurrences).toBe(1); + }); + it('Test 3.4: LLM correction path, correctNewString returns the originalNewString it was passed (which was unescaped) -> final new_string is unescaped', async () => { + const currentContent = 'This is a test string to corrected find me.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find me', + new_string: 'replace with \\\\"this\\\\"', + }; + const newStringForLLMAndReturnedByLLM = 'replace with "this"'; + mockResponses.push({ + corrected_new_string_escaping: newStringForLLMAndReturnedByLLM, + }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(1); + expect(result.params.new_string).toBe(newStringForLLMAndReturnedByLLM); + expect(result.occurrences).toBe(1); + }); + }); + + describe('Scenario Group 4: No Match Found / Multiple Matches', () => { + it('Test 4.1: No version of old_string (original, unescaped, LLM-corrected) matches -> returns original params, 0 occurrences', async () => { + const currentContent = 'This content has nothing to find.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'nonexistent string', + new_string: 'some new string', + }; + mockResponses.push({ corrected_target_snippet: 'still nonexistent' }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(1); + expect(result.params).toEqual(originalParams); + expect(result.occurrences).toBe(0); + }); + it('Test 4.2: unescapedOldStringAttempt results in >1 occurrences -> returns original params, count occurrences', async () => { + const currentContent = + 'This content has find "me" and also find "me" again.'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'find "me"', + new_string: 'some new string', + }; + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(0); + expect(result.params).toEqual(originalParams); + expect(result.occurrences).toBe(2); + }); + }); + + describe('Scenario Group 5: Specific unescapeStringForGeminiBug checks (integrated into ensureCorrectEdit)', () => { + it('Test 5.1: old_string needs LLM to become currentContent, new_string also needs correction', async () => { + const currentContent = 'const x = "a\\nbc\\\\"def\\\\"'; + const originalParams = { + file_path: '/test/file.txt', + old_string: 'const x = \\\\"a\\\\nbc\\\\\\\\"def\\\\\\\\"', + new_string: 'const y = \\\\"new\\\\nval\\\\\\\\"content\\\\\\\\"', + }; + const expectedFinalNewString = 'const y = "new\\nval\\\\"content\\\\"'; + mockResponses.push({ corrected_target_snippet: currentContent }); + mockResponses.push({ corrected_new_string: expectedFinalNewString }); + const result = await ensureCorrectEdit( + currentContent, + originalParams, + mockGeminiClientInstance, + abortSignal, + ); + expect(mockGenerateJson).toHaveBeenCalledTimes(2); + expect(result.params.old_string).toBe(currentContent); + expect(result.params.new_string).toBe(expectedFinalNewString); + expect(result.occurrences).toBe(1); + }); + }); + }); +}); diff --git a/packages/core/src/utils/editCorrector.ts b/packages/core/src/utils/editCorrector.ts index 67d23cde..c1d7be77 100644 --- a/packages/core/src/utils/editCorrector.ts +++ b/packages/core/src/utils/editCorrector.ts @@ -61,10 +61,16 @@ export interface CorrectedEditResult { */ export async function ensureCorrectEdit( currentContent: string, - originalParams: EditToolParams, // This is the EditToolParams from edit.ts, without \'corrected\' + originalParams: EditToolParams & { old_string: string; new_string: string }, client: GeminiClient, abortSignal: AbortSignal, ): Promise<CorrectedEditResult> { + // Ensure we have required old_string and new_string + if (!originalParams.old_string || !originalParams.new_string) { + throw new Error( + 'old_string and new_string are required for edit correction', + ); + } const cacheKey = `${currentContent}---${originalParams.old_string}---${originalParams.new_string}`; const cachedResult = editCorrectionCache.get(cacheKey); if (cachedResult) { @@ -96,7 +102,11 @@ export async function ensureCorrectEdit( // If user expects multiple replacements, return as-is if (occurrences === expectedReplacements) { const result: CorrectedEditResult = { - params: { ...originalParams }, + params: { + file_path: originalParams.file_path, + old_string: originalParams.old_string!, + new_string: originalParams.new_string!, + }, occurrences, }; editCorrectionCache.set(cacheKey, result); @@ -106,7 +116,11 @@ export async function ensureCorrectEdit( // If user expects 1 but found multiple, try to correct (existing behavior) if (expectedReplacements === 1) { const result: CorrectedEditResult = { - params: { ...originalParams }, + params: { + file_path: originalParams.file_path, + old_string: originalParams.old_string!, + new_string: originalParams.new_string!, + }, occurrences, }; editCorrectionCache.set(cacheKey, result); @@ -115,7 +129,11 @@ export async function ensureCorrectEdit( // If occurrences don't match expected, return as-is (will fail validation later) const result: CorrectedEditResult = { - params: { ...originalParams }, + params: { + file_path: originalParams.file_path, + old_string: finalOldString, + new_string: finalNewString, + }, occurrences, }; editCorrectionCache.set(cacheKey, result); @@ -169,7 +187,11 @@ export async function ensureCorrectEdit( } else { // LLM correction also failed for old_string const result: CorrectedEditResult = { - params: { ...originalParams }, + params: { + file_path: originalParams.file_path, + old_string: finalOldString, + new_string: finalNewString, + }, occurrences: 0, // Explicitly 0 as LLM failed }; editCorrectionCache.set(cacheKey, result); @@ -178,7 +200,11 @@ export async function ensureCorrectEdit( } else { // Unescaping old_string resulted in > 1 occurrences const result: CorrectedEditResult = { - params: { ...originalParams }, + params: { + file_path: originalParams.file_path, + old_string: finalOldString, + new_string: finalNewString, + }, occurrences, // This will be > 1 }; editCorrectionCache.set(cacheKey, result); diff --git a/packages/core/src/utils/positionBasedEditProcessor.ts b/packages/core/src/utils/positionBasedEditProcessor.ts new file mode 100644 index 00000000..977d6859 --- /dev/null +++ b/packages/core/src/utils/positionBasedEditProcessor.ts @@ -0,0 +1,239 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Single edit operation with position information + */ +export interface PositionedEdit { + /** + * Original edit operation + */ + original: EditOperation; + + /** + * Index in the original edits array + */ + index: number; + + /** + * Start position in the file content + */ + startPos: number; + + /** + * End position in the file content + */ + endPos: number; + + /** + * The replacement text + */ + newString: string; +} + +/** + * Result of failed edit with reason + */ +export interface FailedEdit { + index: number; + edit: EditOperation; + reason: 'not_found' | 'multiple_matches' | 'empty_old_string'; +} + +/** + * Result of position-based edit processing + */ +export interface PositionBasedEditResult { + finalContent: string; + appliedEdits: PositionedEdit[]; + failedEdits: FailedEdit[]; +} + +/** + * Edit operation interface (matches existing EditOperation) + */ +export interface EditOperation { + old_string: string; + new_string: string; +} + +/** + * Efficient position-based edit processor that minimizes memory usage + * and applies edits in optimal order to prevent position conflicts. + */ +export class PositionBasedEditProcessor { + /** + * Process edits with minimal memory usage and optimal ordering + */ + processEdits( + content: string, + edits: EditOperation[], + ): PositionBasedEditResult { + // Phase 1: Find positions for all edits + const { validEdits, failedEdits } = this.analyzeEdits(content, edits); + + // Phase 2: Sort by position (highest first) to prevent position drift + const sortedEdits = validEdits.sort((a, b) => b.startPos - a.startPos); + + // Phase 3: Build final content in single pass + const finalContent = this.buildFinalContent(content, sortedEdits); + + return { + finalContent, + appliedEdits: sortedEdits, + failedEdits, + }; + } + + /** + * Analyze all edits and categorize into valid/failed + */ + private analyzeEdits( + content: string, + edits: EditOperation[], + ): { + validEdits: PositionedEdit[]; + failedEdits: FailedEdit[]; + } { + const validEdits: PositionedEdit[] = []; + const failedEdits: FailedEdit[] = []; + + for (let i = 0; i < edits.length; i++) { + const edit = edits[i]; + + // Handle empty old_string (file creation case) + if (edit.old_string === '') { + failedEdits.push({ + index: i, + edit, + reason: 'empty_old_string', + }); + continue; + } + + // Find all positions of old_string + const positions = this.findAllPositions(content, edit.old_string); + + if (positions.length === 0) { + failedEdits.push({ + index: i, + edit, + reason: 'not_found', + }); + } else if (positions.length > 1) { + failedEdits.push({ + index: i, + edit, + reason: 'multiple_matches', + }); + } else { + // Exactly one match - valid edit + const startPos = positions[0]; + validEdits.push({ + original: edit, + index: i, + startPos, + endPos: startPos + edit.old_string.length, + newString: edit.new_string, + }); + } + } + + return { validEdits, failedEdits }; + } + + /** + * Find all positions where searchString occurs in content + */ + private findAllPositions(content: string, searchString: string): number[] { + const positions: number[] = []; + let index = content.indexOf(searchString); + + while (index !== -1) { + positions.push(index); + index = content.indexOf(searchString, index + 1); + } + + return positions; + } + + /** + * Build final content in single pass with minimal memory allocation + */ + private buildFinalContent(original: string, edits: PositionedEdit[]): string { + if (edits.length === 0) { + return original; + } + + // Check for overlapping edits (should not happen with our validation, but safety check) + if (this.hasOverlappingEdits(edits)) { + throw new Error('Overlapping edits detected - this should not happen'); + } + + // Build segments array working backwards through the string + const segments: string[] = []; + let currentPos = original.length; + + // Process edits from end to beginning (edits are already sorted by startPos desc) + for (const edit of edits) { + // Add text after this edit position + if (currentPos > edit.endPos) { + segments.unshift(original.slice(edit.endPos, currentPos)); + } + + // Add the replacement text + segments.unshift(edit.newString); + + // Update current position + currentPos = edit.startPos; + } + + // Add remaining text from beginning + if (currentPos > 0) { + segments.unshift(original.slice(0, currentPos)); + } + + // Single join operation creates final string + return segments.join(''); + } + + /** + * Check if any edits overlap (safety validation) + */ + private hasOverlappingEdits(edits: PositionedEdit[]): boolean { + for (let i = 0; i < edits.length - 1; i++) { + for (let j = i + 1; j < edits.length; j++) { + const edit1 = edits[i]; + const edit2 = edits[j]; + + // Check if ranges overlap + if ( + !(edit1.endPos <= edit2.startPos || edit2.endPos <= edit1.startPos) + ) { + return true; + } + } + } + + return false; + } + + /** + * Get human-readable error message for failed edit reason + */ + static getErrorMessage(reason: FailedEdit['reason']): string { + switch (reason) { + case 'not_found': + return 'Old string not found in current content'; + case 'multiple_matches': + return 'Old string found multiple times - please be more specific'; + case 'empty_old_string': + return 'Cannot use empty old_string on existing file'; + default: + return 'Unknown error'; + } + } +} |
