Skip to content

Fix filesystem server crashes on invalid paths with graceful validation #2603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebastien-rosset wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix filesystem server crashes on invalid paths with graceful validation #2603

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions src/filesystem/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ if (args.length === 0) {
console.error(" 1. Command-line arguments (shown above)");
console.error(" 2. MCP roots protocol (if client supports it)");
console.error("At least one directory must be provided by EITHER method for the server to operate.");
console.error("Note: Directories will be validated at startup but operations will be retried at runtime.");
}

// Store allowed directories in normalized and resolved form
Expand All @@ -59,22 +60,33 @@ let allowedDirectories = await Promise.all(
})
);

// Validate that all directories exist and are accessible
await Promise.all(allowedDirectories.map(async (dir) => {
try {
const stats = await fs.stat(dir);
if (!stats.isDirectory()) {
console.error(`Error: ${dir} is not a directory`);
process.exit(1);
// Validate directories at startup - log warnings if path is not accessible at startup.
// Directory accessibility may change between startup and runtime.
const validatedDirectories = await Promise.all(
allowedDirectories.map(async (dir) => {
try {
const stats = await fs.stat(dir);
if (stats.isDirectory()) {
console.error(`Directory accessible: ${dir}`);
return dir;
} else if (stats.isFile()) {
console.error(`${dir} is a file, not a directory - skipping`);
return null;
} else {
// Include symlinks/special files - they might become directories when NAS/VPN reconnects
console.error(`${dir} is not a directory (${stats.isSymbolicLink() ? 'symlink' : 'special file'})`);
return dir;
}
} 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

@olaservo olaservo Aug 24, 2025

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?

return dir;
}
} catch (error) {
console.error(`Error accessing directory ${dir}:`, error);
process.exit(1);
}
}));
})
).then(results => results.filter((dir): dir is string => dir !== null));

// Initialize the global allowedDirectories in lib.ts
setAllowedDirectories(allowedDirectories);
setAllowedDirectories(validatedDirectories);

// Schema definitions
const ReadTextFileArgsSchema = z.object({
Expand Down