fix(filesystem): include error reason + enumerate failed dirs at startup (#4152)#4156
Open
daveCode-dot wants to merge 2 commits into
Open
Conversation
…tup (modelcontextprotocol#4152) When the secure-filesystem-server starts, `allowed_directories` entries that fail `fs.stat` are skipped with only `Warning: Cannot access directory <dir>, skipping`. When **every** entry fails the server exits with `Error: None of the specified directories are accessible`. Neither line tells the host which underlying error happened (ENOENT, EACCES, …) nor — in the aggregate fatal case — which entries were the culprits, so hosts that surface only the final stderr line (Claude Desktop is the example in the issue) show users an opaque "Server disconnected" toast. This change keeps the existing wording (so dependent tests / log-grep consumers stay valid) and: * Adds the underlying `error.message` to the per-directory warning so e.g. `Cannot access directory /missing, skipping (ENOENT: …)` is immediately actionable. * Tracks failed entries in a `failedEntries` list and enumerates them inside the aggregate fatal error, so a host capturing only that final line sees every offending path with its reason. Two regression tests in `startup-validation.test.ts` pin the new behaviour to issue modelcontextprotocol#4152 (per-directory reason + aggregate enumeration); the four pre-existing tests continue to pass unchanged. Co-authored-by: Claude Code <noreply@anthropic.com>
…ntextprotocol#4152 Adds a regression case alongside the ENOENT one so both common host failure modes (missing dir, locked dir) are pinned. Skipped on Windows (chmod has no effect on dir access there) and when running as root (0o000 doesn't restrict root reads). Co-authored-by: Claude Code <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #4152. The secure-filesystem-server's startup validation already filters inaccessible
allowed_directoriesentries, but the diagnostic surface is too thin: per-directory warnings omit the underlying OS error and the aggregate fatal error doesn't enumerate which paths failed. Hosts that surface only the final stderr line (e.g. Claude Desktop in the original report) end up showing users an opaque "Server disconnected" toast with nothing to act on.This change keeps the existing log strings stable for grep-based consumers and adds the missing reason / enumeration so the diagnostic is actionable.
Repro
Fix
src/filesystem/index.ts— startup validation now tracks failed entries with their reason and:Cannot access directory /missing, skipping (ENOENT: no such file or directory, stat '/missing').The four pre-existing
startup-validation.test.tscases continue to pass unchanged — the legacyWarning: Cannot access directoryandError: None of the specified directories are accessiblesubstrings are preserved.Tests
src/filesystem/__tests__/startup-validation.test.tsadds two regression cases pinned to #4152:should include the underlying error reason in the per-directory warning (#4152)— asserts/ENOENT|no such file or directory/ishows up in stderr alongside the path.should list every offending directory inside the aggregate startup error (#4152)— slices stderr from theError:marker and verifies both failing paths appear in the aggregate, not only as scattered earlier warnings.Local result, ran from
src/filesystem:No regression in the other six test files (
directory-tree,lib,path-utils,path-validation,roots-utils,structured-content).Adjacent gaps (out of scope · raising for maintainer decision)
While auditing this code path I noticed
roots-utils.tsalready exposes aformatDirectoryError(dir, error?, reason?)helper used bygetValidRootDirectories(). There are now two error-formatting styles for the same domain:index.tsstartup:Cannot access directory <dir>, skipping (<reason>)and- <dir> (<reason>)enumerationroots-utils.tsMCP roots:Skipping invalid directory: <dir> due to error: <message>Standardising on a single format (either by exporting
formatDirectoryErrorand adopting it inindex.ts, or vice-versa) would remove this drift, but it changes user-facing log strings and the right shape is a maintainer call rather than a unilateral choice in this PR. Happy to follow up with a separate PR once you indicate which way you'd like to consolidate.Notes
package.json/ version bump — leaving release cadence to the maintainers.failedEntrieslist is built only during startup so there is no runtime cost on the hot path.🤖 Co-authored with Claude Code