Skip to content

fix(tools): stop search_files failing on absolute Windows paths under Git Bash - #80314

Closed
fangliquanflq wants to merge 2 commits into
NousResearch:mainfrom
fangliquanflq:fix/search-files-win32-rg-paths
Closed

fix(tools): stop search_files failing on absolute Windows paths under Git Bash#80314
fangliquanflq wants to merge 2 commits into
NousResearch:mainfrom
fangliquanflq:fix/search-files-win32-rg-paths

Conversation

@fangliquanflq

Copy link
Copy Markdown
Contributor

What does this PR do?

On native Windows with Git Bash, search_files failed when given absolute drive paths (C:\... or /c/...). The tool rewrote those paths to MSYS form for bash, then passed the quoted /c/... argv to Win32 rg.exe. With Hermes' default MSYS_NO_PATHCONV, CreateFile does not understand MSYS mounts, so searches returned IO errors even though cd /c/... worked in the same shell.

Bug Cause

_escape_shell_arg rewrites drive paths to /c/... so bash builtins (sed, test, find, GNU grep) resolve correctly. That form is wrong for Win32 ripgrep under MSYS_NO_PATHCONV=1 / MSYS2_ARG_CONV_EXCL=*: quoted /c/... is passed through unchanged and CreateFile fails with os error 3. Relative paths still worked because they never triggered the drive rewrite.

Fix

Add _win32_tool_path / _escape_native_tool_arg and use them for every rg search-root path (_search_with_rg, _search_files_rg, _zero_match_probe). Bash builtins keep _escape_shell_arg (/c/...). Off-Windows behavior is unchanged.

Related Issue

N/A

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/environments/local.py - add _win32_tool_path to normalize MSYS/Cygwin/WSL drive paths to forward-slash C:/... for Win32 tools
  • tools/file_operations.py - add _escape_native_tool_arg; pass Win32-safe paths to all rg invocations; leave bash/find/grep on /c/...
  • tests/tools/test_local_env_windows_msys.py - cover _win32_tool_path including /cygdrive and /mnt forms
  • tests/tools/test_file_operations.py - assert content search, file search, and zero-match probes quote C:/... not /c/...

How to Test

  1. On Windows with Git Bash + a Win32 rg on PATH, from a Hermes checkout:
    • Call search_files with path=/c/<drive>/.../<repo>/tools and a known pattern -> matches (not IO error ... os error 3)
    • Same with path=C:\...\tools -> matches
    • Relative path=tools -> still matches
  2. Automated (already run locally):
scripts/run_tests.sh tests/tools/test_local_env_windows_msys.py tests/tools/test_file_operations.py -q
# focused Win32 path nodes also: pytest -k "Win32Tool or win32_safe" -q

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11 (Git Bash + Win32 ripgrep)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@fangliquanflq
fangliquanflq marked this pull request as ready for review August 6, 2026 11:59
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/file File tools (read, write, patch, search) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows duplicate This issue or pull request already exists labels Aug 6, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #67914. Both repair MSYS-to-native Windows path conversion for local ripgrep; #67914 also preserves remote backend paths and regex patterns.

@monerostar

Copy link
Copy Markdown
Contributor

Native Win11 verification (monerostar)

Host: physical Windows 11 build 26200, CPython 3.11.15 (win32), Win32 ripgrep 15.1.0, Hermes Git Bash at %LOCALAPPDATA%\hermes\git\usr\bin\bash.exe.

Bug class still real under Hermes defaults

Hermes sets MSYS_NO_PATHCONV=1 / MSYS2_ARG_CONV_EXCL=*. Under that, Win32 rg.exe + quoted MSYS path fails CreateFile:

# direct Win32 rg
rg … 'C:/Users/Admin/src/hermes-agent-contrib/tools'  → rc=0, hits
rg … '/c/Users/Admin/src/hermes-agent-contrib/tools' → rc=2
  IO error … The system cannot find the path specified. (os error 3)

# same via Git Bash -lc with single-quoted paths + MSYS_NO_PATHCONV
C:/…/tools → hits
/c/…/tools → os error 3

On this branch, the shell vs native split is correct:

_escape_shell_arg(C:\…\tools)  → '/c/Users/Admin/src/hermes-agent-contrib/tools'   # bash builtins
_escape_native_tool_arg(…)     → 'C:/Users/Admin/src/hermes-agent-contrib/tools'   # Win32 rg
_win32_tool_path('/c/…')       → 'C:/…'
_win32_tool_path('/cygdrive/c/Users/x') → 'C:/Users/x'
_win32_tool_path('/mnt/c/Users/x')      → 'C:/Users/x'

Live ShellFileOperations.search (this PR head 34c11a5d0)

Pattern ShellFileOperations, limit 5 — all path forms returned matches, error=None:

path form n_matches
C:\Users\Admin\src\hermes-agent-contrib\tools 5
C:/Users/Admin/src/hermes-agent-contrib/tools 5
/c/Users/Admin/src/hermes-agent-contrib/tools 5
relative tools 5

Focused tests

pytest tests/tools/test_local_env_windows_msys.py::TestWin32ToolPath \
       tests/tools/test_file_operations.py -k "Win32Tool or win32_safe or escape_native or search_with_rg_passes or search_files_rg_passes or zero_match_probe_passes" \
       -o addopts= -v
# 9 passed

CI on the PR is green (All required checks pass).

Cluster note

Triage flagged overlap with #67914 / related search-files Windows path work. This PR’s split (_bash_safe_path for bash builtins vs _win32_tool_path / _escape_native_tool_arg for Win32 rg) matches the actual failure mode on this host. Whichever sibling merges, please keep the Win32-safe rg path form — MSYS /c/... must not be passed to rg.exe under MSYS_NO_PATHCONV.

Verdict: LGTM from native Win11. Could not leave a formal Approve (fork collaborator scope) — evidence review as comment.

@fangliquanflq

Copy link
Copy Markdown
Contributor Author

Thanks for the verification. This is a real issue I've hit in normal use - it can seriously slow down agent inference. For now I'm working around it via skills.

@monerostar monerostar 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.

Native Win11 live-verify (monerostar)

Host: Windows 11 build 26200 · Git Bash · ripgrep 15.1.0 (Win32) · install tree %LOCALAPPDATA%\hermes\hermes-agent

PR tip: 34c11a5d0 · control: live install main (same box)

Bug still on main

Absolute path content search rewrites to /c/... for bash, then hands that to native rg.exe under MSYS_NO_PATHCONV → IO error. File search fails silent (total=0, err=None).

Fixture: %LOCALAPPDATA%\Temp\hermes-rg-path-repro\subdir\sample.txt needle needle-xyz-unique-content-67914

path form main content main files *.txt PR content PR files
C:\...\hermes-rg-path-repro err os error 2 on /c/... total 0 silent total 1 total 2
C:/... same fail silent 0 total 1 total 2
/c/... same fail silent 0 total 1 total 2

Helper on PR: _escape_native_tool_arg maps all three → 'C:/Users/.../hermes-rg-path-repro'. _escape_shell_arg still MSYS for bash builtins (correct split).

Tests

Path-escape focused: 4 passed.
Broader -k path|msys|windows|escape|rg|native: 36 passed; 3 failed look orthogonal (hidden-root fallback empty set; PortableGit PATH expects /pg/usr/bin vs backslash form on this host). Not blaming the rg path fix.

CI on the PR: green (30 success).

Sibling

#78224 also fixes the live matrix on this box, but is thinner (no local.py path helpers, mergeState BLOCKED, empty checks when I looked). Prefer this PR; cross-link only.

Looks good from this daily driver.

@monerostar monerostar 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.

Sibling pointer (monerostar)

Re-checked 2026-08-11 on native Win11: this tip is still CONFLICTING/DIRTY vs current main.

Fresh absolute-path matrix (same fixture as before) now lands cleanly on #78224 @ e228895a8 (MERGEABLE), including _quote_rg_path for content + files + zero-match probe routing.

Prefer the mergeable tip for landing; rebase/refresh here if you want this helper shape (_escape_native_tool_arg) instead.

search_files rewrote drive paths to /c/... for Git Bash, but native
Win32 rg.exe uses CreateFile under MSYS_NO_PATHCONV and rejects those
mounts. Keep bash builtins on /c/...; quote C:/... for rg argv.
@fangliquanflq
fangliquanflq force-pushed the fix/search-files-win32-rg-paths branch from 34c11a5 to 6f74322 Compare August 11, 2026 15:27
@fangliquanflq

fangliquanflq commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@monerostar Thanks for the re-check and the sibling pointer.

Rebased this branch onto current main and resolved the conflicts in tests/tools/test_file_operations.py (kept the Win32-safe rg path coverage from this PR, and preserved main's updated @pytest.mark.windows_only read_file bash-path assertions).

Tip is now 6f74322c8. Focused path-escape tests: 9 passed. Happy to re-verify the absolute-path matrix on native Win11 against this refreshed tip; still prefer landing whichever sibling maintainers pick, as long as Win32 rg keeps getting C:/... under MSYS_NO_PATHCONV.

@fangliquanflq fangliquanflq reopened this Aug 12, 2026
@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Aug 12, 2026
@teknium1

Copy link
Copy Markdown
Contributor

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.

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

Labels

backend/local Local shell execution P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants