fix(search): keep native Windows paths for rg/grep on Windows - #75455
fix(search): keep native Windows paths for rg/grep on Windows#75455wertigoxxx wants to merge 6 commits into
Conversation
2be93e9 to
9abeb0a
Compare
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Windows search-path fix. The current implementation confirms the premise: tools/file_operations.py:961-976 rewrites native paths for Bash, while all affected search paths still use that helper (tools/file_operations.py:2221, 2232, 2288, 2418); Windows command environments disable MSYS conversion by default in tools/environments/local.py:1178-1198.
Problems
- The new tests validate only
_escape_shell_arg_native. They do not verify that_search_files_rg,_search_with_rg, and_search_with_grepactually emit the native path after this wiring change.
Suggested changes
- Add mock-environment command assertions for the sorted and fallback
rg --filespaths plus the rg and grep content-search paths, under_IS_WINDOWS = True.
Automated hermes-sweeper review.
| @@ -2221,7 +2238,7 @@ def _search_files_rg(self, pattern: str, path: str, limit: int, offset: int) -> | |||
| # Try mtime-sorted first (rg 13+); fall back to unsorted if not supported. | |||
There was a problem hiding this comment.
Please add a search-path command-construction test as well as the helper tests. The new helper tests do not prove this rg --files call (or the sibling rg/grep call sites) actually retains the native drive path.
alt-glitch
added
type/bug
teknium1
added
the
sweeper:blast-moderate
0825211 to
54cfb92
Compare
_escape_shell_arg rewrites C:\... to /c/... for bash builtins, but native Windows rg (WinGet/MSVC build) cannot resolve /c/... when MSYS2_ARG_CONV_EXCL=* disables MSYS argument conversion - it treats the path as root-relative and fails with os error 3. _search_files_rg swallowed the error via 2>/dev/null, silently returning 0 results. Add _escape_shell_arg_native() which quotes without the /c/ rewrite and converts MSYS paths back to native form; use it for the path argument in _search_files_rg, _search_with_rg and _search_with_grep.
Regression tests for the native Windows path form: drive paths stay intact, MSYS /c/... input converts back, relative paths unchanged, and non-Windows hosts are a no-op (platform-independent via _IS_WINDOWS monkeypatch).
Per review feedback: verify the actual commands emitted by _search_files_rg (sorted + fallback), _search_with_rg and _search_with_grep carry the native Windows drive path, not the /c/... rewrite.
- LC_ALL=C for find so '%T@' mtime is locale-independent: on non-C locales (e.g. hu_HU) Cygwin find emits NBSP thousands separators (byte 0xa0) and a comma decimal separator, breaking UTF-8 decoding and the 'mtime path' parse. - Normalize find output paths from MSYS form (/c/Users/x) to the native drive form so results match the rg path and resolve correctly in hidden-root filtering.
subprocess.run(shell=True) invokes cmd.exe on Windows, which cannot run the find/sort/tail pipeline the hidden-path tests drive (and shell=True + executable mangles args as '<exe> /c ...'). Run ["bash", "-c", command] explicitly instead. The umask permission test asserts POSIX semantics that don't exist on Windows - skip it.
Consolidation per triage (related NousResearch#72347, NousResearch#67940): - _uses_native_windows_search_paths() gates the MSYS->native path conversion to the LOCAL Git Bash backend on Windows; remote/container backends keep POSIX path semantics even when the host is Windows - _check_lint now passes the {file} placeholder through the native path form: linters (node/go/rustfmt) are native Windows binaries that reject the MSYS /c/... spelling Adds a lint command-construction regression test.
5bd7115 to
323d0a3
Compare
Native Windows 11 verification (monerostar)Could not leave a formal Approve (fork collaborator scope) — posting live evidence instead. Host: native Windows 11 · Python 3.11.15 · pytest 9.1.1 Root cause reproduced outside HermesWith
So rewriting the search root to PR helpers on live LocalEnvironmentEnd-to-end
TestsNotes
LGTM from a native Win11 machine with real MSVC rg. |
|
for anyone having similar issues I found you can make hermes read this git issue/request, and make a python script and temporary skill for using as substitute for search while this issue is not merged, just make sure to tell hermes to not edit its own files to avoid breaking hermes updater |
SummaryTwenty-five PRs reference this Windows path-handling complex; five directly address the outbound native-ripgrep failure, while the remainder cover distinct terminal-CWD, Bash-selection, inbound file-resolution, deleted-CWD, process-cleanup, or NUL-redirection causes. For the reported search failure, #67914 has the strongest diff because it converts native and MSYS drive forms to Bash-safe native Related pull requests
DuplicatesThe terminal/CWD chains consolidate into #56384 (#35653, #46586, #47854, #47920, #49833, #50191, and #50596), inbound file-tool normalization consolidates into #61915 (#44735, #44765, #46887, #46995, and #50488), and deleted-CWD recovery consolidates into #62189 over #62405. The native-rg chain is #67914 over #63458, #69183, #72922, and the rg portion of #75455; #69293 is the weaker duplicate direction of the withdrawn #65363 NUL work. Suggested consolidationAuthor action: rebase #75455 onto main or split out its independently reviewable grep/lint work, close its rg portion and #63458 as duplicates of #67914, and keep #67914 open with the salvage path identified by its recorded best-fix verdict: local-only Complex graphflowchart TD
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I63177(["issue #63177 (open)"])
I67629(["issue #67629 (open)"])
I75007(["issue #75007 (open)"])
I77036(["issue #77036 (open)"])
subgraph Dup63458 ["PRs duplicating each other"]
P63458["PR #63458 (open)"]
P67914["PR #67914 (open)"]
P69183["PR #69183 (closed)"]
P72922["PR #72922 (closed)"]
P75455["PR #75455 (open)"]
end
P75455 -->|fixes| I63177
P75455 -->|fixes| I67629
P75455 -->|fixes| I75007
P75455 -->|fixes| I77036
class I63177 open
class I67629 open
class I75007 open
class I77036 open
class P63458 open
class P67914 open
class P69183 closed
class P72922 closed
class P75455 open
class P67914 best
class P67914 best
class P67914 best
class P67914 best
class P75455 target
click I63177 "https://github.com/NousResearch/hermes-agent/issues/63177"
click I67629 "https://github.com/NousResearch/hermes-agent/issues/67629"
click I75007 "https://github.com/NousResearch/hermes-agent/issues/75007"
click I77036 "https://github.com/NousResearch/hermes-agent/issues/77036"
click P63458 "https://github.com/NousResearch/hermes-agent/pull/63458"
click P67914 "https://github.com/NousResearch/hermes-agent/pull/67914"
click P69183 "https://github.com/NousResearch/hermes-agent/pull/69183"
click P72922 "https://github.com/NousResearch/hermes-agent/pull/72922"
click P75455 "https://github.com/NousResearch/hermes-agent/pull/75455"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 25 pull requests and 10 issues in this complex. Each diff was read against this issue; Assessment working set: 180 kB of PR diffs, 71 kB of issue/PR text, 66 kB of discussion (90 comments), 78 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Why the broader scope is the fix, not scope creepThanks for the triage write-up. I'd like to make the case for the boundary #75455 draws, since the wider surface is deliberate and every piece of it was hit by a real failure. The bug class is "a native Windows binary receives an MSYS path", not "rg fails". We already have 25 PRs on this complex — because the fixes were narrow. Each one repaired a single call site (cwd normalization, On the backslash vs The broader scope also caught a second real bug. While covering the find fallback I hit the locale breakage: on non-C locales (e.g. Test coverage is on the wiring, not just the helper. Per the sweeper review, If consolidation is the call, I'd suggest adopting #75455 as the base and folding #67914's |
|
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. |
6 participants
_escape_shell_arg rewrites C:... to /c/... for bash builtins, but native Windows rg (WinGet/MSVC build) cannot resolve /c/... when MSYS2_ARG_CONV_EXCL=* disables MSYS argument conversion - it treats the path as root-relative and fails with os error 3. _search_files_rg swallowed the error via 2>/dev/null, silently returning 0 results.
Add _escape_shell_arg_native() which quotes without the /c/ rewrite and converts MSYS paths back to native form; use it for the path argument in _search_files_rg, _search_with_rg and _search_with_grep.