Skip to content

fix: handle Windows drive-letter paths in grep (ripgrep) output - #123

Open
engmohamedsalah wants to merge 1 commit into
andrewyng:mainfrom
engmohamedsalah:fix/windows-rg-parser
Open

fix: handle Windows drive-letter paths in grep (ripgrep) output#123
engmohamedsalah wants to merge 1 commit into
andrewyng:mainfrom
engmohamedsalah:fix/windows-rg-parser

Conversation

@engmohamedsalah

@engmohamedsalah engmohamedsalah commented Jul 25, 2026

Copy link
Copy Markdown

Issue: #17

What

Fixes grep returning corrupted results on Windows when ripgrep (rg) is on PATH. Closes #17.

Root cause

_parse_rg parsed ripgrep output with line.split(":", 2). The search path is absolute, and on Windows an absolute path starts with a drive letter, so:

C:\ws\a.py:12:def hello()

split into ["C", "\\ws\\a.py", "12:def hello()"] — every match was reported as file C, line 0, with the real line number swallowed into the matched text. Linux/macOS never hit this (no drive-letter colon), and CI doesn't either: it has no rg, so it uses the pure-Python fallback (_py_grep), which was already correct.

Fix

Pass --with-filename --null so ripgrep always prints the path and NUL-separates it from line:text, then split the path off on the NUL byte — a byte that can't appear in a path — before parsing line/text. --null defeats the drive-letter colon; --with-filename keeps the format universal so a single-file target isn't silently dropped (see note). Portable: harmless on Linux/macOS, correct everywhere. No new dependencies.

Tests

  • The existing test_grep_finds_matches_and_respects_glob now passes on Windows-with-rg (it was the reproducer for this bug).
  • New test_parse_rg_handles_windows_drive_letter_paths feeds a NUL-delimited drive-letter line straight through _parse_rg, so it runs on every platform (CI is Linux-without-rg, which otherwise never exercises this code path). I confirmed it fails against the old parser, so it's a real regression guard.

Before / after (Windows 11, ripgrep 15.1.0)

$ pytest tests/test_code_tools.py::test_grep_finds_matches_and_respects_glob -q
>       assert "a.py" in files and "b.txt" in files
E       AssertionError: assert ('a.py' in {'C'})
1 failed in 1.81s
$ pytest tests/test_code_tools.py -q
...............                                          [100%]
15 passed in 1.69s

(Terminal screenshot of the same before/after run attached in the comments.)

Notes

  • --with-filename also removes a pre-existing silent-drop: when path names a single file (outside the documented "subdirectory" usage), ripgrep previously emitted no filename, so the parser returned count=0 with no error. The pure-Python fallback (_py_grep) still doesn't support single-file targets (os.walk on a file yields nothing) — out of scope here and worth a separate issue if single-file search should be supported.
  • Scope is otherwise limited to the ripgrep parser.
  • Running the full suite on Windows surfaces 7 unrelated, pre-existing failures (slack-relay socket timeouts, a shell-cwd test, a curated-models assertion). I verified they fail identically on main without this change, so they're not introduced here.

`_parse_rg` split ripgrep output on ":" positionally, so a Windows
absolute path like `C:\ws\a.py:12:def f()` parsed as file="C", line=0,
with the real line number swallowed into the matched text. Every grep
match on Windows (when `rg` is on PATH) came back corrupted.

Pass `--with-filename --null` so ripgrep always prints the path and
NUL-separates it from `line:text`, then split the path off on the NUL
byte — a byte that cannot appear in a path — before parsing. `--null`
defeats the drive-letter colon; `--with-filename` keeps the format
universal so a single-file target isn't silently dropped. Portable:
harmless on Linux/macOS, correct everywhere.

Adds a platform-independent regression test that feeds a NUL-delimited
drive-letter line through `_parse_rg` directly, since CI runs on Linux
without `rg` and never exercises this code path otherwise.

Closes andrewyng#17
@engmohamedsalah
engmohamedsalah force-pushed the fix/windows-rg-parser branch from d8ec995 to 0622e93 Compare July 25, 2026 12:48

@rajpratham1 rajpratham1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request improves cross-platform compatibility for ripgrep-based search by enabling --with-filename and --null output, allowing filenames to be separated from match data without relying on colon delimiters. The parser is updated to split on the NUL separator before extracting the line number and matched text, preventing Windows drive-letter paths (for example, C:...) from being misinterpreted as field separators. The accompanying regression test verifies the corrected parsing behavior for Windows-style absolute paths while remaining platform-independent. Based on the visible changes, the implementation is focused, addresses the parsing issue cleanly, and no blocking issues are apparent.

@engmohamedsalah

Copy link
Copy Markdown
Author

Friendly ping on this one — @rajpratham1 approved it back in July and the branch still merges cleanly (CLEAN, no conflicts, no failing checks). Is there anything else needed from my side, or could this be merged when you get a chance? Happy to rebase or adjust if anything has drifted since. Thanks!

@Dhevenddra

Copy link
Copy Markdown

Heads up, since this is your PR and I did not want to pile onto it: while testing this branch on Windows I ran into a second, separate problem in the same function, and I have filed it as #576.

Short version is that the exclusion globs (--glob !**/build/** and the rest) are matched against the absolute search path, so a workspace that happens to live under a directory named build, dist, venv, node_modules or AppData excludes itself entirely and rg exits 1 with no output.

Your --null change is the right fix for #17 and I verified it works against a normal workspace. The thing worth knowing is that because the cause there is the search path rather than the parsing, test_grep_finds_matches_and_respects_glob will still fail on Windows after this merges, since pytest's tmp_path lands under AppData on that platform.

Not trying to step on this one. It should go in. Happy to pick up #576 separately once this lands, or to leave it with you if you would rather do both together.

@engmohamedsalah

Copy link
Copy Markdown
Author

Thanks for testing this @Dhevenddra — appreciate the detailed writeup. Go ahead and leave #576 with me, I'll take it once this lands (it's in the same function so the context is fresh).

@engmohamedsalah

Copy link
Copy Markdown
Author

Follow-up: #576 is now fixed in #578, with a regression test. It touches the same _parse_rg that this PR rewrites, so I kept it as a separate PR to be rebased after this one lands — merging #123 first keeps both diffs clean. (Also confirms @Dhevenddra's finding that this fix doesn't change the behavior #17 was about.)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

grep tool returns corrupted results on Windows — ripgrep output parser splits on the drive-letter colon

3 participants