fix(filesystem): make move_file fail instead of overwriting the destination (#4630)
The move_file tool description and README both state the operation fails if the destination already exists, but the handler called fs.rename directly, which silently overwrites the destination. Since this server ships no delete tool, move_file effectively provided unadvertised, irreversible file deletion: moving any file onto an existing target destroyed the target. Add a moveFile helper in lib.ts that rejects when the destination already exists (checked with lstat, so an existing symlink is detected rather than followed) and only renames when the target is free, and use it from the move_file handler. Behaviour now matches the documented contract. Fixes #4628
This commit is contained in:
parent
562feeb281
commit
18ce197639
3 changed files with 45 additions and 1 deletions
|
|
@ -14,6 +14,7 @@ import {
|
|||
getFileStats,
|
||||
readFileContent,
|
||||
writeFileContent,
|
||||
moveFile,
|
||||
// Search & filtering functions
|
||||
searchFilesWithValidation,
|
||||
// File editing functions
|
||||
|
|
@ -339,6 +340,29 @@ describe('Lib Functions', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('moveFile', () => {
|
||||
it('moves the file when the destination does not exist', async () => {
|
||||
const enoent = Object.assign(new Error('not found'), { code: 'ENOENT' });
|
||||
mockFs.lstat.mockRejectedValueOnce(enoent);
|
||||
mockFs.rename.mockResolvedValueOnce(undefined);
|
||||
|
||||
await moveFile('/test/source.txt', '/test/dest.txt');
|
||||
|
||||
expect(mockFs.rename).toHaveBeenCalledWith('/test/source.txt', '/test/dest.txt');
|
||||
});
|
||||
|
||||
it('fails without overwriting when the destination already exists', async () => {
|
||||
// lstat resolving means the destination is occupied.
|
||||
mockFs.lstat.mockResolvedValueOnce({} as any);
|
||||
|
||||
await expect(moveFile('/test/source.txt', '/test/dest.txt')).rejects.toThrow(
|
||||
'Destination already exists'
|
||||
);
|
||||
|
||||
expect(mockFs.rename).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
});
|
||||
|
||||
describe('Search & Filtering Functions', () => {
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ import {
|
|||
getFileStats,
|
||||
readFileContent,
|
||||
writeFileContent,
|
||||
moveFile,
|
||||
searchFilesWithValidation,
|
||||
applyFileEdits,
|
||||
tailFile,
|
||||
|
|
@ -631,7 +632,7 @@ server.registerTool(
|
|||
async (args: z.infer<typeof MoveFileArgsSchema>) => {
|
||||
const validSourcePath = await validatePath(args.source);
|
||||
const validDestPath = await validatePath(args.destination);
|
||||
await fs.rename(validSourcePath, validDestPath);
|
||||
await moveFile(validSourcePath, validDestPath);
|
||||
const text = `Successfully moved ${args.source} to ${args.destination}`;
|
||||
const contentBlock = { type: "text" as const, text };
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -193,6 +193,25 @@ export async function writeFileContent(filePath: string, content: string): Promi
|
|||
}
|
||||
|
||||
|
||||
export async function moveFile(sourcePath: string, destinationPath: string): Promise<void> {
|
||||
// The move_file tool contract (and README) state the operation fails if the
|
||||
// destination already exists. fs.rename would silently overwrite it, which is
|
||||
// a data-loss bug, so reject up front when anything - file, directory, or
|
||||
// symlink - occupies the target. lstat is used so an existing symlink at the
|
||||
// destination is detected rather than followed.
|
||||
try {
|
||||
await fs.lstat(destinationPath);
|
||||
} catch (error) {
|
||||
if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
|
||||
await fs.rename(sourcePath, destinationPath);
|
||||
return;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
throw new Error(`Destination already exists: ${destinationPath}`);
|
||||
}
|
||||
|
||||
|
||||
// File Editing Functions
|
||||
interface FileEdit {
|
||||
oldText: string;
|
||||
|
|
|
|||
Loading…
Reference in a new issue