Skip to content

Add path utils and test config for filesystem #1405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jun 20, 2025

Conversation

olaservo
Copy link
Member

@olaservo olaservo commented Apr 14, 2025

Description

Separating out the test config and utils from this older PR to simplify the actual changes needed to the server: #543

Plan is to merge this, then the original PR author can update their branch to reference the utils (or create a new one with just those changes).

Server Details

  • Server: filesystem
  • Changes to: tests

Motivation and Context

Original PR includes a large number of changes so the intention is to split it up so we can finally integrate the Windows path fixes into the server.

How Has This Been Tested?

 PASS  __tests__/path-utils.test.ts
  Path Utilities
    convertToWindowsPath
      √ leaves Unix paths unchanged (4 ms)
      √ converts WSL paths to Windows format (1 ms)
      √ converts Unix-style Windows paths to Windows format
      √ leaves Windows paths unchanged but ensures backslashes
      √ handles Windows paths with spaces
      √ handles uppercase and lowercase drive letters
    normalizePath
      √ preserves Unix paths (1 ms)
      √ removes surrounding quotes (1 ms)
      √ normalizes backslashes (1 ms)
      √ converts forward slashes to backslashes on Windows (1 ms)
      √ handles WSL paths
      √ handles Unix-style Windows paths
      √ handles paths with spaces and mixed slashes (1 ms)
      √ preserves spaces in all path formats (1 ms)
      √ handles special characters in paths (1 ms)
    expandHome
      √ expands ~ to home directory (1 ms)
      √ leaves other paths unchanged (1 ms)

Test Suites: 1 passed, 1 total
Tests:       17 passed, 17 total
Snapshots:   0 total
Time:        6.597 s
Ran all test suites.

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

@olaservo olaservo added the server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem label Apr 18, 2025
@olaservo olaservo added the enhancement New feature or request label Jun 16, 2025
@cliffhall
Copy link
Member

The tests all pass locally on macOS. A few observations/questions.

  • Is file-operations.test.ts just meant to test the node.js functionality on both platforms?

  • Is core-functionality.ts meant to test the way the server does certain things by replicating the logic inside the tests?

  • I don't see any external project code that file-operations.test.ts and core-functionality.ts are testing.

    • For instance in file-operations.test.ts there is a test called formatSize helper function which tests a function the test itself defines, but isn't located anywhere else in the project.
  • I turned on coverage locally just to see where we were, and saw that path-utils.test.ts has a couple of untested branches / lines.

Screenshot 2025-06-17 at 1 46 08 PM
  • Gemini suggested these tests to cover those:
   // New test to ensure drive letter capitalization (hits line 67 true, and line 70)
    it('capitalizes lowercase drive letters for Windows paths', () => {
      expect(normalizePath('c:/windows/system32'))
          .toBe('C:\\windows\\system32');
      expect(normalizePath('/mnt/d/my/folder')) // WSL path with lowercase drive
          .toBe('D:\\my\\folder');
      expect(normalizePath('/e/another/folder')) // Unix-style Windows path with lowercase drive
          .toBe('E:\\another\\folder');
    });

    // New test for the 'else' branch of the condition on line 67 (hits line 73)
    it('returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization', () => {
      // Relative path
      const relativePath = 'some/relative/path';
      expect(normalizePath(relativePath)).toBe(path.normalize(relativePath));

      // UNC Path (should not match the regex on line 67)
      const uncPath = '\\\\SERVER\\share\\folder';
      // Expect the result of path.normalize on the UNC path
      expect(normalizePath(uncPath)).toBe(path.normalize(uncPath));

      // A path that looks somewhat absolute but isn't a drive or recognized Unix root for Windows conversion
      const otherAbsolutePath = '\\someserver\\share\\file';
      expect(normalizePath(otherAbsolutePath)).toBe(path.normalize(otherAbsolutePath));
    });

However, that last test failed with:

 Path Utilities › normalizePath › returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization

    expect(received).toBe(expected) // Object.is equality

    Expected: "\\\\SERVER\\share\\folder"
    Received: "\\SERVER\\share\\folder"

      149 |       const uncPath = '\\\\SERVER\\share\\folder';
      150 |       // Expect the result of path.normalize on the UNC path
    > 151 |       expect(normalizePath(uncPath)).toBe(path.normalize(uncPath));
          |                                      ^
      152 |
      153 |       // A path that looks somewhat absolute but isn't a drive or recognized Unix root for Windows conversion
      154 |       const otherAbsolutePath = '\\someserver\\share\\file';

      at Object.<anonymous> (__tests__/path-utils.test.ts:151:38)

When I asked it for a fix, it pointed out a possible flaw

The test failure indicates that your normalizePath function is altering UNC paths (like \\\\SERVER\\share\\folder) in a way that differs from the standard path.normalize() behavior on your system. Specifically, normalizePath seems to be reducing the leading \\\\ to \\.

This is likely due to the line p = p.replace(/\\\\/g, '\\'); in your normalizePath function in src/filesystem/path-utils.ts. This replacement is too general and affects the leading double backslash of UNC paths.

To fix this and ensure UNC paths are handled correctly (preserving their \\\\ prefix), you should modify the normalizePath function to be more specific when replacing double backslashes.

The fix it suggested for path-utils.ts was

Screenshot 2025-06-17 at 2 02 49 PM
// Convert WSL or Unix-style Windows paths to Windows format
 p = convertToWindowsPath(p);

 // Handle double backslashes, preserving leading UNC \\
 if (p.startsWith('\\\\')) {
   // For UNC paths, normalize double backslashes *after* the leading marker
   const restOfPath = p.substring(2).replace(/\\\\/g, '\\');
   p = '\\\\' + restOfPath;
 } else {
   // For non-UNC paths, normalize all double backslashes
   p = p.replace(/\\\\/g, '\\');
}

With those tests added and the fix above in place, 100% coverage on path-utils.ts
Screenshot 2025-06-17 at 2 05 21 PM

…age, remove problematic test files

- Fixed UNC path handling bug in normalizePath function to preserve leading double backslashes
- Added comprehensive test coverage for drive letter capitalization and UNC paths
- Removed file-operations.test.ts and core-functionality.test.ts as they were testing their own code rather than actual server functionality
- All path-utils tests now pass with 100% coverage of the actual utility functions
@olaservo
Copy link
Member Author

The tests all pass locally on macOS. A few observations/questions.

  • Is file-operations.test.ts just meant to test the node.js functionality on both platforms?

  • Is core-functionality.ts meant to test the way the server does certain things by replicating the logic inside the tests?

  • I don't see any external project code that file-operations.test.ts and core-functionality.ts are testing.

    • For instance in file-operations.test.ts there is a test called formatSize helper function which tests a function the test itself defines, but isn't located anywhere else in the project.
  • I turned on coverage locally just to see where we were, and saw that path-utils.test.ts has a couple of untested branches / lines.

Thanks for checking this out, I originally created the PR to add tests for another PR that someone else had opened related to better Windows handling: #543

I think things got weird with the state of the PR and I obviously didn't look closely enough at what part of this was testing, I appreciate that you did. I think its all good now?

@cliffhall
Copy link
Member

Those changes were a bit different from the suggested ones that passed locally.

I'm getting these errors now:

 FAIL  __tests__/path-utils.test.ts
  Path Utilities
    convertToWindowsPath
      ✓ leaves Unix paths unchanged (1 ms)
      ✓ converts WSL paths to Windows format
      ✓ converts Unix-style Windows paths to Windows format
      ✓ leaves Windows paths unchanged but ensures backslashes
      ✓ handles Windows paths with spaces
      ✓ handles uppercase and lowercase drive letters
    normalizePath
      ✓ preserves Unix paths
      ✓ removes surrounding quotes
      ✓ normalizes backslashes
      ✓ converts forward slashes to backslashes on Windows
      ✓ handles WSL paths
      ✓ handles Unix-style Windows paths (1 ms)
      ✓ handles paths with spaces and mixed slashes
      ✓ preserves spaces in all path formats
      ✓ handles special characters in paths
      ✓ capitalizes lowercase drive letters for Windows paths (1 ms)
      ✕ handles UNC paths correctly (1 ms)
      ✕ returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization (1 ms)
    expandHome
      ✓ expands ~ to home directory
      ✓ leaves other paths unchanged

  ● Path Utilities › normalizePath › handles UNC paths correctly

    expect(received).toBe(expected) // Object.is equality

    Expected: "\\\\SERVER\\share\\folder"
    Received: "\\\\\\SERVER\\share\\folder"

      142 |       // Test UNC path with double backslashes that need normalization
      143 |       const uncPathWithDoubles = '\\\\\\\\SERVER\\\\share\\\\folder';
    > 144 |       expect(normalizePath(uncPathWithDoubles)).toBe('\\\\SERVER\\share\\folder');
          |                                                 ^
      145 |     });
      146 |
      147 |     it('returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization', () => {

      at Object.<anonymous> (__tests__/path-utils.test.ts:144:49)

  ● Path Utilities › normalizePath › returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization

    expect(received).toBe(expected) // Object.is equality

    Expected: "some\\relative\\path"
    Received: "some/relative/path"

      148 |       // Relative path
      149 |       const relativePath = 'some/relative/path';
    > 150 |       expect(normalizePath(relativePath)).toBe(relativePath.replace(/\//g, '\\'));
          |                                           ^
      151 |
      152 |       // A path that looks somewhat absolute but isn't a drive or recognized Unix root for Windows conversion
      153 |       const otherAbsolutePath = '\\someserver\\share\\file';

      at Object.<anonymous> (__tests__/path-utils.test.ts:150:43)

Analysis of the current failures:

  1. Test: handles UNC paths correctly:
  • Input: \\\\\\\\SERVER\\\\share\\\\folder (excessive leading and internal backslashes)
  • Expected: \\\\SERVER\\share\\folder
  • Received: \\\SERVER\share\folder (one too many leading backslashes)
  • The current UNC handling logic in normalizePath seems to be miscounting or incorrectly re-adding a leading backslash when path.normalize interacts with multiple initial backslashes, or the subsequent regex replacement isn't quite right for all edge cases of path.normalize's output.
  1. Test: returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization (for relative paths):
  • Input: some/relative/path
  • Expected: some\\relative\\path (Windows-style, as per the test's expectation)
  • Received: some/relative/path
  • The function currently returns path.normalize('some/relative/path'). On a Unix-like system (or a Node environment where path.normalize preserves forward slashes for relative paths), this will be some/relative/path. The final block that converts / to \ only triggers if the path already looks like a Windows absolute path (e.g., starts with a drive letter). The test implies that if a path is not an explicit "Unix path" (as per isUnixPath), it should be treated as a Windows path, including converting slashes in relative paths.

I'll add the suggestions to path-utils.ts that make the tests pass.

@olaservo
Copy link
Member Author

Those changes were a bit different from the suggested ones that passed locally.

I'm getting these errors now:

I made some updates and ran them on both Mac OS and Windows this time:

Screenshot 2025-06-19 at 9 31 45 PM

image

@olaservo olaservo changed the title Add working test config for filesystem Add path utils and test config for filesystem Jun 20, 2025
- Added test job that runs before build
- Tests are conditionally executed only for packages that have test scripts
- Filesystem tests will now run automatically in CI on every push and PR
- Build job now depends on successful test completion
@olaservo
Copy link
Member Author

Added checks for Typescript servers to run tests if found, here is an example run: https://github.com/modelcontextprotocol/servers/actions/runs/15780741769/job/44485530850?pr=1405

@cliffhall
Copy link
Member

cliffhall commented Jun 20, 2025

Screenshot 2025-06-20 at 3 50 45 PM

Tests ran, but we lost coverage. Did those previously suggested changes fail on Windows?

olaservo and others added 2 commits June 20, 2025 13:54
@olaservo olaservo requested a review from cliffhall June 20, 2025 21:02
Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to get a big win in for the day, but would be good to get coverage on line 76 of the path-utils.ts at some point. This is a massive improvement in confidence for the filesystem server.

@olaservo olaservo merged commit 0a2dbd3 into modelcontextprotocol:main Jun 20, 2025
15 checks passed
PazerOP referenced this pull request in PazerOP/mcp-template Jul 15, 2025
Add path utils and test config for filesystem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants