Do not convert volume target path to Windows style, implement tilde expansion for Windows

[dmjb]

Since we deal with Linux containers, the volume target path should always be a Unix style path, even on Windows. Prior to this, the code would convert the target path to a Windows style path on Windows hosts due to the use of filepath.Clean. This PR changes the code to use path.Clean for the target instead, which always produces a Unix style path. Pre-existing validation ensures that the user cannot pass Windows style paths as input.

Another issue which was discovered by @danbarr as part of this was inconsistent handling of tilde expansion (i.e. converting ~/foo to an absolute path with the user's home folder). This was exacerbated by ToolHive using hand-rolled code to do absolute path expansion when there is a function in the Go standard library to do this. The new code explicltly checks to see if the path starts with ~ on Windows hosts, replaces it with the contents of the USERPROFILE env var if true, and then uses the Go standard library to convert to an absolute path.

[codecov]

Codecov Report

❌ Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.66%. Comparing base (9598136) to head (cb1c9fb).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/container/docker/client.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2488   +/-   ##
=======================================
  Coverage   55.66%   55.66%           
=======================================
  Files         295      295           
  Lines       28086    28086           
=======================================
  Hits        15635    15635           
- Misses      11029    11030    +1     
+ Partials     1422     1421    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[danbarr]

@claude please review this

[claude]

Claude finished @danbarr's task —— View job

---

Code Review

Todo List

• Read and analyze the changed files
• Review path handling changes in pkg/container/docker/client.go
• Review permission profile changes in pkg/permissions/profile.go
• Assess test coverage concerns
• Provide comprehensive feedback

---

Summary

This PR makes important fixes to path handling for volume mounts in ToolHive, addressing two key issues:

1. Container target paths now correctly remain Unix-style on Windows hosts - Switched from filepath.Clean to path.Clean for target paths
2. Improved tilde expansion on Windows - Replaced hand-rolled path resolution with Go standard library filepath.Abs() and proper ~ handling

Detailed Review

✅ Strengths

1. Correct Use of path.Clean vs filepath.Clean

   • The distinction is critical: filepath.Clean is platform-dependent (uses Windows separators on Windows), while path.Clean always produces Unix paths
   • Since containers run Linux, target paths should always be Unix-style - this change is correct
   • Good inline comments explaining the rationale (lines 299-300, 348-349, 384-385, 404 in profile.go)

2. Better Tilde Expansion on Windows

   • Using filepath.Abs() is the idiomatic Go approach for path resolution
   • Explicit tilde expansion for Windows using USERPROFILE is correct (lines 760-764 in client.go)
   • Cleaner than the previous hand-rolled filepath.Join(os.Getwd(), source) approach

3. Comprehensive Test Coverage Already Exists

   • The existing test suite in pkg/permissions/profile_test.go has excellent coverage of edge cases
   • Tests validate Windows paths, resource URIs, security issues, etc.
   • The changes align well with the existing test expectations

🔍 Observations & Questions

1. Removed cleanPath() Helper Function

In pkg/permissions/profile.go, the cleanPath() helper was removed and calls were replaced inline. While this is fine, I notice:

• It was only being used for filepath.Clean calls anyway
• Removal reduces indirection, which is good
• ✅ No issues here

2. Tilde Expansion - Platform Consistency

The tilde expansion in pkg/container/docker/client.go:760-764 only handles Windows:

if rt.GOOS == "windows" && strings.HasPrefix(source, "~") {
    homeDir := os.Getenv("USERPROFILE")
    source = strings.Replace(source, "~", homeDir, 1)
}

Question: What about Unix/Linux/macOS tilde expansion?

• On Unix systems, filepath.Abs() doesn't expand ~ automatically
• Users might expect ~/Documents to work on macOS/Linux too
• This could lead to inconsistent behavior across platforms

Recommendation: Consider using os.UserHomeDir() for cross-platform tilde expansion:

if strings.HasPrefix(source, "~") {
    homeDir, err := os.UserHomeDir()
    if err != nil {
        logger.Warnf("Warning: Failed to get user home directory: %v", err)
    } else {
        source = strings.Replace(source, "~", homeDir, 1)
    }
}

3. Test Coverage Gap - Windows Target Path Validation

The codecov report shows 5 lines missing coverage in client.go, likely the new tilde expansion code (lines 760-764).

Recommendation: Add a test specifically for the Windows tilde expansion path. This would involve:

• Testing that ~/foo gets expanded on Windows
• Verifying that the expanded path is then correctly converted to absolute

However, this might require platform-specific test builds or mocking rt.GOOS, which could be tricky.

4. Single vs Multiple Path Handling

Looking at parseSinglePath() in profile.go:398-407, when there's no colon separator (single path format), it now uses path.Clean instead of filepath.Clean:

// See comment above about using path.Clean instead of filepath.Clean.
cleanedPath := pkgpath.Clean(declaration)
return cleanedPath, cleanedPath, nil

Question: Is this intentional for single-path declarations?

• For single paths, both source AND target are set to the cleaned path
• Using path.Clean means even the source (host) path will be Unix-style on Windows
• This might cause issues if a user specifies C:\Users\foo as a single path on Windows

Example scenario:

Input: "C:\Users\foo"  (single path, Windows host)
After path.Clean: "C:/Users/foo"  (forward slashes)
Result: Tries to mount "C:/Users/foo" from host → might not match actual Windows path

Recommendation: For single-path declarations, consider keeping filepath.Clean for the source since it represents the host path. Only use path.Clean when the path is explicitly a target path.

📊 Test Coverage Analysis

Current patch coverage: 61.53% (5 lines missing)

The missing coverage is likely in pkg/container/docker/client.go around the new tilde expansion logic. Given this is Windows-specific code and testing it requires Windows or careful mocking, this is understandable but worth addressing if possible.

🎯 Recommendations Summary

1. High Priority: Review the parseSinglePath() behavior - should it use filepath.Clean for source paths?
2. Medium Priority: Consider adding cross-platform tilde expansion (not just Windows)
3. Low Priority: Add tests for Windows tilde expansion if feasible

📝 Minor Notes

• Code style and formatting look good
• Inline comments are helpful and clear
• The PR description accurately describes the changes

Verdict

The core fix is correct and well-implemented - using path.Clean for container target paths is the right approach. The main concern is around the parseSinglePath() function potentially using Unix-style cleaning for Windows host paths, which could cause mount failures.

I'd recommend verifying the single-path behavior on Windows before merging, or adding a test case to validate it works as expected.

---

[dmjb]

Question: What about Unix/Linux/macOS tilde expansion?

• On Unix systems, filepath.Abs() doesn't expand ~ automatically
• Users might expect ~/Documents to work on macOS/Linux too
• This could lead to inconsistent behavior across platforms

Recommendation: Consider using os.UserHomeDir() for cross-platform tilde expansion:

This is intentional - it is assumed that that Unix shells will expand tilde for us.