Skip to content

fix(vfsevents): fix fsnotify Windows drive-letter path round-trip#345

Open
funkyshu wants to merge 2 commits into
mainfrom
fix/344-fsnotify-windows-drive-letter
Open

fix(vfsevents): fix fsnotify Windows drive-letter path round-trip#345
funkyshu wants to merge 2 commits into
mainfrom
fix/344-fsnotify-windows-drive-letter

Conversation

@funkyshu

Copy link
Copy Markdown
Member

fsnotify: Windows drive-letter file:// URI round-trip produces an invalid path (#344)

The fsnotify watcher's Start() built the path passed to fsnotify.Add() by naively stripping the "file://" prefix off the location's URI (file:///C:/Temp/... -> /C:/Temp/...). For a Windows drive-letter path this leaves a stray leading slash, which Windows rejects:

failed to add watch path: failed to add watch path /C:/Temp/TestDebouncingEdgeCases3938112569/001: GetFileAttributes: The filename, directory name, or volume label syntax is incorrect.

convertEvent() had the mirror problem on the way back out: it built the event's URI by naively prepending "file://" to the native OS path fsnotify reports (C:\Temp\...), producing an invalid URI with backslashes and no leading slash before the drive letter.

Fix

Added vfsPathToNative/nativePathToURI helpers (each with a GOOS-parameterized ...OS variant so the Windows-specific behavior can be unit tested from any platform) that correctly convert between VFS-style paths (forward slashes, leading slash before a drive letter) and native OS paths in both directions. Start() now uses vfsPathToNative before calling fsnotify.Add(), and convertEvent() now uses nativePathToURI when building the event URI.

Tests

  • Re-enabled the fsnotify watcher tests that were temporarily skipped on Windows for this issue in fix: sftp conn-timer data race + enable -race in CI (fixes #338) #343 (TestFSNotifyWatcherTestSuite, TestDebouncing, TestDebouncingEdgeCases, TestEventAnalysis), updating a couple of assertions that previously assumed a native path could be substring/equality-compared against a file:// URI.
  • Added path_test.go with GOOS-parameterized unit tests for the new conversion helpers (drive-letter stripping/re-adding, separator conversion, round-trip) so the Windows code path is verified without needing a Windows runner.

Fixes #344

Made with Cursor

The fsnotify watcher's Start() built the fsnotify.Add() path by naively
stripping the "file://" prefix off the location's URI. For a Windows
drive-letter path this leaves a stray leading slash (e.g. /C:/Temp/...),
which Windows rejects as an invalid path. convertEvent() had the mirror
problem: it built the event's URI by naively prepending "file://" to the
native OS path fsnotify reports, which on Windows produces an invalid URI
with backslashes and no leading slash before the drive letter.

Added vfsPathToNative/nativePathToURI (with GOOS-parameterized variants
for cross-platform unit testing) to properly convert between VFS-style
paths and native OS paths in both directions, and used them in Start()
and convertEvent(). Re-enabled the fsnotify watcher tests that were
temporarily skipped on Windows for this issue.

Co-authored-by: Cursor <cursoragent@cursor.com>
@c2fo-cibot c2fo-cibot Bot added the size/L Denotes a PR that changes 100-499 lines label Jul 17, 2026
@funkyshu
funkyshu requested a review from Copilot July 17, 2026 21:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes Windows drive-letter file:// URI round-tripping in the contrib/vfsevents fsnotify watcher by introducing explicit conversions between VFS-style paths/URIs and native OS paths. This prevents invalid watch paths like /C:/... from reaching fsnotify.Add() on Windows and ensures emitted event URIs are valid file:///C:/... URLs.

Changes:

  • Add vfsPathToNative / nativePathToURI helpers (plus GOOS-parameterized variants) and use them in Start() and convertEvent().
  • Re-enable and update fsnotify watcher tests to compare against normalized file:// URIs across platforms.
  • Add unit tests for the new path conversion helpers and record the fix in the contrib changelog.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
contrib/vfsevents/watchers/fsnotify/fsnotify.go Fixes Windows path handling by converting VFS URI paths to native paths for fsnotify.Add(), and native event paths back to file:// URIs.
contrib/vfsevents/watchers/fsnotify/path_test.go Adds unit tests for the new conversion helpers (including Windows drive-letter behavior via GOOS parameterization).
contrib/vfsevents/watchers/fsnotify/fsnotify_test.go Re-enables Windows tests and updates assertions to compare event URIs using the new helper.
contrib/vfsevents/watchers/fsnotify/event_analysis_test.go Un-skips Windows and updates URI/path substring checks to be slash-normalized.
contrib/vfsevents/watchers/fsnotify/debounce_test.go Un-skips Windows tests now that drive-letter conversion is fixed.
contrib/vfsevents/CHANGELOG.md Documents the Windows drive-letter file:// round-trip fix under Unreleased.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread contrib/vfsevents/watchers/fsnotify/fsnotify.go Outdated
Comment thread contrib/vfsevents/watchers/fsnotify/fsnotify.go
Comment thread contrib/vfsevents/watchers/fsnotify/path_test.go
- Fix Windows drive-root paths (e.g. a location at C:\) being reduced to
  a bare "C:", which Windows interprets as "current directory on drive
  C" rather than the drive root.
- Derive the fsnotify watch path from Location.Path() instead of
  hand-parsing Location.URI(), so a location with a non-empty authority
  can no longer have it swept into the path and mangled by the
  drive-letter conversion. Extracted this into a small, directly
  testable resolveWatchPath/resolveWatchPathOS pair.
- Preserve the location's authority when converting a native fsnotify
  event path back into a file:// URI, instead of silently dropping it.
- Cross-reference the near-duplicate drive-letter conversion logic in
  backend/os/file.go's toNativeOSPath (which contrib/vfsevents can't
  import across module boundaries) so the two stay in sync.
- Extend path_test.go to cover the new drive-root, authority, and
  resolveWatchPathOS cases.

Co-authored-by: Cursor <cursoragent@cursor.com>
@funkyshu

Copy link
Copy Markdown
Member Author

Pushed a follow-up commit addressing self-review feedback:

  • Drive-root paths: a location watching an entire Windows drive (e.g. C:\) was being reduced to a bare "C:", which Windows interprets as "current directory on drive C", not the drive root. vfsPathToNativeOS now re-adds the trailing separator when stripping leaves a bare drive letter.
  • Authority-safety: Start() now derives the watch path via Location.Path() instead of hand-parsing Location.URI(), so a location with a non-empty authority (e.g. file://host/C:/Temp) can no longer have the authority swept into the path and mangled by the drive-letter conversion. Extracted into a small, directly testable resolveWatchPath/resolveWatchPathOS pair.
  • Authority round-trip: convertEvent()'s URI conversion now preserves the location's authority instead of silently dropping it.
  • Cross-referenced the near-duplicate drive-letter logic in backend/os/file.go's toNativeOSPath (can't import across module boundaries) so the two stay in sync going forward.
  • Extended path_test.go with cases for the drive-root fix, authority preservation, and a new resolveWatchPathOS suite that builds real (virtual) vfs.Locations via vfssimple to exercise the full location -> native-path path, still without needing a Windows runner.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

contrib/vfsevents/watchers/fsnotify/fsnotify.go:569

  • The comment says this function “mirrors” backend/os.toNativeOSPath, but the implementations already diverge: this version also normalizes a bare drive letter ("C:") into an explicit drive root ("C:\") after trimming trailing slashes. Please clarify the note so future changes don’t incorrectly assume the logic is identical.
// NOTE: mirrors the unexported toNativeOSPath in
// github.com/c2fo/vfs/v7/backend/os/file.go; keep both in sync if the Windows
// drive-letter handling changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vfsevents fsnotify: file:// URI round-trip produces invalid path on Windows

2 participants