fix(filesystem): merge MCP roots with CLI-provided directories instead of replacing them

[flobo3]

Fixes #3602

Problem

When an MCP client supports the roots protocol, the filesystem server's oninitialized handler unconditionally replaces all command-line allowed directories with only the roots provided by the client. This causes any additional directories passed via CLI arguments to be silently discarded and become inaccessible.

Solution

Modified updateAllowedDirectoriesFromRoots in src/filesystem/index.ts to merge the client-provided roots with the existing allowedDirectories (which contains the CLI-provided directories) using a Set to ensure uniqueness, instead of completely replacing them.

This ensures that both CLI-provided directories and client-provided roots are accessible to the server.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 28s —— View job

---

Code Review

• Gather context
• Read changed file and diff
• Analyze fix for correctness and edge cases
• Post review feedback

---

The fix addresses a real problem and is a clear improvement over the original behavior, but it introduces a subtle security issue in the roots/list_changed path.

What the PR changes

Before (src/filesystem/index.ts:708):

allowedDirectories = [...validatedRootDirs];

After:

const merged = new Set([...allowedDirectories, ...validatedRootDirs]);
allowedDirectories = [...merged];

The updateAllowedDirectoriesFromRoots function is called in two places:

1. oninitialized — once, at startup
2. RootsListChangedNotificationSchema handler — dynamically, whenever the client updates its roots

---

Issue 1: Accumulation bug (security concern)

The merge approach works correctly for case 1 (initialization). For case 2 (dynamic updates), it causes allowedDirectories to grow monotonically — a directory once added via MCP roots can never be removed.

Scenario:

1. Server starts with CLI dir [/projects], client provides roots [/tmp/workspace-A]
   → allowedDirectories = [/projects, /tmp/workspace-A] ✓
2. Client removes /tmp/workspace-A and adds /tmp/workspace-B (fires roots/list_changed)
   → allowedDirectories = [/projects, /tmp/workspace-A, /tmp/workspace-B] ✗

The intent of roots/list_changed is that the client's root list has changed, not that it's been appended to. Keeping revoked roots accessible is a security regression compared to the old behaviour (which at least reflected the client's current intent, just discarding CLI dirs).

Recommended fix: Track CLI-provided directories separately so each roots update cleanly replaces the MCP-roots portion while preserving CLI dirs:

// At the top of the file, save CLI dirs immutably after startup
const cliAllowedDirectories = [...allowedDirectories];

// In updateAllowedDirectoriesFromRoots:
async function updateAllowedDirectoriesFromRoots(requestedRoots: Root[]) {
  const validatedRootDirs = await getValidRootDirectories(requestedRoots);
  if (validatedRootDirs.length > 0) {
    const merged = new Set([...cliAllowedDirectories, ...validatedRootDirs]);
    allowedDirectories = [...merged];
    setAllowedDirectories(allowedDirectories);
    console.error(`Updated allowed directories from MCP roots: ${validatedRootDirs.length} valid directories`);
  } else {
    console.error("No valid root directories provided by client");
  }
}

This way CLI dirs always remain, and MCP roots replace the previous MCP roots on each update. Fix this →

---

Issue 2: Stale comment at line 719

The comment above the RootsListChangedNotificationSchema handler still says:

// Handles dynamic roots updates during runtime, when client sends "roots/list_changed" notification, server fetches the updated roots and replaces all allowed directories with the new roots.

"replaces all allowed directories" is no longer accurate and should be updated to reflect the new merge semantics.

---

Issue 3: No tests for the merge behaviour

__tests__/roots-utils.test.ts covers getValidRootDirectories but there are no tests for updateAllowedDirectoriesFromRoots itself. Tests covering:

• CLI dirs preserved after init with roots
• CLI dirs preserved after roots/list_changed
• Removed roots no longer accessible after roots/list_changed

…would prevent regressions here.

---

Summary

The core intent of this PR is correct — CLI-provided directories should not be silently dropped when an MCP client provides roots. The merge using Set is the right idea. The fix just needs to be applied against the saved CLI dirs snapshot rather than the ever-growing allowedDirectories, to avoid the accumulation problem in the dynamic-update path.

[flobo3]

Thanks for the thorough review! All three points addressed in 65c4a49:

1. Accumulation bug — CLI directories are now saved to an immutable cliAllowedDirectories snapshot after startup filtering. Each updateAllowedDirectoriesFromRoots call merges against this snapshot, so removed MCP roots are properly replaced rather than accumulated.

2. Stale comment — Updated the RootsListChangedNotificationSchema handler comment to reflect the new merge semantics.

3. Tests — Added roots-merge.test.ts covering CLI dir preservation after init, root replacement without accumulation across roots/list_changed, and multiple update cycles.

[LuuOW]

Technical audit: Verified MCP server implementation for consistency with current SDK patterns.