diff options
Diffstat (limited to 'packages/core/src/tools/edit.test.ts')
| -rw-r--r-- | packages/core/src/tools/edit.test.ts | 420 |
1 files changed, 153 insertions, 267 deletions
diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 2f6a6642..8c351929 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -194,7 +194,8 @@ describe('EditTool', () => { it('should return null for valid params', () => { const params: EditToolParams = { file_path: path.join(rootDir, 'test.txt'), - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; expect(tool.validateToolParams(params)).toBeNull(); }); @@ -202,7 +203,8 @@ describe('EditTool', () => { it('should return error for relative path', () => { const params: EditToolParams = { file_path: 'test.txt', - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; expect(tool.validateToolParams(params)).toMatch( /File path must be absolute/, @@ -212,7 +214,8 @@ describe('EditTool', () => { it('should return error for path outside root', () => { const params: EditToolParams = { file_path: path.join(tempDir, 'outside-root.txt'), - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; expect(tool.validateToolParams(params)).toMatch( /File path must be within the root directory/, @@ -231,7 +234,8 @@ describe('EditTool', () => { it('should return false if params are invalid', async () => { const params: EditToolParams = { file_path: 'relative.txt', - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; expect( await tool.shouldConfirmExecute(params, new AbortController().signal), @@ -242,7 +246,8 @@ describe('EditTool', () => { fs.writeFileSync(filePath, 'some old content here'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; // ensureCorrectEdit will be called by shouldConfirmExecute mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 1 }); @@ -263,48 +268,26 @@ describe('EditTool', () => { fs.writeFileSync(filePath, 'some content here'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'not_found', new_string: 'new' }], + old_string: 'not_found', + new_string: 'new', }; - mockEnsureCorrectEdit.mockResolvedValueOnce({ - params: { - file_path: filePath, - old_string: 'not_found', - new_string: 'new', - }, - occurrences: 0, - }); - - // Our new implementation shows confirmation but with no changes, - // which should still return false due to no edits applied - const result = await tool.shouldConfirmExecute( - params, - new AbortController().signal, - ); - // If no edits would be applied, confirmation should be false - expect(result).toBe(false); + mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); + expect( + await tool.shouldConfirmExecute(params, new AbortController().signal), + ).toBe(false); }); it('should return false if multiple occurrences of old_string are found (ensureCorrectEdit returns > 1)', async () => { fs.writeFileSync(filePath, 'old old content here'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; - mockEnsureCorrectEdit.mockResolvedValueOnce({ - params: { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }, - occurrences: 2, - }); - - // Multiple occurrences should result in failed edit, no confirmation - const result = await tool.shouldConfirmExecute( - params, - new AbortController().signal, - ); - expect(result).toBe(false); + mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 2 }); + expect( + await tool.shouldConfirmExecute(params, new AbortController().signal), + ).toBe(false); }); it('should request confirmation for creating a new file (empty old_string)', async () => { @@ -312,41 +295,87 @@ describe('EditTool', () => { const newFilePath = path.join(rootDir, newFileName); const params: EditToolParams = { file_path: newFilePath, - edits: [{ old_string: '', new_string: 'new file content' }], + old_string: '', + new_string: 'new file content', }; + // ensureCorrectEdit might not be called if old_string is empty, + // as shouldConfirmExecute handles this for diff generation. + // If it is called, it should return 0 occurrences for a new file. + mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); const confirmation = await tool.shouldConfirmExecute( params, new AbortController().signal, ); expect(confirmation).toEqual( expect.objectContaining({ - title: expect.stringContaining(newFileName), + title: `Confirm Edit: ${newFileName}`, fileName: newFileName, fileDiff: expect.any(String), }), ); }); - it('should not use AI correction and provide clear feedback for non-matching text', async () => { + it('should use corrected params from ensureCorrectEdit for diff generation', async () => { const originalContent = 'This is the original string to be replaced.'; - const nonMatchingOldString = 'completely different text'; // This won't match at all - const newString = 'new string'; + const originalOldString = 'original string'; + const originalNewString = 'new string'; + + const correctedOldString = 'original string to be replaced'; // More specific + const correctedNewString = 'completely new string'; // Different replacement + const expectedFinalContent = 'This is the completely new string.'; fs.writeFileSync(filePath, originalContent); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: nonMatchingOldString, new_string: newString }], + old_string: originalOldString, + new_string: originalNewString, }; - // With deterministic approach, this should return false (no confirmation) - // because the old_string doesn't match exactly - const confirmation = await tool.shouldConfirmExecute( + // The main beforeEach already calls mockEnsureCorrectEdit.mockReset() + // Set a specific mock for this test case + let mockCalled = false; + mockEnsureCorrectEdit.mockImplementationOnce( + async (content, p, client) => { + console.log('mockEnsureCorrectEdit CALLED IN TEST'); + mockCalled = true; + expect(content).toBe(originalContent); + expect(p).toBe(params); + expect(client).toBe((tool as any).client); + return { + params: { + file_path: filePath, + old_string: correctedOldString, + new_string: correctedNewString, + }, + occurrences: 1, + }; + }, + ); + + const confirmation = (await tool.shouldConfirmExecute( params, new AbortController().signal, + )) as FileDiff; + + expect(mockCalled).toBe(true); // Check if the mock implementation was run + // expect(mockEnsureCorrectEdit).toHaveBeenCalledWith(originalContent, params, expect.anything()); // Keep this commented for now + expect(confirmation).toEqual( + expect.objectContaining({ + title: `Confirm Edit: ${testFile}`, + fileName: testFile, + }), ); + // Check that the diff is based on the corrected strings leading to the new state + expect(confirmation.fileDiff).toContain(`-${originalContent}`); + expect(confirmation.fileDiff).toContain(`+${expectedFinalContent}`); - // Should return false because edit will fail (no exact match) - expect(confirmation).toBe(false); + // Verify that applying the correctedOldString and correctedNewString to originalContent + // indeed produces the expectedFinalContent, which is what the diff should reflect. + const patchedContent = originalContent.replace( + correctedOldString, // This was the string identified by ensureCorrectEdit for replacement + correctedNewString, // This was the string identified by ensureCorrectEdit as the replacement + ); + expect(patchedContent).toBe(expectedFinalContent); }); }); @@ -375,7 +404,8 @@ describe('EditTool', () => { it('should return error if params are invalid', async () => { const params: EditToolParams = { file_path: 'relative.txt', - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; const result = await tool.execute(params, new AbortController().signal); expect(result.llmContent).toMatch(/Error: Invalid parameters provided/); @@ -388,29 +418,26 @@ describe('EditTool', () => { fs.writeFileSync(filePath, initialContent, 'utf8'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; - // Mock ensureCorrectEdit to return the expected params and occurrences - mockEnsureCorrectEdit.mockResolvedValueOnce({ - params: { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }, - occurrences: 1, - }); + // Specific mock for this test's execution path in calculateEdit + // ensureCorrectEdit is NOT called by calculateEdit, only by shouldConfirmExecute + // So, the default mockEnsureCorrectEdit should correctly return 1 occurrence for 'old' in initialContent + + // Simulate confirmation by setting shouldAlwaysEdit + (tool as any).shouldAlwaysEdit = true; const result = await tool.execute(params, new AbortController().signal); - expect(result.llmContent).toMatch(/Successfully applied 1\/1 edits/); - expect(result.editsApplied).toBe(1); - expect(result.editsAttempted).toBe(1); - expect(result.editsFailed).toBe(0); + (tool as any).shouldAlwaysEdit = false; // Reset for other tests + + expect(result.llmContent).toMatch(/Successfully modified file/); expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent); const display = result.returnDisplay as FileDiff; - expect(display.fileDiff).toContain('-This is some old text.'); - expect(display.fileDiff).toContain('+This is some new text.'); + expect(display.fileDiff).toMatch(initialContent); + expect(display.fileDiff).toMatch(newContent); expect(display.fileName).toBe(testFile); }); @@ -420,7 +447,8 @@ describe('EditTool', () => { const fileContent = 'Content for the new file.'; const params: EditToolParams = { file_path: newFilePath, - edits: [{ old_string: '', new_string: fileContent }], + old_string: '', + new_string: fileContent, }; (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( @@ -429,65 +457,42 @@ describe('EditTool', () => { const result = await tool.execute(params, new AbortController().signal); expect(result.llmContent).toMatch(/Created new file/); - expect(result.editsApplied).toBe(1); - expect(result.editsAttempted).toBe(1); - expect(result.editsFailed).toBe(0); + expect(fs.existsSync(newFilePath)).toBe(true); expect(fs.readFileSync(newFilePath, 'utf8')).toBe(fileContent); - expect(result.returnDisplay).toContain('Created'); + expect(result.returnDisplay).toBe(`Created ${newFileName}`); }); it('should return error if old_string is not found in file', async () => { fs.writeFileSync(filePath, 'Some content.', 'utf8'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'nonexistent', new_string: 'replacement' }], + old_string: 'nonexistent', + new_string: 'replacement', }; - // Mock ensureCorrectEdit to return 0 occurrences - mockEnsureCorrectEdit.mockResolvedValueOnce({ - params: { - file_path: filePath, - old_string: 'not_found', - new_string: 'replacement', - }, - occurrences: 0, - }); - + // The default mockEnsureCorrectEdit will return 0 occurrences for 'nonexistent' const result = await tool.execute(params, new AbortController().signal); - expect(result.llmContent).toMatch(/Failed to apply any edits/); - expect(result.editsApplied).toBe(0); - expect(result.editsAttempted).toBe(1); - expect(result.editsFailed).toBe(1); - expect(result.failedEdits).toHaveLength(1); - expect(result.failedEdits![0].error).toMatch(/String not found/); + expect(result.llmContent).toMatch( + /0 occurrences found for old_string in/, + ); + expect(result.returnDisplay).toMatch( + /Failed to edit, could not find the string to replace./, + ); }); it('should return error if multiple occurrences of old_string are found', async () => { - const initialContent = 'old old content here'; - fs.writeFileSync(filePath, initialContent, 'utf8'); + fs.writeFileSync(filePath, 'multiple old old strings', 'utf8'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; - - // Mock ensureCorrectEdit to return multiple occurrences - mockEnsureCorrectEdit.mockResolvedValueOnce({ - params: { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }, - occurrences: 2, - }); - + // The default mockEnsureCorrectEdit will return 2 occurrences for 'old' const result = await tool.execute(params, new AbortController().signal); - - expect(result.llmContent).toMatch(/Failed to apply any edits/); - expect(result.editsApplied).toBe(0); - expect(result.editsAttempted).toBe(1); - expect(result.editsFailed).toBe(1); - expect(result.failedEdits).toHaveLength(1); - expect(result.failedEdits![0].error).toMatch( - /Expected 1 occurrences but found 2/, + expect(result.llmContent).toMatch( + /Expected 1 occurrences but found 2 for old_string in file/, + ); + expect(result.returnDisplay).toMatch( + /Failed to edit, expected 1 occurrence\(s\) but found 2/, ); }); @@ -495,7 +500,8 @@ describe('EditTool', () => { fs.writeFileSync(filePath, 'old text old text old text', 'utf8'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', expected_replacements: 3, }; @@ -506,7 +512,7 @@ describe('EditTool', () => { (tool as any).shouldAlwaysEdit = false; // Reset for other tests - expect(result.llmContent).toMatch(/Successfully applied 1\/1 edits/); + expect(result.llmContent).toMatch(/Successfully modified file/); expect(fs.readFileSync(filePath, 'utf8')).toBe( 'new text new text new text', ); @@ -520,159 +526,45 @@ describe('EditTool', () => { fs.writeFileSync(filePath, 'old text old text', 'utf8'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', expected_replacements: 3, // Expecting 3 but only 2 exist }; const result = await tool.execute(params, new AbortController().signal); expect(result.llmContent).toMatch( - /Failed to apply any edits.*Expected 3 occurrences but found 2/, + /Expected 3 occurrences but found 2 for old_string in file/, + ); + expect(result.returnDisplay).toMatch( + /Failed to edit, expected 3 occurrence\(s\) but found 2/, ); - expect(result.returnDisplay).toMatch(/No edits applied/); }); it('should return error if trying to create a file that already exists (empty old_string)', async () => { - const existingContent = 'File already exists.'; - fs.writeFileSync(filePath, existingContent, 'utf8'); + fs.writeFileSync(filePath, 'Existing content', 'utf8'); const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: '', new_string: 'new content' }], + old_string: '', + new_string: 'new content', }; - const result = await tool.execute(params, new AbortController().signal); - - expect(result.llmContent).toMatch(/File already exists/); - expect(result.editsApplied).toBe(0); - expect(result.editsAttempted).toBe(1); - expect(result.editsFailed).toBe(1); - }); - - it('should reject multiple edits with mixed file creation and editing on non-existent file', async () => { - // Ensure file doesn't exist - if (fs.existsSync(filePath)) { - fs.unlinkSync(filePath); - } - - const params: EditToolParams = { - file_path: filePath, - edits: [ - { old_string: '', new_string: 'new content' }, - { old_string: 'some text', new_string: 'replacement' }, - ], - }; - - const result = await tool.execute(params, new AbortController().signal); - - // File should be created with first edit, but second edit should fail - expect(result.llmContent).toMatch(/Created new file.*Failed edits/); - expect(result.editsApplied).toBe(1); - expect(result.editsFailed).toBe(1); - expect(result.failedEdits![0].error).toMatch(/String not found/); - - // File should now exist with content from first edit - expect(fs.existsSync(filePath)).toBe(true); - expect(fs.readFileSync(filePath, 'utf8')).toBe('new content'); - }); - - it('should demonstrate deterministic position-based edit behavior', async () => { - // Demonstrates that position-based processor is strict about exact matches - const originalContent = `function processUser(userData) { - const userName = userData.name; - console.log('Processing user:', userName); - return { user: userName, processed: true }; -}`; - - fs.writeFileSync(filePath, originalContent); - - const params: EditToolParams = { - file_path: filePath, - edits: [ - // This edit will succeed - userData appears exactly once - { old_string: 'userData', new_string: 'userInfo' }, - // This edit will fail - after first edit, this exact string no longer exists - { - old_string: 'const userName = userData.name;', - new_string: 'const displayName = userInfo.name;', - }, - // These demonstrate that dependent edits fail when context changes - { - old_string: "console.log('Processing user:', userName);", - new_string: "console.log('Processing user:', displayName);", - }, - ], - }; - - const result = await tool.execute(params, new AbortController().signal); - expect(result.llmContent).toMatch(/Successfully applied 2\/3 edits/); - expect(result.llmContent).toMatch( - /Failed edits.*Expected 1 occurrences but found 2/, + expect(result.llmContent).toMatch(/File already exists, cannot create/); + expect(result.returnDisplay).toMatch( + /Attempted to create a file that already exists/, ); - - // Verify what edits were actually applied (based on position-based processing) - const finalContent = fs.readFileSync(filePath, 'utf8'); - // Check that the content changed in some way (deterministic behavior test) - expect(finalContent).not.toBe(originalContent); - // The exact result depends on position-based processing order - expect(finalContent).toContain('userInfo'); - }); - - it('should handle non-conflicting edits efficiently', async () => { - // Demonstrates successful position-based processing with non-conflicting edits - const originalContent = `const config = { - apiUrl: 'https://api.old.com', - timeout: 5000, - retries: 3 -}; - -function makeRequest() { - return fetch(config.apiUrl); -}`; - - fs.writeFileSync(filePath, originalContent); - - const params: EditToolParams = { - file_path: filePath, - edits: [ - // These edits don't interfere with each other - { - old_string: "apiUrl: 'https://api.old.com'", - new_string: "apiUrl: 'https://api.new.com'", - }, - { old_string: 'timeout: 5000', new_string: 'timeout: 10000' }, - { old_string: 'retries: 3', new_string: 'retries: 5' }, - ], - }; - - const result = await tool.execute(params, new AbortController().signal); - expect(result.llmContent).toMatch(/Successfully applied 3\/3 edits/); - - // All edits should succeed because they don't conflict - const finalContent = fs.readFileSync(filePath, 'utf8'); - const expectedContent = `const config = { - apiUrl: 'https://api.new.com', - timeout: 10000, - retries: 5 -}; - -function makeRequest() { - return fetch(config.apiUrl); -}`; - - expect(finalContent).toBe(expectedContent); }); }); describe('getDescription', () => { - it('should return consistent format even if old_string and new_string are the same', () => { + it('should return "No file changes to..." if old_string and new_string are the same', () => { const testFileName = 'test.txt'; const params: EditToolParams = { file_path: path.join(rootDir, testFileName), - edits: [ - { old_string: 'identical_string', new_string: 'identical_string' }, - ], + old_string: 'identical_string', + new_string: 'identical_string', }; // shortenPath will be called internally, resulting in just the file name expect(tool.getDescription(params)).toBe( - `${testFileName}: identical_string => identical_string`, + `No file changes to ${testFileName}`, ); }); @@ -680,12 +572,8 @@ function makeRequest() { const testFileName = 'test.txt'; const params: EditToolParams = { file_path: path.join(rootDir, testFileName), - edits: [ - { - old_string: 'this is the old string value', - new_string: 'this is the new string value', - }, - ], + old_string: 'this is the old string value', + new_string: 'this is the new string value', }; // shortenPath will be called internally, resulting in just the file name // The snippets are truncated at 30 chars + '...' @@ -698,7 +586,8 @@ function makeRequest() { const testFileName = 'short.txt'; const params: EditToolParams = { file_path: path.join(rootDir, testFileName), - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; expect(tool.getDescription(params)).toBe(`${testFileName}: old => new`); }); @@ -707,14 +596,10 @@ function makeRequest() { const testFileName = 'long.txt'; const params: EditToolParams = { file_path: path.join(rootDir, testFileName), - edits: [ - { - old_string: - 'this is a very long old string that will definitely be truncated', - new_string: - 'this is a very long new string that will also be truncated', - }, - ], + old_string: + 'this is a very long old string that will definitely be truncated', + new_string: + 'this is a very long new string that will also be truncated', }; expect(tool.getDescription(params)).toBe( `${testFileName}: this is a very long old string... => this is a very long new string...`, @@ -740,9 +625,8 @@ function makeRequest() { const originalContent = 'original content'; const params: EditToolParams = { file_path: filePath, - edits: [ - { old_string: originalContent, new_string: 'modified content' }, - ], + old_string: originalContent, + new_string: 'modified content', }; fs.writeFileSync(filePath, originalContent, 'utf8'); @@ -765,9 +649,8 @@ function makeRequest() { expect(result).toBeDefined(); expect(result!.updatedParams).toEqual({ file_path: filePath, - edits: [ - { old_string: originalContent, new_string: 'modified content' }, - ], + old_string: originalContent, + new_string: 'modified content', }); expect(result!.updatedDiff).toEqual(`Index: some_file.txt =================================================================== @@ -788,7 +671,8 @@ function makeRequest() { it('should handle non-existent files and return updated params', async () => { const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: '', new_string: 'new file content' }], + old_string: '', + new_string: 'new file content', }; const result = await tool.onModify( @@ -804,7 +688,8 @@ function makeRequest() { expect(result).toBeDefined(); expect(result!.updatedParams).toEqual({ file_path: filePath, - edits: [{ old_string: '', new_string: 'new file content' }], + old_string: '', + new_string: 'new file content', }); expect(result!.updatedDiff).toContain('new file content'); @@ -816,7 +701,8 @@ function makeRequest() { it('should clean up previous temp files before creating new ones', async () => { const params: EditToolParams = { file_path: filePath, - edits: [{ old_string: 'old', new_string: 'new' }], + old_string: 'old', + new_string: 'new', }; fs.writeFileSync(filePath, 'some old content', 'utf8'); |