Repository navigation
Conversation
The grep tool advertises include="*.{ts,tsx}", but such calls returned no
matches: ripgrep's -g selected the right files, then _finalize_matches
re-applied _path_matches_filters, whose include check was
fnmatch.fnmatch(basename, pattern). fnmatch has no brace expansion, so
every match was dropped. The same post-filter is the only include filter
for the system grep and Python backends, and it also silently rejected
any include containing a path separator (sub/*.ts, **/*.py).
Expand {a,b} alternatives (nested groups included) and translate each
glob to a segment-aware regex: `*` and `?` stay within one path segment
while a whole-segment `**` spans zero or more. Globs with a slash match
the path relative to the search root; globs without one match the file
name, mirroring ripgrep. ripgrep now runs from the search root because it
resolves path-bearing globs against its working directory, so all three
backends agree on brace and path includes.
Co-authored-by: openhands <[email protected]>
Co-Authored-By: Claude Code <[email protected]>
Adjacent globstar segments such as "**/" repeated compile to nested (?:[^/]*/)* repetitions. A failing match then has to enumerate every way to distribute path segments across those repetitions: a 20x "**/" include took 2.2s / 15.4s / 87s for 10 / 12 / 14 segment paths. Merge adjacent `**/` segments before compiling. `**/**/` matches the same paths as `**/`, so normal-pattern semantics are unchanged, while the pathological include now matches in microseconds. Co-authored-by: openhands <[email protected]> Co-Authored-By: Claude Code <[email protected]>
|
|
@BlueX888 Could you fill the HUMAN section? |
|
Hey @BlueX888, following up on my comment from yesterday: the description check is still failing. It needs a short note in your own words under |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
In scope: this repository owns the grep tool (openhands-tools), and the linked tracker #6348 (ready-for-dev) pins exactly this defect — "*.{ts,tsx} is filtered with fnmatch, which has no brace expansion". The change is contained to the include matcher in openhands-tools/openhands/tools/grep/impl.py plus tests. No product/architecture decision needed.
Verification on the current head c4521ada
I re-ran the PR's own scenarios against real ripgrep 15.1.0 (local reproduction; CI runners do not install rg, so the ripgrep-marked tests are skipped there — the tools-tests job installs only uv deps):
- The brace/path matcher itself is correct. The head's Python backend and the head's ripgrep backend (now run with
cwd=search_path) return identical file sets for 18 include patterns I cross-checked (*.{ts,tsx},sub/*.ts,**/*.ts,src/*.{py,md},level1/**/*.ts, classes,?, nested braces). Hidden-directory exclusion is preserved (_path_matches_filtersruns after the include check and still rejects dot-directories), and the**/-run collapse removes the catastrophic backtracking on the("**/"*20)+"*.ts"case (~0.02–0.12 ms on a 14-segment path). - The Python and system-grep backends agree on the new semantics; path-bearing globs now match relative to the search root as documented.
Material finding: the head regresses two fixes already merged to main
The branch last merged main at e12c9615 (Nov 3). Since then, main gained two fixes to the same _execute_with_ripgrep method, and the current head dropped both:
-ebefore the pattern (#6537, merged Nov 7 11:59Z): without it, ripgrep parses a leading-dash pattern as CLI flags. Demonstrated on this head with realrg:include=None, pattern="-foo"→ head returns "No files found" (main returnsflags.txt).pattern="--files"→ head lists every file in the tree (main returns[]). That is user-controlled flag injection into thergargv, previously fixed and now reintroduced.
- Python-search fallback when ripgrep exits ≠ 0/1 (#6593, merged Nov 7 18:17Z): a pattern
re.compileaccepts but rust-regex rejects (e.g.(?<=a)b) makesrgexit 2; the head silently reports "No files found" whilemainlogs a warning and falls back to the Python search, which finds the match.
Both fixes' regression tests (test_pattern_starting_with_dash, test_ripgrep_pattern_cannot_inject_flags) exist on main and are absent from the head's test_grep_executor.py. A plain three-way merge of main into this branch auto-restores both hunks (I verified in a throwaway worktree: the merged file contains -e, the fallback, and the new include matcher simultaneously), but the head as it stands is behind main — and a squash merge of this head would lose both fixes outright. Please re-merge main (and keep the two hunks) before this is merged.
Other checks
Validate PR descriptionis failing: theHUMAN:section still holds the placeholder. This is already known to the author (VascoSch92's comments); it gates the merge but is not a code defect.- PR body's own scope notes are accurate: the SDK terminal
grepfallback inopenhands-sdkis a separate implementation and is correctly left untouched; the Windows case-sensitivity change matches ripgrep and is covered by the passingwindows-testsjob. - All other checks on
c4521adapass, includingtools-tests,check,pre-commit, andsecurity.
With the main re-merge in place, I see no remaining correctness, security, or compatibility defect in the include-matcher change itself.
🔄 CHANGES REQUESTED
|
Merged |
|
Hey @BlueX888, thank you for the work on this and for keeping the branch up to date with main. The That change doesn't cover the part where your PR went further: includes with a path, like |
|
Thanks for the clear pointer. I opened #6680 as the smaller follow-up: it keeps brace expansion from #6636 and only adds path-bearing include support ( |
HUMAN:
AGENT:
Why
The grep tool documents
include="*.{ts,tsx}"as the canonical filter example (openhands-tools/openhands/tools/grep/definition.py), but that call never returned a match. ripgrep receives-g <include>and selects the right files;_finalize_matchesthen re-applies_path_matches_filters, whose include check wasfnmatch.fnmatch(filename, include_pattern)on the basename only:fnmatchhas no brace expansion, so every match for*.{ts,tsx}was dropped: "No files found ... (filtered by '*.{ts,tsx}')".sub/*.ts,**/*.py) were matched against the basename too, so they were silently empty.Summary
{a,b}alternatives (including nested groups) and translate each glob to a segment-aware regex:*/?stay within one segment, a whole-segment**spans zero or more, character classes are supported./now match the path relative to the search root; globs without one match the file name — the same split ripgrep uses. ripgrep now runs withcwd=search_pathso its-gresolves path-bearing globs against the search root too, and all three backends agree.**/segments before compiling. Without this,("**/" * 20) + "*.zzz"compiled to nested(?:[^/]*/)*repetitions and took ~105 s on a 14-segment path; it now matches in ~0.1 ms.Issue Number
Part of #6348 — this PR fixes the brace/path include case (
*.{ts,tsx},sub/*.ts) from the edge-case bug collection.How to Test
Fixture with
a.ts,b.tsx, and files whose content matches the pattern:Included tests:
tests/tools/grep/test_grep_executor.py(ripgrep, system grep and Python backends for brace and path patterns; pathological**/timing),tests/tools/grep/test_consistency.py(all backends return the same files),tests/tools/grep/test_grep_tool.py(tool-level brace include).Before/after evidence: on
07e8c26the eight new assertions fail with empty match lists (assert 0 == 2); with this branchtests/tools/grepis 59 passed. Baseline vs candidate acrosstests/tools: 898 passed, 9 failures identical on baseline (browser-use and task MCP, unrelated).ruff check/ruff format --checkclean. An independent fuzz comparison againstrg -gover 1,643 patterns found no semantic difference for the supported syntax.Video/Screenshots
Not visual — the commands and outputs above are the evidence.
Design Doc
Not needed: the change is contained to the include matcher in one file plus tests.
Notes on scope and behavior:
!negation, backslash-escaped literals such as\[, leading/anchors) keeps its previous behavior and is not addressed here.fnmatchwas case-insensitive there).*.tsno longer matchesX.TS; ripgrep treats that pattern as case-sensitive on all platforms.grepfallback (openhands-sdk/.../agent/utils.py::_GREP_FALLBACK_SCRIPT) is a separate embedded implementation and still usesfnmatchon the basename; this PR only covers thegreptool's include filter.**/runs (the reported trigger). Non-adjacent adversarial sequences such as repeated"**/a/"segments can still backtrack in theory; closing that would need a different matcher and is left out of this focused fix.