fix(file_ops): preserve native Windows paths for ripgrep - #83363
fix(file_ops): preserve native Windows paths for ripgrep#83363Dolverin wants to merge 6 commits into
Conversation
|
I found two argument-boundary cases on exact head 1. Drive-relative paths become drive-rooted
source = r"D:logs\app.txt" # relative to D:'s current directory
actual = _native_windows_path_for_exe(source)
assert actual == "D:logs/app.txt"
assert not ntpath.isabs(actual)Actual: 2. A regex beginning with
|
|
Thanks @yuzilongleif-collab — both boundary cases are addressed in
|
…67629) Closes NousResearch#67629 Root cause: search_files path args went through _bash_safe_path which rewrites D:\... to /d/... for Git Bash, but the Win32 ripgrep binary does not resolve MSYS drive paths (os error 3). Fix: add _native_windows_path_for_exe + _escape_native_exe_arg and use that for rg path args so drive paths stay as D:/... while bash still launches the pipeline. Verification: pytest tests/tools/test_local_env_windows_msys.py tests/tools/test_file_operations.py -k "native_windows or escape_native or search_with_rg_uses" (cherry picked from commit 0761f4c)
The native drive rewrite in _escape_native_exe_arg applied whenever the host OS was Windows, regardless of the execution backend. A non-local backend (SSH, Docker, Modal, Daytona) owns its own path namespace, so a valid target-side path like /mnt/d/project was reinterpreted as a host Windows drive form (D:/project), breaking remote rg lookups (reported on NousResearch#67914). Restrict the conversion to _IS_WINDOWS AND the local backend via a new _is_local_backend() helper. Non-local backends keep their own path semantics. Adds a regression test asserting /mnt/d/... and plain POSIX paths pass through untouched for non-local backends. (cherry picked from commit dbce6c7)
Add _escape_pattern_arg and use it for rg + grep patterns so backslash regex escapes (\w, \d, \() are preserved and not mangled into forward slashes on Windows. Consolidates the pattern portion of NousResearch#69183 per the Co-authored-by: rille111 <rille111@users.noreply.github.com> (cherry picked from commit b012329)
The zero-match near-miss probes are native rg invocations too. Use the same local-Windows native path escaping and quote-only pattern handling as the primary rg search so case/literal/hidden hints do not silently disappear under MSYS_NO_PATHCONV. Regression coverage captures all three probe commands.
Preserve drive-relative Windows paths by only normalizing slash-rooted drive paths. Pass search patterns with -e and insert -- before search paths in rg/grep content searches and zero-match probes so option-like patterns remain data.
ba5d231 to
db2d3d3
Compare
|
Verified on current exact head
Both findings from my prior comment are resolved; I have no remaining concern on those two boundaries. |
Duplicate of #67914: it already covers the local native-rg path conversion and protects remote backend path namespaces. |
|
Closing as implemented on main: #84378 (merged) covers this fix — a shared native-path escaper (_escape_native_tool_arg) applied to all six rg call sites, the zero-match probes, and the shell linter interpolation, with Windows regression tests. Thank you for diagnosing this — your PR correctly identified the same root cause (native binaries + MSYS_NO_PATHCONV means /c/... is never translated back), and the merged fix lands the same approach across the full call-site set. Sorry we couldn't land this one directly. |
Bug Description
On native Windows with the Git Bash local backend,
search_filescan fail for absolute paths in two ways:rg.exereceives an MSYS path such as/c/Users/...and fails withIO error ... (os error 3);total_count: 0, which looks like a legitimate no-match result.Regex patterns are affected by the same boundary: backslash escapes such as
\d,\w, and\(must not be rewritten as if they were Windows path separators.Fixes #67629
Fixes #63177
Root Cause
_escape_shell_arg()routes arguments through_bash_safe_path(), which intentionally rewritesC:\.../C:/...to Git Bash form (/c/...) for bash and MSYS-aware tools.That is correct for bash builtins and the grep/find paths, but not for a Win32
rg.exe: Hermes deliberately setsMSYS_NO_PATHCONV=1andMSYS2_ARG_CONV_EXCL=*to stop MSYS from mangling native command switches, so no later layer converts/c/...back before native ripgrep receives it.The same path-oriented rewriting must not be applied to search patterns: a regex is not a path.
Fix
This PR is a current-
mainsalvage/rebase of #67914, preserving the original commits and attribution, plus one focused follow-up for the zero-match probe sibling call sites._native_windows_path_for_exe()for native Windows executables._escape_native_exe_arg()and use it for nativergpath arguments._escape_pattern_arg()so search patterns are shell-quoted without MSYS path rewriting.rg --files, content search, and the three zero-matchrgprobes./mnt/d/...must not becomeD:/...on a remote target).D:logs/app.txtinstead of turning them into drive-rooted paths.-e/--argument boundaries for rg/grep searches and zero-match probes so option-like patterns such as-fooremain data.Salvage / Attribution
main.Co-authored-by: rille111trailer on the consolidated pattern-handling commit._zero_match_probe(), which used the same incorrect path/pattern escaping for nativerg.How to Verify
On native Windows with Git Bash and a Win32 ripgrep build:
C:\Temp\hermes-rg-live-verify\subdir\sample.txtcontainingneedle-windows-rg-path-fix.C:\Temp\hermes-rg-live-verifyC:/Temp/hermes-rg-live-verify/c/Temp/hermes-rg-live-verifytarget='files'andtarget='content'return the sample file instead ofos error 3or silent zero results.Test Plan
scripts/run_tests.sh tests/tools/test_file_operations.py tests/tools/test_local_env_windows_msys.py -k "native_windows or escape_native or search_with_rg_uses or escape_pattern or preserves_regex or native_windows_path_for_exe or zero_match_probe_uses or drive_relative or argument_boundaries" -q→ 15 passed.origin/mainmakes the new regression tests fail; restoring the fix makes them pass.origin/mainbaseline worktree. The remaining failing test names are pre-existing baseline failures tied to Windows symlink privileges and this host's global-ignore/Temp environment; no new failing test names were introduced.ruff check tools/environments/local.py tools/file_operations.py tests/tools/test_file_operations.py tests/tools/test_local_env_windows_msys.py→ passed.git diff --check→ passed.C:\...,C:/..., and/c/...path forms.Risk Assessment
Low / Medium — the change is confined to search-command argument construction. Bash/MSYS-aware file operations keep
_escape_shell_arg(), grep paths keep MSYS form, and native Windows path conversion is limited to the local backend. CI should cover the non-Windows no-op paths.