diff --git a/src/filesystem/__tests__/startup-validation.test.ts b/src/filesystem/__tests__/startup-validation.test.ts index 3be283df74..93d421e3c4 100644 --- a/src/filesystem/__tests__/startup-validation.test.ts +++ b/src/filesystem/__tests__/startup-validation.test.ts @@ -97,4 +97,73 @@ 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 — 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 + // 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); }