fix(tools): match grep_search's include glob against the relative path - #1675
Closed
Preciousuche wants to merge 1 commit into
Closed
Preciousuche wants to merge 1 commit into
Preciousuche wants to merge 1 commit into
Conversation
grep_search matched include against fp.name only
(fnmatch.fnmatch(fp.name, include)). fp.name never contains a
directory separator, so any path-qualified pattern -- "tests/*.py",
"src/**/*.ts" -- matched nothing, and grep_search silently reported
"No matches", indistinguishable to the caller from "this code does not
exist".
Matched against fp.relative_to(base).as_posix() instead. fnmatch's `*`
already spans `/` (fnmatch("src/a.py", "*.py") is True), so every
bare-filename pattern documented today keeps working exactly as
before; path-qualified patterns now also work instead of silently
matching nothing. Updated the include parameter's description, which
previously documented only the bare-filename form.
Fixes use-agent-os#1571
Contributor
|
Thanks for the PR. #1571 was fixed by #1848. Matching only the relative path regresses bare globs with a literal prefix: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
grep_search matched the include glob against fp.name only (fnmatch.fnmatch(fp.name, include))
fp.name never contains a directory separator, so any path-qualified pattern (tests/.py, src/**/.ts) matched nothing, and grep_search silently reported "No matches" — indistinguishable to the caller from "this code does not exist"
Matched against fp.relative_to(base).as_posix() instead
fnmatch's * already spans / (fnmatch("src/a.py", ".py") is True), so every bare-filename pattern documented today keeps working exactly as before; path-qualified patterns now also work instead of silently matching nothing
Updated the include parameter's description, which previously documented only the bare-filename form
Fixes #1571
Test plan
Verified the tests catch the regression: stashed the source fix and reran — the path-qualified tests fail against the old code with the exact reported symptom ("No matches for 'def test_'"), while the bare-filename regression test correctly passed either way
Added test_grep_search_include.py (4 tests): the issue's exact reproduction (tests/.py now matches), the required non-regression (.py still matches at any depth), a src/**/.py case documenting fnmatch's actual non-recursive ** semantics, and a path-qualified pattern that correctly reports no matches when nothing qualifies
uv run pytest tests/test_tools -k "filesystem or grep" -q — 25 passed, 4 skipped
uv run ruff check / uv run mypy clean on changed files