Skip to content

fix(filesystem): let a list_directory_with_sizes stat failure reach the caller - #4911

Open
feiiiiii5 wants to merge 1 commit into
modelcontextprotocol:mainfrom
feiiiiii5:fix/list-directory-with-sizes-propagate-stat
Open

feiiiiii5 wants to merge 1 commit into
modelcontextprotocol:mainfrom
feiiiiii5:fix/list-directory-with-sizes-propagate-stat

Conversation

@feiiiiii5

Copy link
Copy Markdown

Description

list_directory_with_sizes reported a size for entries it could not read. The per-entry fs.stat had a catch that substituted size: 0 and mtime: new Date(0), and formatSize renders 0 as 0 B — so an unreadable file and an empty one come out identical:

$ ls -la            # dangling -> gone
lrwxr-xr-x  1 fei  staff  11 Jan  1 12:00 dangling -> no-such-target

list_directory_with_sizes {path: <dir>}
  [FILE] dangling                        0 B        # <- fabricated
  Total: 3 files, 1 directories
  Combined size: 14 B                     # <- the fabricated 0, added in

Nothing marks the number as unknown, so the listing reads as a complete measurement.

Publishing Your Server

Not applicable — this modifies the existing filesystem server.

Server Details

  • Server: filesystem
  • Changes to: tools (list_directory_with_sizes)

Motivation and Context

size: 0 is a real measurement for an empty file and a fabricated one for a file whose metadata could not be read. The two cases reach the same line, and because the fabricated 0 is summed into Combined size, a directory containing unreadable entries reports a total that is too small rather than one that is incomplete.

A failing fs.stat is not unusual: a file removed between readdir and stat is enough, and a permission-restricted entry does it too.

The fix is to let the error propagate, which is what list_directory and directory_tree already do with their readdir.

How Has This Been Tested?

One case in src/filesystem/__tests__/structured-content.test.ts using a dangling symlink, so readdir lists the entry and fs.stat genuinely fails on it. On main the call succeeds — isError is undefined and the entry is listed as 0 B; on this branch it rejects with ENOENT.

vitest run in the package is 166 passed. The three failures in unicode-paths.test.ts are pre-existing on this machine: macOS resolves os.tmpdir() through the /var → /private/var symlink, and APFS treats the composed and decomposed spellings of café as one name, so the test's second mkdir gets EEXIST. They fail the same way without this change.

Breaking Changes

None to the tool's schema. A directory containing an entry whose metadata cannot be read now answers isError where it previously answered a listing with a 0 B in it.

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

One judgement call worth flagging: the alternative is to keep listing the entry and print its size as unknown rather than 0 B, which keeps a partial listing useful at the cost of an output format change. I went with propagating because it is what the two sibling tools in this server already do, and because a combined size that silently excludes entries is the more dangerous of the two wrong answers. Say the word if you would rather have the unknown-size form — it is a small change from here.

Related, not overlapping: #4910 makes search_files propagate its readdir failure for the same reason. These are separate tools with separate root causes, so I have kept them apart.

…he caller

The per-entry `fs.stat` had a catch that substituted `size: 0` and
`mtime: new Date(0)`. `formatSize` renders 0 as "0 B", so an entry whose
size could not be read came out identical to an empty file, and the
fabricated 0 was added to the combined size the tool reports.

A stat failure is not a measurement, so let it reject. `list_directory`
and `directory_tree` already let their `readdir` reject, and
`search_files` now does the same for its own recursion.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant