fix: preserve POSIX rg paths for remote backends on Windows - #84557
fix: preserve POSIX rg paths for remote backends on Windows#84557guoyutw wants to merge 1 commit into
Conversation
monerostar
left a comment
There was a problem hiding this comment.
Native Windows 11 verification
Host: Windows 11 (Windows-10-10.0.26200-SP0), CPython 3.11.15, worktree ab6019d7b. Native rg is on PATH via WinGet: ripgrep 15.1.0 (C:\Users\Admin\AppData\Local\Microsoft\WinGet\Packages\...\rg.EXE).
Gap on current main
tools/file_operations.py::_escape_native_tool_arg on upstream main is still:
if _IS_WINDOWS and arg:
arg = _msys_to_windows_path(arg).replace("\\", "/")That keys off the controller OS, not the execution backend. Live helper on this box:
_msys_to_windows_path("/c/remote/project") -> C:\remote\project
_msys_to_windows_path("/c/Users/Admin") -> C:\Users\Admin
So a Windows controller talking to SSH/Docker would rewrite a legitimate remote POSIX path /c/remote/project into C:/remote/project. That is the bug this PR names.
After this PR
Mocked SSHEnvironment search for /c/remote/project captured:
rg --files ... '/c/remote/project'
No C:/remote/project.
Mocked LocalEnvironment search for C:\Users\Admin captured:
rg --files ... 'C:/Users/Admin'
Local native conversion from #84378 is preserved.
Live LocalEnvironment → native rg
Temp dir C:\Users\Admin\AppData\Local\Temp\rg84557_ufcm_5fm with NATIVE_RG_LIVE_TOKEN in native-rg-live.txt:
| search | result |
|---|---|
content NATIVE_RG_LIVE_TOKEN |
error=None, count=1 |
files native-rg-live.txt |
error=None, hit C:/Users/Admin/AppData/Local/Temp/rg84557_ufcm_5fm\native-rg-live.txt |
| absent token | error=None, count=0 |
rg --version via LocalEnvironment |
rc 0, ripgrep 15.1.0 |
No os error 3.
Tests
python -m pytest tests/tools/test_file_operations.py::TestEscapeNativeToolArg \
tests/tools/test_file_tools_live.py::TestWindowsNativeRgLiveSearch \
-q --tb=short -o addopts=
10 passed in 4.26s
Review notes
- Narrow follow-up to #84378: keep the local Windows native spelling, stop leaking it into remote POSIX backends.
isinstance(self.env, LocalEnvironment)is the right boundary. - MagicMock in older tests is now
spec=LocalEnvironment, which is what makes the new guard testable. - I cannot exercise a real SSH backend on this machine tonight; the mock + the live
_msys_to_windows_pathconversion is the evidence that main would have rewritten/c/remote/project.
I cannot merge. From native Win11 the local path is solid and the remote-preservation change matches the demonstrated gap.
CI note: no checks reported on the head branch. Local slice above is green.
fix: preserve POSIX rg paths for remote backends on Windows
|
|
@Enough1122 Thanks for calling out the concrete I completed a read-only audit of the production Environment implementations and all For the current architecture, This is a valid future extensibility concern, but not a current correctness gap. If a non-subclass local proxy or Windows-native remote backend is introduced, it should add the capability seam together with path-authority tests. Existing tests cover Windows local conversion, Windows-controller/remote-POSIX preservation, non-Windows quoting equivalence, and live Windows rg paths; the live test explicitly skips when rg is unavailable. |
Summary
Problem
PR #84378 correctly fixed native Windows rg path handling. However, the Windows-native conversion was selected from the controller host OS rather than the actual execution backend.
For a Windows controller using an SSH or other non-local POSIX backend, a target such as:
/c/remote/project
could be rewritten as:
C:/remote/project
/c/remote/project may be a legitimate POSIX path on the remote backend. It must not be interpreted as an MSYS drive path merely because the controller host is Windows.
Fix
Limit Windows-native rg target conversion to LocalEnvironment when running on Windows.
Non-local backends retain the existing _escape_shell_arg() behavior and therefore preserve POSIX/Bash-safe target paths.
This is a narrow follow-up to PR #84378, not a replacement or redesign of that fix.
Testing
Related
This change addresses the demonstrated backend-boundary path-conversion gap and does not claim to resolve issues beyond that scope.