fix(grep): dedupe overlapping context windows; honor max_matches=0 - #167
Conversation
Two fixes in the grep tool (vedaant00#165): - each match re-emitted its full context window with no tracking, so overlapping windows duplicated lines (5-line file produced 11); a per-file printed set now emits each line once - the cap was checked after appending, so max_matches=0 returned one match labelled '(capped at 0)'; the check now runs before emitting Tests: three regression tests in tests/test_tools.py (overlap dedupe, max_matches=0, max_matches=1 still includes the first hit). Note: two existing context tests fail on Windows due to path separators in _rel() output ('src\a.py' vs 'src/a.py'); that is pre-existing on main and unrelated to this change.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Overlapping matches can retain an incorrect context marker, and the regression test does not assert marker correctness.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates grep to deduplicate overlapping context output and correctly honor max_matches=0.
Changes:
- Deduplicates overlapping context lines.
- Checks match caps before emitting results.
- Adds regression tests for overlap and match limits.
| File | Description |
|---|---|
tests/test_tools.py |
Adds regression coverage for deduplication and match caps. |
src/opendot/tools/local.py |
Implements context deduplication and cap handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ches Copilot review on the PR: when an overlapping context window reached a line already emitted as context (j in printed) and that line was the current match (j == i), the entry kept its dash marker, so the fixture returned f.txt:3-aaa instead of f.txt:3:aaa — a real match hidden as context, contradicting the docstring's path:line:text format. The entry is now rewritten to the colon form, and the overlap regression test asserts every marker.
|
Addressed the Copilot finding in the latest commit: when an overlapping context window reaches a line already emitted as context and that line is the current match ( The overlap regression test asserts every marker explicitly: |
The logic changes were correct but three lines exceeded the configured width, so CI's lint job failed while the three test jobs passed. No behavior change: the dedupe, the pre-append cap check and the context-to-match marker upgrade all produce identical output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Nice work @t957095, logic is correct on all three counts, and I like that the overlap test asserts every marker rather than just counting lines. Verified locally: output now matches real grep -C2 exactly, max_matches=0 returns no matches, and cross-file line collisions are handled. Both new tests fail on pre-fix code and pass with it. Only the lint job was red (three lines over the width limit). I pushed a ruff format commit to your branch, no behavior change. Good to merge once CI is green. And yes please on the Windows _rel() separator follow-up, that's a real gap our Linux-only CI can't see. |

Both halves of #165:
context=2emitted 11 lines. A per-fileprintedset now emits each line at most once.max_matches=0returned one match labelled(capped at 0). The cap was checked after appending; the check now runs before a match is emitted, so0yieldsno matchesandmax_matches=1still returns the first hit plus the cap note.Tests: three regression tests added (overlap dedupe emits each line once and in order;
max_matches=0;max_matches=1keeps the first hit). All pass.Note:
test_grep_context_lines_returns_neighborsandtest_grep_context_zero_is_default_and_no_context_markersfail on Windows on clean main too —_rel()emits OS-native separators (src\a.pyvs the assertedsrc/a.py). Pre-existing, unrelated to this change; happy to send a small follow-up normalizing separators if you want.Closes #165