From 8102fbe27a92535754cf44d42987af150b08f105 Mon Sep 17 00:00:00 2001 From: nulone <115600674+nulone@users.noreply.github.com> Date: Fri, 28 Aug 2026 09:17:22 +0700 Subject: [PATCH 1/4] fix(filesystem): handle negative values in formatSize() (#3231) Return '0 B' for negative byte values instead of 'NaN B'. File sizes cannot be negative, so this is a safe default. Co-authored-by: Claude Opus 4.5 --- src/filesystem/__tests__/lib.test.ts | 6 ++++-- src/filesystem/lib.ts | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index e0ae61224f..da1adf2391 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -65,8 +65,10 @@ describe('Lib Functions', () => { }); it('handles negative numbers', () => { - // Negative numbers will result in NaN for the log calculation - expect(formatSize(-1024)).toContain('NaN'); + // Negative numbers should return '0 B' as file sizes cannot be negative + expect(formatSize(-1)).toBe('0 B'); + expect(formatSize(-1024)).toBe('0 B'); + expect(formatSize(-1000000)).toBe('0 B'); expect(formatSize(-0)).toBe('0 B'); }); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index ce4af9f38a..57b93a043e 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -43,7 +43,7 @@ export interface SearchResult { // Pure Utility Functions export function formatSize(bytes: number): string { const units = ['B', 'KB', 'MB', 'GB', 'TB']; - if (bytes === 0) return '0 B'; + if (bytes <= 0) return '0 B'; const i = Math.floor(Math.log(bytes) / Math.log(1024)); From 781a93db14c430b9410d89910f42a65a3c9d0c72 Mon Sep 17 00:00:00 2001 From: 1060996408 Date: Fri, 28 Aug 2026 10:17:27 +0800 Subject: [PATCH 2/4] fix(everything): remove dead code in template URI validation and subscription cleanup (#4104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resources/templates.ts: - `parseResourceId` had a guard that compared the URI against both `textUriBase` and `blobUriBase` with `&&`. Those prefixes are mutually exclusive, so the condition is always false and the branch is dead. Drop it; the SDK's template-based routing already guarantees the URI prefix is one of the two before the handler runs. The remaining positive-integer check on `resourceId` is preserved. resources/subscriptions.ts: - `sendSimulatedResourceUpdates` had an `else` branch that called `subscribers.delete(sessionId)` whenever the session wasn't in a URI's subscriber set, with a comment claiming the session had disconnected. That conclusion doesn't follow — a session not subscribed to URI A can still be subscribed to URI B — and the delete is a no-op when the element is absent anyway. Remove the branch. No behavioral change. Existing 95 / 95 tests in `__tests__` still pass. Co-authored-by: Jia Xuan <1060996408+jiaxuan@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) --- src/everything/resources/subscriptions.ts | 9 ++---- src/everything/resources/templates.ts | 37 ++++++++++------------- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/everything/resources/subscriptions.ts b/src/everything/resources/subscriptions.ts index 2a5e57460f..854a8633a2 100644 --- a/src/everything/resources/subscriptions.ts +++ b/src/everything/resources/subscriptions.ts @@ -99,10 +99,9 @@ export const setSubscriptionHandlers = (server: McpServer) => { /** * Sends simulated resource update notifications to the subscribed client. * - * This function iterates through all resource URIs stored in the subscriptions - * and checks if the specified session ID is subscribed to them. If so, it sends - * a notification through the provided server. If the session ID is no longer valid - * (disconnected), it removes the session ID from the list of subscribers. + * Iterates the URIs in `subscriptions` and emits a + * `notifications/resources/updated` notification for each URI the given + * sessionId is subscribed to. * * @param {McpServer} server - The server instance used to send notifications. * @param {string | undefined} sessionId - The session ID of the client to check for subscriptions. @@ -122,8 +121,6 @@ const sendSimulatedResourceUpdates = async ( method: "notifications/resources/updated", params: { uri }, }); - } else { - subscribers.delete(sessionId); // subscriber has disconnected } } }; diff --git a/src/everything/resources/templates.ts b/src/everything/resources/templates.ts index 6d4903f74c..4530741c54 100644 --- a/src/everything/resources/templates.ts +++ b/src/everything/resources/templates.ts @@ -127,31 +127,26 @@ export const blobResourceUri = (resourceId: number) => new URL(`${blobUriBase}/${resourceId}`); /** - * Parses the resource identifier from the provided URI and validates it - * against the given variables. Throws an error if the URI corresponds - * to an unknown resource or if the resource identifier is invalid. + * Parses the resource identifier from the provided variables and validates + * that it is a positive integer. * - * @param {URL} uri - The URI of the resource to be parsed. - * @param {Record} variables - A record containing context-specific variables that include the resourceId. - * @returns {number} The parsed and validated resource identifier as an integer. - * @throws {Error} Throws an error if the URI matches unsupported base URIs or if the resourceId is invalid. + * The SDK only routes URIs that match the registered template to the + * resource handler, so by the time we get here the URI prefix is already + * known to be one of `textUriBase` / `blobUriBase`. Only the resourceId + * variable still needs validating. + * + * @param {URL} uri - The URI of the resource (used in the error message). + * @param {Record} variables - Context variables including resourceId. + * @returns {number} The parsed and validated resource identifier as a positive integer. + * @throws {Error} If the resourceId is not a finite positive integer. */ const parseResourceId = (uri: URL, variables: Record) => { - const uriError = `Unknown resource: ${uri.toString()}`; - if ( - uri.toString().startsWith(textUriBase) && - uri.toString().startsWith(blobUriBase) - ) { - throw new Error(uriError); - } else { - const idxStr = String((variables as any).resourceId ?? ""); - const idx = Number(idxStr); - if (Number.isFinite(idx) && Number.isInteger(idx) && idx > 0) { - return idx; - } else { - throw new Error(uriError); - } + const idxStr = String((variables as any).resourceId ?? ""); + const idx = Number(idxStr); + if (Number.isFinite(idx) && Number.isInteger(idx) && idx > 0) { + return idx; } + throw new Error(`Unknown resource: ${uri.toString()}`); }; /** From 71e3cfbd1e7e0ff88fa24ce5406f8e220ceae5c4 Mon Sep 17 00:00:00 2001 From: Ryan Lopopolo Date: Thu, 27 Aug 2026 19:17:32 -0700 Subject: [PATCH 3/4] test(filesystem): add MCP SDK regression coverage for directory_tree (#3245) * test(filesystem): add directory_tree MCP SDK regression coverage * docs(filesystem): drop troubleshooting note from README --- .../__tests__/directory-tree.mcp-sdk.test.ts | 57 +++++++++++++++++++ .../__tests__/structured-content.test.ts | 4 +- 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 src/filesystem/__tests__/directory-tree.mcp-sdk.test.ts diff --git a/src/filesystem/__tests__/directory-tree.mcp-sdk.test.ts b/src/filesystem/__tests__/directory-tree.mcp-sdk.test.ts new file mode 100644 index 0000000000..0cbe48c65c --- /dev/null +++ b/src/filesystem/__tests__/directory-tree.mcp-sdk.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs/promises'; +import * as path from 'path'; +import * as os from 'os'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; + +describe('directory_tree MCP SDK regression', () => { + let client: Client; + let transport: StdioClientTransport; + let testDir: string; + + beforeEach(async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-fs-tree-')); + testDir = await fs.realpath(tmp); + + await fs.writeFile(path.join(testDir, 'root.txt'), 'root'); + await fs.mkdir(path.join(testDir, 'nested')); + await fs.writeFile(path.join(testDir, 'nested', 'child.txt'), 'child'); + + const serverPath = path.resolve(__dirname, '../dist/index.js'); + transport = new StdioClientTransport({ + command: 'node', + args: [serverPath, testDir], + }); + + client = new Client( + { name: 'directory-tree-regression-test', version: '1.0.0' }, + { capabilities: {} }, + ); + + await client.connect(transport); + }); + + afterEach(async () => { + await client?.close(); + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it('returns structuredContent.content as a string (not an array) when called via MCP SDK', async () => { + const result = await client.callTool({ + name: 'directory_tree', + arguments: { path: testDir }, + }); + + // Regression test for issues where structuredContent was returned as an array, + // which causes MCP SDK validation to throw -32602 (invalid structured content). + const structured = result.structuredContent as { content: unknown }; + expect(structured).toBeDefined(); + expect(typeof structured.content).toBe('string'); + expect(Array.isArray(structured.content)).toBe(false); + + const parsed = JSON.parse(structured.content as string); + expect(Array.isArray(parsed)).toBe(true); + expect(parsed.length).toBeGreaterThan(0); + }); +}); diff --git a/src/filesystem/__tests__/structured-content.test.ts b/src/filesystem/__tests__/structured-content.test.ts index 4605b72a8f..67da16f901 100644 --- a/src/filesystem/__tests__/structured-content.test.ts +++ b/src/filesystem/__tests__/structured-content.test.ts @@ -22,7 +22,9 @@ describe('structuredContent schema compliance', () => { beforeEach(async () => { // Create a temp directory for testing - testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-fs-test-')); + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-fs-test-')); + // macOS temp dirs may be symlinked (/var -> /private/var); resolve for server allowlist checks. + testDir = await fs.realpath(tmpDir); // Create test files await fs.writeFile(path.join(testDir, 'test.txt'), 'test content'); From 562feeb2817266e0d155b9e4bff54a0233557f12 Mon Sep 17 00:00:00 2001 From: drudiger Date: Thu, 27 Aug 2026 19:36:45 -0700 Subject: [PATCH 4/4] fix(filesystem): preserve file permissions during write and edit operations (#4115) * fix(filesystem): preserve file permissions during write and edit operations The atomic write pattern (write temp file + rename) replaces the original inode, causing the new file to have default 0644 permissions regardless of what the original file had. This breaks executable scripts and other files with non-default permissions. Fix: capture stat.mode before writing and restore it with chmod after rename. Fixes both writeFileContent() and applyFileEdits(). * fix(filesystem): mask chmod mode and keep chmod failure from failing the write Pass origStats.mode & 0o777 to chmod instead of the full st_mode. Move the chmod out of the try whose catch unlinks the temp file and rethrows, since rename has already succeeded by then. Add a test that an EPERM from chmod does not reject the write. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018Wo28CHPXyM3DHoLWvKnCK --------- Co-authored-by: Dustin Rudiger Co-authored-by: olaservo Co-authored-by: Claude Fable 5 --- src/filesystem/__tests__/lib.test.ts | 53 ++++++++++++++++++++++++++++ src/filesystem/lib.ts | 16 +++++++++ 2 files changed, 69 insertions(+) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index da1adf2391..1da741f0b0 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -310,6 +310,33 @@ describe('Lib Functions', () => { expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' }); }); + + it('preserves file permissions when overwriting existing file', async () => { + // First writeFile call with 'wx' flag fails because file exists + mockFs.writeFile.mockRejectedValueOnce(Object.assign(new Error('EEXIST'), { code: 'EEXIST' })); + // stat returns executable permissions + mockFs.stat.mockResolvedValueOnce({ mode: 0o100755 }); + // Second writeFile (to temp) succeeds + mockFs.writeFile.mockResolvedValueOnce(undefined); + mockFs.rename.mockResolvedValueOnce(undefined); + mockFs.chmod.mockResolvedValueOnce(undefined); + + await writeFileContent('/test/script.sh', 'new content'); + + expect(mockFs.stat).toHaveBeenCalledWith('/test/script.sh'); + expect(mockFs.chmod).toHaveBeenCalledWith('/test/script.sh', 0o755); + }); + + it('does not fail the write when chmod fails', async () => { + mockFs.writeFile.mockRejectedValueOnce(Object.assign(new Error('EEXIST'), { code: 'EEXIST' })); + mockFs.stat.mockResolvedValueOnce({ mode: 0o100755 }); + mockFs.writeFile.mockResolvedValueOnce(undefined); + mockFs.rename.mockResolvedValueOnce(undefined); + mockFs.chmod.mockRejectedValueOnce(Object.assign(new Error('EPERM'), { code: 'EPERM' })); + + await expect(writeFileContent('/test/script.sh', 'new content')).resolves.toBeUndefined(); + expect(mockFs.unlink).not.toHaveBeenCalled(); + }); }); }); @@ -418,6 +445,8 @@ describe('Lib Functions', () => { beforeEach(() => { mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); mockFs.writeFile.mockResolvedValue(undefined); + mockFs.stat.mockResolvedValue({ mode: 0o100644 }); + mockFs.chmod.mockResolvedValue(undefined); }); it('applies simple text replacement', async () => { @@ -512,6 +541,30 @@ describe('Lib Functions', () => { ); }); + it('preserves file permissions after applying edits', async () => { + mockFs.stat.mockResolvedValue({ mode: 0o100755 }); + const edits = [ + { oldText: 'line2', newText: 'modified line2' } + ]; + + mockFs.rename.mockResolvedValueOnce(undefined); + + await applyFileEdits('/test/script.sh', edits, false); + + expect(mockFs.stat).toHaveBeenCalledWith('/test/script.sh'); + expect(mockFs.chmod).toHaveBeenCalledWith('/test/script.sh', 0o755); + }); + + it('does not restore permissions in dry run mode', async () => { + const edits = [ + { oldText: 'line2', newText: 'modified line2' } + ]; + + await applyFileEdits('/test/file.txt', edits, true); + + expect(mockFs.chmod).not.toHaveBeenCalled(); + }); + it('throws error for non-matching edits', async () => { const edits = [ { oldText: 'nonexistent line', newText: 'replacement' } diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 57b93a043e..a1c6f04b67 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -168,6 +168,7 @@ export async function writeFileContent(filePath: string, content: string): Promi // Security: Use atomic rename to prevent race conditions where symlinks // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. + const origStats = await fs.stat(filePath); const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, content, 'utf-8'); @@ -178,6 +179,13 @@ export async function writeFileContent(filePath: string, content: string): Promi } catch {} throw renameError; } + // Restore original permission bits since the atomic rename replaces the + // inode and the temp file has default (0644) permissions. Mask off the + // file-type bits; POSIX leaves them unspecified for chmod. A chmod + // failure must not fail the write, which has already succeeded. + try { + await fs.chmod(filePath, origStats.mode & 0o777); + } catch {} } else { throw error; } @@ -266,6 +274,7 @@ export async function applyFileEdits( // Security: Use atomic rename to prevent race conditions where symlinks // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. + const origStats = await fs.stat(filePath); const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, modifiedContent, 'utf-8'); @@ -276,6 +285,13 @@ export async function applyFileEdits( } catch {} throw error; } + // Restore original permission bits since the atomic rename replaces the + // inode and the temp file has default (0644) permissions. Mask off the + // file-type bits; POSIX leaves them unspecified for chmod. A chmod + // failure must not fail the write, which has already succeeded. + try { + await fs.chmod(filePath, origStats.mode & 0o777); + } catch {} } return formattedDiff;