abbyswag/qwen-code
1
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2025-12-20 08:47:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

🎯 PR: Improve Edit Tool Reliability with Fuzzy Matching Pipeline (#1025)

Browse Source
This commit is contained in:
tanzhenxin
2025-11-13 19:01:09 +08:00
committed by GitHub
parent b029f0d2ce
commit 0752a31e1e
5 changed files with 718 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -425,7 +425,9 @@ describe('EditTool', () => {
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/Successfully modified file/);
expect(result.llmContent).toMatch(
/Showing lines \d+-\d+ of \d+ from the edited file:/,
);
expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent);
const display = result.returnDisplay as FileDiff;
expect(display.fileDiff).toMatch(initialContent);
@@ -450,6 +452,9 @@ describe('EditTool', () => {
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/Created new file/);
expect(result.llmContent).toMatch(
/Showing lines \d+-\d+ of \d+ from the edited file:/,
);
expect(fs.existsSync(newFilePath)).toBe(true);
expect(fs.readFileSync(newFilePath, 'utf8')).toBe(fileContent);
@@ -485,7 +490,7 @@ describe('EditTool', () => {
);
});
it('should return error if multiple occurrences of old_string are found', async () => {
it('should return error if multiple occurrences of old_string are found and replace_all is false', async () => {
fs.writeFileSync(filePath, 'multiple old old strings', 'utf8');
const params: EditToolParams = {
file_path: filePath,
@@ -494,27 +499,27 @@ describe('EditTool', () => {
};
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(
/Expected 1 occurrence but found 2 for old_string in file/,
);
expect(result.llmContent).toMatch(/replace_all was not enabled/);
expect(result.returnDisplay).toMatch(
/Failed to edit, expected 1 occurrence but found 2/,
/Failed to edit because the text matches multiple locations/,
);
});
it('should successfully replace multiple occurrences when expected_replacements specified', async () => {
it('should successfully replace multiple occurrences when replace_all is true', async () => {
fs.writeFileSync(filePath, 'old text\nold text\nold text', 'utf8');
const params: EditToolParams = {
file_path: filePath,
old_string: 'old',
new_string: 'new',
expected_replacements: 3,
replace_all: true,
};
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/Successfully modified file/);
expect(result.llmContent).toMatch(
/Showing lines \d+-\d+ of \d+ from the edited file/,
);
expect(fs.readFileSync(filePath, 'utf8')).toBe(
'new text\nnew text\nnew text',
);
@@ -535,24 +540,6 @@ describe('EditTool', () => {
});
});
it('should return error if expected_replacements does not match actual occurrences', async () => {
fs.writeFileSync(filePath, 'old text old text', 'utf8');
const params: EditToolParams = {
file_path: filePath,
old_string: 'old',
new_string: 'new',
expected_replacements: 3, // Expecting 3 but only 2 exist
};
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(
/Expected 3 occurrences but found 2 for old_string in file/,
);
expect(result.returnDisplay).toMatch(
/Failed to edit, expected 3 occurrences but found 2/,
);
});
it('should return error if trying to create a file that already exists (empty old_string)', async () => {
fs.writeFileSync(filePath, 'Existing content', 'utf8');
const params: EditToolParams = {
@@ -568,38 +555,6 @@ describe('EditTool', () => {
);
});
it('should include modification message when proposed content is modified', async () => {
const initialContent = 'Line 1\nold line\nLine 3\nLine 4\nLine 5\n';
fs.writeFileSync(filePath, initialContent, 'utf8');
const params: EditToolParams = {
file_path: filePath,
old_string: 'old',
new_string: 'new',
modified_by_user: true,
ai_proposed_content: 'Line 1\nAI line\nLine 3\nLine 4\nLine 5\n',
};
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
ApprovalMode.AUTO_EDIT,
);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(
/User modified the `new_string` content/,
);
expect((result.returnDisplay as FileDiff).diffStat).toStrictEqual({
model_added_lines: 1,
model_removed_lines: 1,
model_added_chars: 7,
model_removed_chars: 8,
user_added_lines: 1,
user_removed_lines: 1,
user_added_chars: 8,
user_removed_chars: 7,
});
});
it('should not include modification message when proposed content is not modified', async () => {
const initialContent = 'This is some old text.';
fs.writeFileSync(filePath, initialContent, 'utf8');
@@ -723,13 +678,12 @@ describe('EditTool', () => {
expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_OCCURRENCE_FOUND);
});
it('should return EXPECTED_OCCURRENCE_MISMATCH error', async () => {
it('should return EXPECTED_OCCURRENCE_MISMATCH error when replace_all is false and text is not unique', async () => {
fs.writeFileSync(filePath, 'one one two', 'utf8');
const params: EditToolParams = {
file_path: filePath,
old_string: 'one',
new_string: 'new',
expected_replacements: 3,
};
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);

87
packages/core/src/tools/edit.ts
View File

@@ -34,6 +34,12 @@ import type {
} from './modifiable-tool.js';
import { IdeClient } from '../ide/ide-client.js';
import { safeLiteralReplace } from '../utils/textUtils.js';
import {
countOccurrences,
extractEditSnippet,
maybeAugmentOldStringForDeletion,
normalizeEditStrings,
} from '../utils/editHelper.js';
export function applyReplacement(
currentContent: string | null,
@@ -77,10 +83,9 @@ export interface EditToolParams {
new_string: string;
/**
* Number of replacements expected. Defaults to 1 if not specified.
* Use when you want to replace multiple occurrences.
* Replace every occurrence of old_string instead of requiring a unique match.
*/
expected_replacements?: number;
replace_all?: boolean;
/**
* Whether the edit was modified manually by the user.
@@ -118,12 +123,12 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
* @throws File system errors if reading the file fails unexpectedly (e.g., permissions)
*/
private async calculateEdit(params: EditToolParams): Promise<CalculatedEdit> {
const expectedReplacements = params.expected_replacements ?? 1;
const replaceAll = params.replace_all ?? false;
let currentContent: string | null = null;
let fileExists = false;
let isNewFile = false;
const finalNewString = params.new_string;
const finalOldString = params.old_string;
let finalNewString = params.new_string;
let finalOldString = params.old_string;
let occurrences = 0;
let error:
| { display: string; raw: string; type: ToolErrorType }
@@ -144,7 +149,15 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
fileExists = false;
}
if (params.old_string === '' && !fileExists) {
const normalizedStrings = normalizeEditStrings(
currentContent,
finalOldString,
finalNewString,
);
finalOldString = normalizedStrings.oldString;
finalNewString = normalizedStrings.newString;
if (finalOldString === '' && !fileExists) {
// Creating a new file
isNewFile = true;
} else if (!fileExists) {
@@ -155,7 +168,13 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
type: ToolErrorType.FILE_NOT_FOUND,
};
} else if (currentContent !== null) {
occurrences = this.countOccurrences(currentContent, params.old_string);
finalOldString = maybeAugmentOldStringForDeletion(
currentContent,
finalOldString,
finalNewString,
);
occurrences = countOccurrences(currentContent, finalOldString);
if (params.old_string === '') {
// Error: Trying to create a file that already exists
error = {
@@ -169,13 +188,10 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
raw: `Failed to edit, 0 occurrences found for old_string in ${params.file_path}. No edits made. The exact text in old_string was not found. Ensure you're not escaping content incorrectly and check whitespace, indentation, and context. Use ${ReadFileTool.Name} tool to verify.`,
type: ToolErrorType.EDIT_NO_OCCURRENCE_FOUND,
};
} else if (occurrences !== expectedReplacements) {
const occurrenceTerm =
expectedReplacements === 1 ? 'occurrence' : 'occurrences';
} else if (!replaceAll && occurrences > 1) {
error = {
display: `Failed to edit, expected ${expectedReplacements} ${occurrenceTerm} but found ${occurrences}.`,
raw: `Failed to edit, Expected ${expectedReplacements} ${occurrenceTerm} but found ${occurrences} for old_string in file: ${params.file_path}`,
display: `Failed to edit because the text matches multiple locations. Provide more context or set replace_all to true.`,
raw: `Failed to edit. Found ${occurrences} occurrences for old_string in ${params.file_path} but replace_all was not enabled.`,
type: ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH,
};
} else if (finalOldString === finalNewString) {
@@ -221,22 +237,6 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
};
}
/**
* Counts occurrences of a substring in a string
*/
private countOccurrences(str: string, substr: string): number {
if (substr === '') {
return 0;
}
let count = 0;
let pos = str.indexOf(substr);
while (pos !== -1) {
count++;
pos = str.indexOf(substr, pos + substr.length); // Start search after the current match
}
return count;
}
/**
* Handles the confirmation prompt for the Edit tool in the CLI.
* It needs to calculate the diff to show the user.
@@ -422,12 +422,16 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
const llmSuccessMessageParts = [
editData.isNewFile
? `Created new file: ${this.params.file_path} with provided content.`
: `Successfully modified file: ${this.params.file_path} (${editData.occurrences} replacements).`,
: `The file: ${this.params.file_path} has been updated.`,
];
if (this.params.modified_by_user) {
llmSuccessMessageParts.push(
`User modified the \`new_string\` content to be: ${this.params.new_string}.`,
);
const snippetResult = extractEditSnippet(
editData.currentContent,
editData.newContent,
);
if (snippetResult) {
const snippetText = `Showing lines ${snippetResult.startLine}-${snippetResult.endLine} of ${snippetResult.totalLines} from the edited file:\n\n---\n\n${snippetResult.content}`;
llmSuccessMessageParts.push(snippetText);
}
return {
@@ -470,7 +474,7 @@ export class EditTool
super(
EditTool.Name,
ToolDisplayNames.EDIT,
`Replaces text within a file. By default, replaces a single occurrence, but can replace multiple occurrences when \`expected_replacements\` is specified. This tool requires providing significant context around the change to ensure precise targeting. Always use the ${ReadFileTool.Name} tool to examine the file's current content before attempting a text replacement.
`Replaces text within a file. By default, replaces a single occurrence. Set \`replace_all\` to true when you intend to modify every instance of \`old_string\`. This tool requires providing significant context around the change to ensure precise targeting. Always use the ${ReadFileTool.Name} tool to examine the file's current content before attempting a text replacement.
The user has the ability to modify the \`new_string\` content. If modified, this will be stated in the response.
@@ -480,7 +484,7 @@ Expectation for required parameters:
3. \`new_string\` MUST be the exact literal text to replace \`old_string\` with (also including all whitespace, indentation, newlines, and surrounding code etc.). Ensure the resulting code is correct and idiomatic.
4. NEVER escape \`old_string\` or \`new_string\`, that would break the exact literal text requirement.
**Important:** If ANY of the above are not satisfied, the tool will fail. CRITICAL for \`old_string\`: Must uniquely identify the single instance to change. Include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. If this string matches multiple locations, or does not match exactly, the tool will fail.
**Multiple replacements:** Set \`expected_replacements\` to the number of occurrences you want to replace. The tool will replace ALL occurrences that match \`old_string\` exactly. Ensure the number of replacements matches your expectation.`,
**Multiple replacements:** Set \`replace_all\` to true when you want to replace every occurrence that matches \`old_string\`.`,
Kind.Edit,
{
properties: {
@@ -491,7 +495,7 @@ Expectation for required parameters:
},
old_string: {
description:
'The exact literal text to replace, preferably unescaped. For single replacements (default), include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. For multiple replacements, specify expected_replacements parameter. If this string is not the exact literal text (i.e. you escaped it) or does not match exactly, the tool will fail.',
'The exact literal text to replace, preferably unescaped. For single replacements (default), include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. If this string is not the exact literal text (i.e. you escaped it) or does not match exactly, the tool will fail.',
type: 'string',
},
new_string: {
@@ -499,11 +503,10 @@ Expectation for required parameters:
'The exact literal text to replace `old_string` with, preferably unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic.',
type: 'string',
},
expected_replacements: {
type: 'number',
replace_all: {
type: 'boolean',
description:
'Number of replacements expected. Defaults to 1 if not specified. Use when you want to replace multiple occurrences.',
minimum: 1,
'Replace all occurrences of old_string (default false).',
},
},
required: ['file_path', 'old_string', 'new_string'],