abbyswag/qwen-code
1
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2025-12-19 09:33:53 +00:00
Code Issues Packages Projects Releases Wiki Activity

Revert "Add batch editing capabilities to Edit Tool (#648)" (#857)

Browse Source
This commit is contained in:
N. Taylor Mullen
2025-06-08 16:20:43 -07:00
committed by GitHub
parent 152af28a34
commit d62dad5575
21 changed files with 1544 additions and 3159 deletions
Download Patch File Download Diff File Expand all files Collapse all files

424
packages/core/src/tools/edit.test.ts
View File

@@ -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(
params,
new AbortController().signal,
// 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,
};
},
);
// Should return false because edit will fail (no exact match)
expect(confirmation).toBe(false);
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}`);
// 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');