fix(filesystem): ensure bare Windows drive letters normalize to root#3434
Conversation
Previously,
ormalizePath('C:') returned C:. on Windows, which could break path validation.
By appending a separator before normalization for bare drive letters, we ensure they
consistently resolve to the drive root (e.g., C:\).
Fixes modelcontextprotocol#3418
olaservo
left a comment
olaservo
left a comment
There was a problem hiding this comment.
The fix itself is correct and well-placed — nice work! The regex ^[a-zA-Z]:$ precisely matches bare drive letters, the process.platform guard keeps it inert on non-Windows, and the placement after backslash normalization but before path.normalize() is exactly right.
However, there's one issue that needs to be fixed before this can merge:
UTF-8 BOM introduced on line 1
The diff shows:
-import path from "path";
+import path from "path";
The replacement line starts with U+FEFF (UTF-8 Byte Order Mark), which wasn't in the original file. This was likely introduced by your editor. It will pollute git blame and is inconsistent with the rest of the codebase. Please re-save src/filesystem/path-utils.ts as UTF-8 without BOM and force-push.
Optional suggestion: Consider adding a test case for the bare drive letter fix in src/filesystem/__tests__/path-utils.test.ts, using the platform-mocking pattern already established in the existing WSL tests.
Once the BOM is removed, this is a clean merge. Thanks for the contribution!
Reviewed with the assistance of Claude Code (claude-opus-4-6). Claims were independently verified against the Node.js test suite and source code.
|
Addressed, thanks. I removed the UTF-8 BOM from \src/filesystem/path-utils.ts\ and force-pushed the branch. I also added a test covering bare Windows drive-letter normalization to the drive root on Windows. Verified with: \ |
olaservo
left a comment
olaservo
left a comment
There was a problem hiding this comment.
Both issues from the initial review are addressed:
- BOM removed — verified at the byte level, file now starts cleanly with
import - Test added —
normalizes bare Windows drive letters to the drive root on WindowscoversC:→C:\andd:→D:\, correctly placed inside theWSL path handlingdescribe block which hasafterEachplatform restoration. Test passes on CI.
Fix logic is correct: precise regex (^[a-zA-Z]:$), platform-guarded, placed after backslash normalization and before path.normalize().
LGTM — thanks for the quick update @Chelebii!
Reviewed with the assistance of Claude Code (claude-opus-4-6). BOM removal verified via raw byte inspection; test correctness verified against CI results.
2 participants
Description
Currently,
ormalizePath('C:') returns C:. on Windows instead of C:. This behavior comes from Node's path.normalize() which interprets a bare drive letter as the current directory on that drive. This can break downstream path validation logic in the filesystem MCP server.
This PR ensures that bare Windows drive letters (e.g., C:, D:) are consistently normalized to the drive root by appending a path separator before calling path.normalize().
Key Changes
ormalizePath in src/filesystem/path-utils.ts to detect bare drive letters on Windows.
Related Issues
Fixes #3418