Skip to content

Conversation

@nandsha
Copy link
Contributor

@nandsha nandsha commented Jun 4, 2025

Description

Implements a new copy_file tool in the filesystem server to enable efficient copying of files and directories without requiring read/write operations for large files.

Server Details

  • Server: filesystem
  • Changes to: tool implementation, schema definitions, documentation

Motivation and Context

Fixes #1581 - Currently, the agent has to read all the file content and write it to another location. For large files, this takes considerable time and memory. This implementation adds a dedicated copy tool that uses optimized native file operations.

How Has This Been Tested?

  • Built successfully with npm run build
  • Tested copying individual files
  • Tested copying directories recursively
  • Tested error cases (destination exists, invalid paths)
  • Maintains all existing security validations

Breaking Changes

No breaking changes. This is a new feature that adds functionality without modifying existing behavior.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

Implementation Details:

  • Uses fs.copyFile for individual files (optimized for large files)
  • Uses fs.cp with recursive option for directories
  • Validates both source and destination paths are within allowed directories
  • Fails if destination already exists (consistent with move_file behavior)
  • Follows existing patterns and conventions in the codebase

Key Changes:

  1. index.ts:

    • Added CopyFileArgsSchema following existing schema patterns
    • Added copy_file tool definition to the tools list
    • Implemented copy_file handler with proper validation and error handling
  2. README.md:

    • Added documentation for the new copy_file tool
    • Documented inputs, behavior, and constraints

@olaservo olaservo added server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem enhancement New feature or request labels Jun 5, 2025
@nandsha nandsha force-pushed the feat/filesystem-copy-tool-1581 branch from 73e8965 to 2003e17 Compare June 25, 2025 10:40
…ying

Implements a new copy_file tool that addresses issue modelcontextprotocol#1581 by providing an efficient way to copy files and directories without requiring read/write operations for large files.

## Changes:
- Add copy_file tool with support for both files and directories
- Use fs.copyFile for files (optimized for large files)
- Use fs.cp with recursive option for directories
- Include proper validation and error handling
- Update README documentation

## Implementation Details:
- Validates both source and destination paths are within allowed directories
- Checks if destination exists and fails appropriately
- Follows existing tool patterns for consistency
- Maintains security checks and path validation

Fixes modelcontextprotocol#1581
@nandsha nandsha force-pushed the feat/filesystem-copy-tool-1581 branch from 2003e17 to 81cf93a Compare June 26, 2025 13:16
Copy link
Member

@olaservo olaservo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, left some comments. It could also make sense to group the copy tool code alongside the move_file tool.

};
}

case "copy_file": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new tool needs to incorporate the same security fix that was added to the write_file tool, something like:

case "copy_file": {
  const parsed = CopyFileArgsSchema.safeParse(args);
  if (!parsed.success) {
    throw new Error(`Invalid arguments for copy_file: ${parsed.error}`);
  }
  const validSourcePath = await validatePath(parsed.data.source);
  const validDestPath = await validatePath(parsed.data.destination);

  // Check if source exists and get stats
  const sourceStats = await fs.stat(validSourcePath);
  
  // Use atomic temp file + rename approach (like write_file)
  const tempPath = `${validDestPath}.${randomBytes(16).toString('hex')}.tmp`;
  
  try {
    if (sourceStats.isDirectory()) {
      // For directories, use fs.cp with options to prevent symlink following
      await fs.cp(validSourcePath, tempPath, { 
        recursive: true,
        dereference: false,  // Don't follow symlinks
        preserveTimestamps: true
      });
    } else {
      // For files, copy to temp location first
      await fs.copyFile(validSourcePath, tempPath);
    }
    
    // Atomic rename - this will fail if destination exists and won't follow symlinks
    await fs.rename(tempPath, validDestPath);
    
  } catch (error) {
    // Cleanup temp file/directory
    try {
      const tempStats = await fs.stat(tempPath);
      if (tempStats.isDirectory()) {
        await fs.rm(tempPath, { recursive: true });
      } else {
        await fs.unlink(tempPath);
      }
    } catch {}
    
    if ((error as NodeJS.ErrnoException).code === 'EEXIST') {
      throw new Error(`Destination already exists: ${parsed.data.destination}`);
    }
    throw error;
  }

  return {
    content: [{ type: "text", text: `Successfully copied ${parsed.data.source} to ${parsed.data.destination}` }],
  };
}

The tldr; version of what needs to be addressed, based on the previous PR:

  • Atomic Operation: fs.rename() is atomic - either succeeds completely or fails completely
  • No Symlink Following: fs.rename() doesn't follow symlinks at the destination
  • Fail-Safe: If destination exists (including symlinks), fs.rename() fails with EEXIST
  • Race Condition Proof: No window between "check" and "action" - the action itself is the check

@olaservo olaservo added the waiting for submitter Waiting for the submitter to provide more info label Jul 8, 2025
@domdomegg
Copy link
Member

Hey, thanks for the contribution! This hasn't been updated for a while, so I'm closing it to help us more easily track active contributions. If you're still interested in working on this, please feel free to reopen it. Thanks again for your contribution!

@domdomegg domdomegg closed this Aug 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem waiting for submitter Waiting for the submitter to provide more info

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Filesystem] Tool to copy a file

3 participants