fix edit retrigger (#2306)
This commit is contained in:
parent
3518ff7663
commit
601d9ba36d
|
@ -549,6 +549,65 @@ describe('EditTool', () => {
|
||||||
/Attempted to create a file that already exists/,
|
/Attempted to create a file that already exists/,
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should include modification message when proposed content is modified', async () => {
|
||||||
|
const initialContent = 'This is some old text.';
|
||||||
|
fs.writeFileSync(filePath, initialContent, 'utf8');
|
||||||
|
const params: EditToolParams = {
|
||||||
|
file_path: filePath,
|
||||||
|
old_string: 'old',
|
||||||
|
new_string: 'new',
|
||||||
|
modified_by_user: true,
|
||||||
|
};
|
||||||
|
|
||||||
|
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
|
||||||
|
ApprovalMode.AUTO_EDIT,
|
||||||
|
);
|
||||||
|
const result = await tool.execute(params, new AbortController().signal);
|
||||||
|
|
||||||
|
expect(result.llmContent).toMatch(
|
||||||
|
/User modified the `new_string` content/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
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');
|
||||||
|
const params: EditToolParams = {
|
||||||
|
file_path: filePath,
|
||||||
|
old_string: 'old',
|
||||||
|
new_string: 'new',
|
||||||
|
modified_by_user: false,
|
||||||
|
};
|
||||||
|
|
||||||
|
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
|
||||||
|
ApprovalMode.AUTO_EDIT,
|
||||||
|
);
|
||||||
|
const result = await tool.execute(params, new AbortController().signal);
|
||||||
|
|
||||||
|
expect(result.llmContent).not.toMatch(
|
||||||
|
/User modified the `new_string` content/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should not include modification message when modified_by_user is not provided', async () => {
|
||||||
|
const initialContent = 'This is some old text.';
|
||||||
|
fs.writeFileSync(filePath, initialContent, 'utf8');
|
||||||
|
const params: EditToolParams = {
|
||||||
|
file_path: filePath,
|
||||||
|
old_string: 'old',
|
||||||
|
new_string: 'new',
|
||||||
|
};
|
||||||
|
|
||||||
|
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
|
||||||
|
ApprovalMode.AUTO_EDIT,
|
||||||
|
);
|
||||||
|
const result = await tool.execute(params, new AbortController().signal);
|
||||||
|
|
||||||
|
expect(result.llmContent).not.toMatch(
|
||||||
|
/User modified the `new_string` content/,
|
||||||
|
);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('getDescription', () => {
|
describe('getDescription', () => {
|
||||||
|
|
|
@ -49,6 +49,11 @@ export interface EditToolParams {
|
||||||
* Use when you want to replace multiple occurrences.
|
* Use when you want to replace multiple occurrences.
|
||||||
*/
|
*/
|
||||||
expected_replacements?: number;
|
expected_replacements?: number;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether the edit was modified manually by the user.
|
||||||
|
*/
|
||||||
|
modified_by_user?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
interface CalculatedEdit {
|
interface CalculatedEdit {
|
||||||
|
@ -81,6 +86,8 @@ export class EditTool
|
||||||
'Edit',
|
'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, 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.
|
||||||
|
|
||||||
|
The user has the ability to modify the \`new_string\` content. If modified, this will be stated in the response.
|
||||||
|
|
||||||
Expectation for required parameters:
|
Expectation for required parameters:
|
||||||
1. \`file_path\` MUST be an absolute path; otherwise an error will be thrown.
|
1. \`file_path\` MUST be an absolute path; otherwise an error will be thrown.
|
||||||
2. \`old_string\` MUST be the exact literal text to replace (including all whitespace, indentation, newlines, and surrounding code etc.).
|
2. \`old_string\` MUST be the exact literal text to replace (including all whitespace, indentation, newlines, and surrounding code etc.).
|
||||||
|
@ -414,12 +421,19 @@ Expectation for required parameters:
|
||||||
displayResult = { fileDiff, fileName };
|
displayResult = { fileDiff, fileName };
|
||||||
}
|
}
|
||||||
|
|
||||||
const llmSuccessMessage = editData.isNewFile
|
const llmSuccessMessageParts = [
|
||||||
? `Created new file: ${params.file_path} with provided content.`
|
editData.isNewFile
|
||||||
: `Successfully modified file: ${params.file_path} (${editData.occurrences} replacements).`;
|
? `Created new file: ${params.file_path} with provided content.`
|
||||||
|
: `Successfully modified file: ${params.file_path} (${editData.occurrences} replacements).`,
|
||||||
|
];
|
||||||
|
if (params.modified_by_user) {
|
||||||
|
llmSuccessMessageParts.push(
|
||||||
|
`User modified the \`new_string\` content to be: ${params.new_string}.`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
llmContent: llmSuccessMessage,
|
llmContent: llmSuccessMessageParts.join(' '),
|
||||||
returnDisplay: displayResult,
|
returnDisplay: displayResult,
|
||||||
};
|
};
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
@ -474,6 +488,7 @@ Expectation for required parameters:
|
||||||
...originalParams,
|
...originalParams,
|
||||||
old_string: oldContent,
|
old_string: oldContent,
|
||||||
new_string: modifiedProposedContent,
|
new_string: modifiedProposedContent,
|
||||||
|
modified_by_user: true,
|
||||||
}),
|
}),
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
|
@ -567,5 +567,49 @@ describe('WriteFileTool', () => {
|
||||||
expect(fs.existsSync(filePath)).toBe(true);
|
expect(fs.existsSync(filePath)).toBe(true);
|
||||||
expect(fs.readFileSync(filePath, 'utf8')).toBe(content);
|
expect(fs.readFileSync(filePath, 'utf8')).toBe(content);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should include modification message when proposed content is modified', async () => {
|
||||||
|
const filePath = path.join(rootDir, 'new_file_modified.txt');
|
||||||
|
const content = 'New file content modified by user';
|
||||||
|
mockEnsureCorrectFileContent.mockResolvedValue(content);
|
||||||
|
|
||||||
|
const params = {
|
||||||
|
file_path: filePath,
|
||||||
|
content,
|
||||||
|
modified_by_user: true,
|
||||||
|
};
|
||||||
|
const result = await tool.execute(params, abortSignal);
|
||||||
|
|
||||||
|
expect(result.llmContent).toMatch(/User modified the `content`/);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should not include modification message when proposed content is not modified', async () => {
|
||||||
|
const filePath = path.join(rootDir, 'new_file_unmodified.txt');
|
||||||
|
const content = 'New file content not modified';
|
||||||
|
mockEnsureCorrectFileContent.mockResolvedValue(content);
|
||||||
|
|
||||||
|
const params = {
|
||||||
|
file_path: filePath,
|
||||||
|
content,
|
||||||
|
modified_by_user: false,
|
||||||
|
};
|
||||||
|
const result = await tool.execute(params, abortSignal);
|
||||||
|
|
||||||
|
expect(result.llmContent).not.toMatch(/User modified the `content`/);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should not include modification message when modified_by_user is not provided', async () => {
|
||||||
|
const filePath = path.join(rootDir, 'new_file_unmodified.txt');
|
||||||
|
const content = 'New file content not modified';
|
||||||
|
mockEnsureCorrectFileContent.mockResolvedValue(content);
|
||||||
|
|
||||||
|
const params = {
|
||||||
|
file_path: filePath,
|
||||||
|
content,
|
||||||
|
};
|
||||||
|
const result = await tool.execute(params, abortSignal);
|
||||||
|
|
||||||
|
expect(result.llmContent).not.toMatch(/User modified the `content`/);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
|
@ -45,6 +45,11 @@ export interface WriteFileToolParams {
|
||||||
* The content to write to the file
|
* The content to write to the file
|
||||||
*/
|
*/
|
||||||
content: string;
|
content: string;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether the proposed content was modified by the user.
|
||||||
|
*/
|
||||||
|
modified_by_user?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
interface GetCorrectedFileContentResult {
|
interface GetCorrectedFileContentResult {
|
||||||
|
@ -68,7 +73,9 @@ export class WriteFileTool
|
||||||
super(
|
super(
|
||||||
WriteFileTool.Name,
|
WriteFileTool.Name,
|
||||||
'WriteFile',
|
'WriteFile',
|
||||||
'Writes content to a specified file in the local filesystem.',
|
`Writes content to a specified file in the local filesystem.
|
||||||
|
|
||||||
|
The user has the ability to modify \`content\`. If modified, this will be stated in the response.`,
|
||||||
{
|
{
|
||||||
properties: {
|
properties: {
|
||||||
file_path: {
|
file_path: {
|
||||||
|
@ -270,9 +277,16 @@ export class WriteFileTool
|
||||||
DEFAULT_DIFF_OPTIONS,
|
DEFAULT_DIFF_OPTIONS,
|
||||||
);
|
);
|
||||||
|
|
||||||
const llmSuccessMessage = isNewFile
|
const llmSuccessMessageParts = [
|
||||||
? `Successfully created and wrote to new file: ${params.file_path}`
|
isNewFile
|
||||||
: `Successfully overwrote file: ${params.file_path}`;
|
? `Successfully created and wrote to new file: ${params.file_path}.`
|
||||||
|
: `Successfully overwrote file: ${params.file_path}.`,
|
||||||
|
];
|
||||||
|
if (params.modified_by_user) {
|
||||||
|
llmSuccessMessageParts.push(
|
||||||
|
`User modified the \`content\` to be: ${params.content}`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
const displayResult: FileDiff = { fileDiff, fileName };
|
const displayResult: FileDiff = { fileDiff, fileName };
|
||||||
|
|
||||||
|
@ -298,7 +312,7 @@ export class WriteFileTool
|
||||||
}
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
llmContent: llmSuccessMessage,
|
llmContent: llmSuccessMessageParts.join(' '),
|
||||||
returnDisplay: displayResult,
|
returnDisplay: displayResult,
|
||||||
};
|
};
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
@ -395,6 +409,7 @@ export class WriteFileTool
|
||||||
) => ({
|
) => ({
|
||||||
...originalParams,
|
...originalParams,
|
||||||
content: modifiedProposedContent,
|
content: modifiedProposedContent,
|
||||||
|
modified_by_user: true,
|
||||||
}),
|
}),
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue