Skip to content

Conversation

@sebastien-rosset
Copy link
Contributor

Description

This PR fixes filesystem server crashes when invalid or inaccessible paths are configured by implementing graceful path validation. The server now logs warnings for problematic paths at startup but continues running, allowing operations to retry at runtime when storage becomes available.

Server Details

  • Server: filesystem
  • Changes to: startup validation logic, path handling, error handling, logging

Motivation and Context

Addresses GitHub issues #2113 and #2483. The filesystem server would crash immediately if any configured directory path was invalid, inaccessible, or pointed to a file instead of a directory. This was problematic for users with:

  • Network Attached Storage (NAS) that might be temporarily offline
  • VPN-mounted drives that connect after server startup
  • USB/external drives that may not be connected at startup
  • Typos in path configuration
  • Directories that don't exist yet but will be created later

The original behavior provided a poor user experience and made the server fragile in dynamic storage environments.

How Has This Been Tested?

  • Tested with valid accessible directories (normal operation)
  • Tested with non-existent paths (graceful warning, runtime retry)
  • Tested with file paths instead of directories (proper skipping)
  • Tested with mixed valid/invalid configurations
  • Verified parallel validation performance with Promise.all
  • Confirmed runtime operations work when storage becomes available

Breaking Changes

No breaking changes. This is backwards compatible - existing configurations continue to work. The only change is that invalid paths now generate warnings instead of crashes.

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:

  • Replaced sequential validation loop with Promise.all for better performance
  • Added clear comments explaining retry vs skip logic for different path types
  • Regular files are permanently excluded (can never become directories)
  • Symlinks and special files are included for runtime retry (NAS/VPN scenarios)
  • Inaccessible paths are included for runtime retry (network storage, permissions)

Design Philosophy:
The new approach is optimistic about dynamic storage - we include paths that might become valid directories later while permanently excluding things that can never be directories. This balances robustness with usability for modern storage environments.

@olaservo olaservo added the server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem label Aug 23, 2025
}
} catch (error) {
// Include inaccessible paths - they might become accessible when storage/network reconnects
console.error(`Directory not accessible: ${dir} - ${error instanceof Error ? error.message : String(error)}`);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it makes sense to include invalid drives is the list of allowed directories, since it create a weird experience if someone makes a mistake with a filepath. For example, I tested with a purposeful typo and Claude tired to search for files in the directory and failed. Skipping those and logging an error seem like a better way to go from a user experience point of view?

And/or it might make sense to include a validate_allowed_directories tool that the LLM could use to help the user figure out if any directories have issues?

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.

Hi, thanks for the PR, Added a comment below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants