Add path helper functions to resolve url.URL issue with Windows paths - #266
Merged
Conversation
WalkthroughIntroduces ChangesWindows-safe file URL conversion
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
checkinnuggets
changed the title
Contributor
|
Hey @checkinnuggets since the other PRs have merged to If so could you rebase it please and mark it ready? |
… concatenation or direct calls to url.URL, which does not produce the expected result on Windows.
checkinnuggets
force-pushed
the
windows-path-fix
branch
from
July 8, 2026 16:54
5369efc to
a5471be
Compare
checkinnuggets
marked this pull request as ready for review
July 8, 2026 16:57
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/files/paths.go`:
- Around line 31-38: FileURLToPath currently only reads u.Path, so file://C:/...
URLs lose the drive letter because net/url places C: in u.Host. Update
FileURLToPath to fall back to u.Host for Windows drive-letter file URLs, and
keep the existing windowsURIDrivePath handling so both host- and path-based
drive formats resolve correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 957327f5-4639-40e8-944b-f4e40c9e7e66
📒 Files selected for processing (8)
internal/cache/cache.gointernal/files/paths.gointernal/files/paths_test.gointernal/files/paths_windows_test.gointernal/provider/mcpm/registry_test.gointernal/provider/mozilla_ai/registry_test.gointernal/runtime/loader.gointernal/runtime/loader_test.go
checkinnuggets
marked this pull request as draft
July 8, 2026 17:05
checkinnuggets
marked this pull request as ready for review
July 20, 2026 19:32
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/files/paths.go (1)
29-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve or reject non-local file URL authorities.
For
file://server/share/registry.json,u.Hostisserverandu.Pathis/share/registry.json; this code returns/share/registry.jsonand loses the server component. Preserve the host for non-localhostauthorities and add a regression test for a canonical UNC file URL, or return an error for unsupported authorities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/files/paths.go` around lines 29 - 44, Update FileURLToPath to preserve non-local file URL authorities such as the server component in canonical UNC URLs, while retaining the existing Windows drive-letter and localhost behavior; alternatively, reject unsupported authorities with an error if the function contract permits it. Add a regression test covering a canonical UNC file URL like file://server/share/registry.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/files/paths.go`:
- Around line 9-13: Update PathToFileURL so Windows drive-letter detection
requires a path separator after the drive prefix, preventing C: and
C:registry.json from being treated as absolute drive paths. Resolve and
canonicalise relative paths before constructing the file URL, preserving correct
round-tripping through FileURLToPath. Add regression coverage for
cache/registry.json and C:registry.json.
---
Outside diff comments:
In `@internal/files/paths.go`:
- Around line 29-44: Update FileURLToPath to preserve non-local file URL
authorities such as the server component in canonical UNC URLs, while retaining
the existing Windows drive-letter and localhost behavior; alternatively, reject
unsupported authorities with an error if the function contract permits it. Add a
regression test covering a canonical UNC file URL like
file://server/share/registry.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a4ca940-a932-479c-b383-ebc8b26e6562
📒 Files selected for processing (1)
internal/files/paths.go
Comment on lines
+9
to
+13
| // windowsDrivePath matches a Windows drive-letter path (e.g. "C:/Users/..."). | ||
| var windowsDrivePath = regexp.MustCompile(`^[a-zA-Z]:`) | ||
|
|
||
| // windowsURIDrivePath matches a Windows drive-letter path as a file URI (e.g. "/C:/Users/..."). | ||
| var windowsURIDrivePath = regexp.MustCompile(`^/[a-zA-Z]:`) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 '\b(PathToFileURL|FileURLToPath)\b' internal/files internal/cache internal/runtime --glob '*.go'
rg -n -C4 'relative|C:.*foo|PathToFileURL' internal/files --glob '*_test.go'Repository: mozilla-ai/mcpd
Length of output: 10290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- paths.go ---'
cat -n internal/files/paths.go
echo '--- paths_test.go tests ---'
cat -n internal/files/paths_test.go
echo '--- paths_windows_test.go tests ---'
cat -n internal/files/paths_windows_test.go
echo '--- runtime loader ---'
cat -n internal/runtime/loader.go
echo '--- cache path construction ---'
rg -n -C3 'cachePath|PathToFileURL|GetCache|cache' internal/cache --glob '*.go'Repository: mozilla-ai/mcpd
Length of output: 29208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe Go net/url behaviour without importing repository code.
go version
cat > /tmp/url_probe.go <<'EOF'
package main
import (
"fmt"
"net/url"
"runtime"
)
func main() {
paths := []string{
"C:/Users/foo.txt",
"C:Users/foo.txt",
"cache/registry.json",
"\\share\\file.txt",
"//server/share",
"//server/share?x=1",
}
for _, path := range paths {
p := path
_, isWindows := runtime.LookupEnv("GOOS")
if isWindows {
fmt.Printf("%s (windows os env=%s)\n", fmt.Sprintf("%s: ", path), isWindows)
if len(p) >= 2 } {
fmt.Printf("\n")
}
}
u := url.URL{Scheme: "file", Path: p}
fmt.Printf("input=%s url=%s host=%s path=%s rawpath=%s\n", path, u.String(), u.Host, u.Path, u.RawPath)
parsed, err := url.Parse(u.String())
if err != nil {
fmt.Printf("parsed error: %v\n", err)
} else {
fmt.Printf("parsed: scheme=%s host=%s path=%s fragment=%s query=%s\n", parsed.Scheme, parsed.Host, parsed.Path, parsed.Fragment, parsed.RawQuery)
}
}
}
EOF
go run /tmp/url_probe.go
echo '--- read-only path shape check ---'
python3 - <<'PY'
import re
regex = re.compile(r'^[a-zA-Z]:')
uris = re.compile(r'^/[a-zA-Z]:')
for path in [
"C:/Users/foo.txt",
"C:Users/foo.txt",
"cache/registry.json",
"C:",
"C:",
"\\\\server\\share",
]:
print(f"{path!r}: windowsDrivePath={bool(regex.match(path))}; uriDrivePath={bool(uris.match('/'+path if len(path)>=2 and path[1]==':' else path))}")
PYRepository: mozilla-ai/mcpd
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe Go net/url behaviour without importing repository code.
cat > /tmp/url_probe.go <<'EOF'
package main
import (
"fmt"
"net/url"
)
func main() {
paths := []string{
"C:/Users/foo.txt",
"C:Users/foo.txt",
"cache/registry.json",
"\\\\server\\share",
"//server/share",
"//server/share?x=1",
}
for _, path := range paths {
u := url.URL{Scheme: "file", Path: path}
fmt.Printf("input=%s url=%s host=%s path=%s rawpath=%s\n", path, u.String(), u.Host, u.Path, u.RawPath)
parsed, err := url.Parse(u.String())
if err != nil {
fmt.Printf("parsed error: %v\n", err)
} else {
fmt.Printf("parsed: scheme=%s host=%s path=%s fragment=%s query=%s\n", parsed.Scheme, parsed.Host, parsed.Path, parsed.Fragment, parsed.RawQuery)
}
}
}
EOF
go run /tmp/url_probe.go
echo '--- read-only path shape check ---'
python3 - <<'PY'
import re
regex = re.compile(r'^[a-zA-Z]:')
uris = re.compile(r'^/[a-zA-Z]:')
for path in [
"C:/Users/foo.txt",
"C:Users/foo.txt",
"cache/registry.json",
"C:",
"\\\\server\\share",
]:
prefixed = ("/" + path) if len(path) >= 2 and path[1] == ":" else path
print(f"{path!r}: windowsDrivePath={bool(regex.match(path))}: uriDrivePath={bool(uris.match(prefixed))}")
PYRepository: mozilla-ai/mcpd
Length of output: 1440
Resolve relative paths before creating file URLs.
PathToFileURL only checks for [a-zA-Z]:, so C:foo and C: are treated as drive-letter paths. Relative paths like cache/registry.json are serialized as file://cache/registry.json, then parsed with host cache and path /registry.json; FileURLToPath reads /registry.json and cannot resolve the cached registry location. Require a separator after the drive letter and resolve/canonicalise relative paths before making a file:// URL.
Add regression tests for cache/registry.json and C:registry.json.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/files/paths.go` around lines 9 - 13, Update PathToFileURL so Windows
drive-letter detection requires a path separator after the drive prefix,
preventing C: and C:registry.json from being treated as absolute drive paths.
Resolve and canonicalise relative paths before constructing the file URL,
preserving correct round-tripping through FileURLToPath. Add regression coverage
for cache/registry.json and C:registry.json.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Original issue: #218
Further discussion with @peteski22 on PR: #221
This PR was split out of #264. It addresses an issue where url.URL produces unexpected output on Windows.
mcpd has a couple of examples of building/parsing
file://URLs -"file://" + path,url.URLand so on. This doesn't work on Windows. This seems to be a longstanding/known issue (golang/go#32456) which can cause the drive letter to be parsed intou.Hostinstead ofu.Path.This PR adds helpers to
internal/files, namelyPathToFileURLandFileURLToPathwhich centralise the solution, and updates call sites to construct paths through this one route.PR Type
Relevant issues
Checklist
make lint,make test).AI Usage
AI Model/Tool used:
Claude Code
Any additional AI details you'd like to share:
Used to help me analyse codebase and validate understanding of the problem
NOTE:
When responding to reviewer questions, please respond yourself rather than copy/pasting reviewer comments into an AI and pasting back its answer. We want to discuss with you, not your AI :)
Summary by CodeRabbit
Bug Fixes
file://resources when loading cached and runtime content.Tests
file://URLs.