Skip to content

fix(filesystem): name a broken symlink instead of blaming its parent - #4915

Open
v0ropaev wants to merge 1 commit into
modelcontextprotocol:mainfrom
v0ropaev:fix/broken-symlink-message
Open

v0ropaev wants to merge 1 commit into
modelcontextprotocol:mainfrom
v0ropaev:fix/broken-symlink-message

Conversation

@v0ropaev

Copy link
Copy Markdown

Description

A symlink pointing at nothing makes the filesystem server report a missing parent directory for a directory that is plainly there.

Server Details

  • Server: filesystem
  • Changes to: path validation (src/filesystem/lib.ts), one new test file

Motivation and Context

validatePath catches ENOENT out of resolveUnicodeEquivalentPath and turns it into

Parent directory does not exist: <dirname>

That message never fits the case it names. When a path component is genuinely missing, the resolver does not throw: it returns the joined tail, with a comment saying so, precisely so create_directory can mkdir -p it. What does raise ENOENT in there is fs.realpath on an entry that appears in the directory listing but points nowhere.

So with an allowed directory R holding a dangling link.txt:

Parent directory does not exist: /var/folders/.../mcp-broken-symlink-N9OgbP

R exists, fs.stat(R) succeeds, and the caller is sent looking at the wrong thing. A stale node_modules/.bin entry gives the same shape.

The fix classifies the error where it happens, at the realpath call, rather than guessing from an errno two frames up:

Broken symlink: <path>

I left the existing fallback in place. The other ENOENT sources in that function are a vanished allowed directory and an intermediate directory removed mid-walk, where "parent directory does not exist" reads closer to true.

I first tried putting an fs.stat(parent) check in the catch to tell the two apart. That is worse: __tests__/lib.test.ts auto-mocks fs/promises, so fs.stat resolves to undefined and the check passes for both cases. Classifying at the source needs no such guess.

How Has This Been Tested?

New src/filesystem/__tests__/broken-symlink.test.ts, two cases against a real temp directory: a dangling symlink now reports Broken symlink while its parent still stats fine, and a genuinely missing path still resolves to the joined tail so create_directory keeps working. The first fails on current main with the message quoted above.

npx vitest run     167 passed, 3 failed
npx tsc --noEmit   clean

The three failures are in __tests__/unicode-paths.test.ts and are the same three on unmodified main in this checkout; they look like the composed/decomposed distinction not surviving on APFS. Not touched by this change.

Not tested through an LLM client, this is an error-message path.

Breaking Changes

None. No client configuration changes, and the only difference a caller sees is the wording of an error that was already being raised.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking 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 (no user-facing behaviour to document, only the error text)
  • 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 (none added)

validatePath catches ENOENT out of resolveUnicodeEquivalentPath and
reports "Parent directory does not exist". That message never fits the
case it names: when a component is genuinely missing the resolver returns
the joined tail rather than throwing, so create_directory can mkdir -p it.

What does throw ENOENT in there is fs.realpath on an entry that is in the
directory listing but points nowhere. So a dangling symlink reports a
missing parent for a directory that is right there:

    Parent directory does not exist: /var/folders/.../mcp-broken-symlink-N9OgbP

Classify it where it happens, at the realpath call, rather than guessing
from an errno two frames up. The existing fallback is left for the other
ENOENT sources in that function, a vanished allowed directory or an
intermediate directory removed mid-walk, where it reads closer to true.
Copilot AI balanced review requested due to automatic review settings September 30, 2026 09:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

2 participants