added read_binary_file tool for file system MCP server

[robertyates]

added a read_binary_file tool to the file system MCP Server

Description

Server Details

• Server: filesystem
• Changes to: added read_binary_file tool

Motivation and Context

I was trying to use the existing server to fetch an excel file and it fails

How Has This Been Tested?

I have a custom agent that this works in and also include a unit test

Breaking Changes

pure additive, although read_file should likely now be removed vs. just being marked deprecated

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

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 49s —— View job

---

PR Review: read_binary_file tool for filesystem server

• Gather context and understand request
• Read changed files (index.ts, test file, README.md)
• Review implementation quality, security, and correctness
• Check test coverage and style
• Verify alignment with repo conventions
• Post final review

---

The core concept is sound — using embedded resources (type: "resource" with blob) is the correct MCP protocol approach for binary files, and it's a real gap that the existing read_file/read_text_file tools don't handle binary content well. However, there are several issues to address.

---

Issues

1. Regression in read_media_file (unrelated change, breaking behavior)

src/filesystem/index.ts lines 269–275 change the fallback content type from "blob" to "image" for non-image, non-audio MIME types:

-    // Fallback for other binary types, not officially supported by the spec but has been used for some time
-    : "blob";
-const contentItem = { type: type as 'image' | 'audio' | 'blob', data, mimeType };
+    : "image"; // Use "image" for all other binary types as fallback
+const contentItem = {
+  type: type as 'image' | 'audio',
+  data,
+  mimeType
+};

This is a silent breaking change to an existing tool: clients that relied on the "blob" type fallback (or sent Excel/PDF to read_media_file) will now receive type: "image" for those files, which is semantically incorrect. This unrelated modification should either be reverted or documented as a separate change.

Fix this →

2. Windows URI construction bug

src/filesystem/index.ts line 358:

const uri = `file://${validPath}`;

On Windows, validPath looks like C:\path\to\file.xlsx, which produces file://C:\path\to\file.xlsx — an invalid URI. Use the Node built-in:

import { pathToFileURL } from 'url';
const uri = pathToFileURL(validPath).toString();

Fix this →

3. MIME type table duplicated

The MIME type map is copied verbatim from read_media_file (with additions). This should be extracted to a shared constant or utility function to avoid the tables drifting out of sync over time.

Fix this →

4. Input schema reuse is misleading

src/filesystem/index.ts line 312:

async (args: z.infer<typeof ReadMediaFileArgsSchema>): Promise<CallToolResult> => {

The read_binary_file handler is typed with ReadMediaFileArgsSchema, which is semantically for read_media_file. Define a separate ReadBinaryFileArgsSchema (even if identical) for clarity and future independence.

5. Artifact in test file

src/filesystem/__tests__/read-binary-file.test.ts line 232:

// Made with Bob

This leftover comment should be removed before merging.

Fix this →

6. Array<any> in tests

src/filesystem/__tests__/read-binary-file.test.ts lines 98, 132, 148, etc. use Array<any>, which undermines type safety. The SDK's CallToolResult type should be used, or at minimum a typed interface for the expected content shape.

7. No test for the read_media_file change

The modification to read_media_file (changing the fallback type) is not covered by any test. Since this PR modifies an existing tool, there should either be a test confirming the new behavior or the change should be reverted.

---

Minor Notes

• Tool overlap: read_binary_file and read_media_file now both handle images and audio files. The descriptions should clearly differentiate them — read_media_file returns inline image/audio content (for models that can process media natively), while read_binary_file returns an embedded resource blob (for clients that support resource handling). This distinction matters for LLM clients.
• No file size guard: readFileAsBase64Stream buffers the entire file in memory before base64-encoding (via Buffer.concat). This is fine for most use cases but could be noted in the description as a consideration for very large files.
• read_binary_file description: Calling it "the recommended way to read binary files" is a bit strong given that read_media_file already exists for images/audio. Consider scoping the claim to document types and archives.

---

Summary

The embedded resource approach for read_binary_file is correct and fills a real protocol gap. The main blockers are: the unintended regression in read_media_file, the Windows URI bug, and the test artifact. The MIME type duplication and type annotation issues are worth fixing before merge.