From e7eb1b5dfb7bdd1c694ac01c99a2f318211442ed Mon Sep 17 00:00:00 2001 From: David Date: Thu, 14 May 2026 04:05:40 +0200 Subject: [PATCH 1/2] fix(filesystem): include error reason + enumerate failed dirs at startup (#4152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the secure-filesystem-server starts, `allowed_directories` entries that fail `fs.stat` are skipped with only `Warning: Cannot access directory , 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 #4152 (per-directory reason + aggregate enumeration); the four pre-existing tests continue to pass unchanged. Co-authored-by: Claude Code --- .../__tests__/startup-validation.test.ts | 40 +++++++++++++++++++ src/filesystem/index.ts | 24 +++++++++-- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/filesystem/__tests__/startup-validation.test.ts b/src/filesystem/__tests__/startup-validation.test.ts index 3be283df74..33b61dac9d 100644 --- a/src/filesystem/__tests__/startup-validation.test.ts +++ b/src/filesystem/__tests__/startup-validation.test.ts @@ -97,4 +97,44 @@ describe('Startup Directory Validation', () => { // Should still start with the valid directory expect(result.stderr).toContain('Secure MCP Filesystem Server running on stdio'); }); + + // Regression coverage for #4152 — warnings about inaccessible startup + // directories must include the underlying error reason so hosts can + // surface an actionable diagnostic to end users (the original report + // saw a silent transport close because nothing identified the bad path + // beyond a generic "Cannot access directory" line). + it('should include the underlying error reason in the per-directory warning (#4152)', async () => { + const nonExistentDir = path.join(testDir, 'non-existent-dir-with-reason'); + + const result = await spawnServer([nonExistentDir, accessibleDir]); + + // Should still skip + continue with the valid directory. + expect(result.stderr).toContain('Secure MCP Filesystem Server running on stdio'); + // Diagnostic must name the directory AND the OS-level error reason. + expect(result.stderr).toContain(nonExistentDir); + expect(result.stderr).toMatch(/ENOENT|no such file or directory/i); + }); + + // Regression coverage for #4152 — when ALL startup directories fail, the + // aggregate exit error must enumerate each offending path AFTER the + // "Error:" marker (not only as scattered per-directory warnings) so a + // host capturing only the final fatal line can show users every entry of + // allowed_directories that needs fixing. + it('should list every offending directory inside the aggregate startup error (#4152)', async () => { + const bad1 = path.join(testDir, 'aggregate-fail-1'); + const bad2 = path.join(testDir, 'aggregate-fail-2'); + + const result = await spawnServer([bad1, bad2]); + + expect(result.exitCode).toBe(1); + + // Slice from the "Error:" marker to end-of-stderr — the aggregate + // message itself must contain both paths, not only the per-directory + // warnings that precede it. + const errIdx = result.stderr.indexOf('Error:'); + expect(errIdx, 'aggregate Error: line missing').toBeGreaterThanOrEqual(0); + const aggregate = result.stderr.slice(errIdx); + expect(aggregate).toContain(bad1); + expect(aggregate).toContain(bad2); + }); }); diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts index 7b67e63e58..735b07fad9 100644 --- a/src/filesystem/index.ts +++ b/src/filesystem/index.ts @@ -66,24 +66,40 @@ let allowedDirectories = (await Promise.all( }) )).flat(); -// Filter to only accessible directories, warn about inaccessible ones +// Filter to only accessible directories, warn about inaccessible ones. +// Track failed entries with their reason so the aggregate fatal error +// (when EVERY directory is unusable) can enumerate them — hosts using the +// stdio transport often surface only the final error line, so per-path +// warnings are not enough. See issue #4152. const accessibleDirectories: string[] = []; +const failedEntries: { dir: string; reason: string }[] = []; for (const dir of allowedDirectories) { try { const stats = await fs.stat(dir); if (stats.isDirectory()) { accessibleDirectories.push(dir); } else { + const reason = "not a directory"; console.error(`Warning: ${dir} is not a directory, skipping`); + failedEntries.push({ dir, reason }); } } catch (error) { - console.error(`Warning: Cannot access directory ${dir}, skipping`); + const reason = error instanceof Error ? error.message : String(error); + console.error(`Warning: Cannot access directory ${dir}, skipping (${reason})`); + failedEntries.push({ dir, reason }); } } -// Exit only if ALL paths are inaccessible (and some were specified) +// Exit only if ALL paths are inaccessible (and some were specified). The +// aggregate error enumerates every failed entry so the host can show the +// user which allowed_directories items to fix (#4152). if (accessibleDirectories.length === 0 && allowedDirectories.length > 0) { - console.error("Error: None of the specified directories are accessible"); + const enumerated = failedEntries + .map(({ dir, reason }) => ` - ${dir} (${reason})`) + .join("\n"); + console.error( + `Error: None of the specified directories are accessible:\n${enumerated}`, + ); process.exit(1); } From cede9e850b87d462ba8b2abaa2575276515e200a Mon Sep 17 00:00:00 2001 From: David Date: Thu, 14 May 2026 04:30:04 +0200 Subject: [PATCH 2/2] test(filesystem): cover permission-denied (EACCES) bucket for #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 --- .../__tests__/startup-validation.test.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/filesystem/__tests__/startup-validation.test.ts b/src/filesystem/__tests__/startup-validation.test.ts index 33b61dac9d..93d421e3c4 100644 --- a/src/filesystem/__tests__/startup-validation.test.ts +++ b/src/filesystem/__tests__/startup-validation.test.ts @@ -115,6 +115,35 @@ describe('Startup Directory Validation', () => { expect(result.stderr).toMatch(/ENOENT|no such file or directory/i); }); + // Regression coverage for #4152 — permission-denied (EACCES) directories + // must surface the OS reason in the warning the same way ENOENT does. The + // original report only mentioned non-existent dirs, but the underlying + // try/catch handles every fs.stat failure and hosts on Windows / locked + // mounts hit EACCES just as often. Skipped on POSIX root (no chmod 0o000 + // restriction) and on Windows (chmod has no effect on directory access). + const skipChmodEacces = process.platform === 'win32' || (typeof process.getuid === 'function' && process.getuid() === 0); + (skipChmodEacces ? it.skip : it)('should include EACCES reason for permission-denied directory (#4152)', async () => { + const lockedDir = path.join(testDir, 'locked-dir'); + await fs.mkdir(lockedDir); + // Drop *all* perms on the parent so fs.stat on a child path returns EACCES. + const parent = path.join(testDir, 'no-traverse'); + await fs.mkdir(parent); + const child = path.join(parent, 'inner'); + await fs.mkdir(child); + await fs.chmod(parent, 0o000); + + try { + const result = await spawnServer([child, accessibleDir]); + // Should still continue with the accessible one. + expect(result.stderr).toContain('Secure MCP Filesystem Server running on stdio'); + expect(result.stderr).toContain(child); + expect(result.stderr).toMatch(/EACCES|permission denied/i); + } finally { + // Restore perms so afterEach() can remove the tree. + await fs.chmod(parent, 0o700).catch(() => {}); + } + }); + // Regression coverage for #4152 — when ALL startup directories fail, the // aggregate exit error must enumerate each offending path AFTER the // "Error:" marker (not only as scattered per-directory warnings) so a