Repository navigation
fix: make grep truncation exhaustively recoverable - #905
kvandre12-commits wants to merge 1 commit into
Conversation
# Conflicts: # code_puppy/i18n/locales/en-US.json # code_puppy/messaging/rich_renderer.py # code_puppy/tools/file_operations.py # code_puppy/tools/tools_content.py # tests/tools/test_grep_live_behavior.py
cc7c204 to
61fb011
Compare
|
Rebased onto current main (306 commits had landed since this was last synced, including the configurable Notable conflict resolutions, not just mechanical:
Local: 228 passed across the touched test files, ruff format clean. |
thomwebb
left a comment
There was a problem hiding this comment.
[REQUESTING CHANGES]
Reviewed 61fb011 against live base c705664; both unchanged at publication. Authenticated reviewer account is thomwebb, distinct from author kvandre12-commits. Posted on TJ's authorization from Onyx's completed independent review. No active assignee lock or existing native review was found.
P2: retain requested trailing context at the real-match budget
code_puppy/tools/file_operations.py:1328–1334, specifically page_match_count >= max_matches at line 1331.
The new context-event filter drops requested -A rows as soon as the real-match budget is full. Context rows are supposed to remain outside that budget. On an exactly-at-budget search, the last requested context row is lost even though the response reports truncated=False and next_offset=None, so no continuation can recover it.
Minimal reproduction: set the grep match budget to 3 (or patch code_puppy.config.get_grep_max_matches to return 3 in a test), write this six-line UTF-8 file into a searchable directory:
hit 1
after1
hit 2
after2
hit 3
after3
Then call _grep(None, '-A 1 hit', directory):
| Revision | Returned line numbers | Terminal state |
|---|---|---|
| Exact live base | 1, 2, 3, 4, 5, 6 | truncated=False |
| Candidate | 1, 2, 3, 4, 5 | truncated=False, next_offset=None |
Rows 2/4/6 are after-context, not real matches. The candidate omits after3; an independent exact-base module probe returns it. This is a candidate regression, not an existing backend limitation. A four-hit fixture also moves the previous page's trailing after-context onto the next page.
Please fix the context/page boundary so requested context for the final returned real match is retained, while preserving _MAX_GREP_CONTEXT_ROWS, real-match-only offsets, and the rule that only an additional real match proves truncation. Add an exactly-at-budget trailing-context regression test (and page-boundary coverage). Do not remove the context safety cap or charge context rows against the real-match budget.
What works / nonblocking caveats
- Static real-match pagination works: a bounded 15-match fixture across three files recovered all 15 exactly once in five pages of 3. Repeat traversal was identical; the reference backend returned the same flat-fixture order and pages. UTF-8 filename/content and CRLF input were included. No ordinary unchanged-result omission/duplicate issue is established.
- Predecessor #904 is merged and its merge commit is an ancestor of current live main. Its truncation/context requirements remain relevant; this PR is not waiting on an unmerged predecessor.
- Later offsets rescan and capture more ripgrep output before trimming. With four files of 120 matches each, budget 3, offsets 0/60 captured 4,154/45,593 stdout bytes. This is a bounded performance observation, not a merge blocker or request for broad streaming/cursor redesign; baseline already uses full subprocess capture.
- Completeness assumes unchanged file contents, not just unchanged query/directory. Deleting an earlier match between pages can skip a still-existing match. This is an expected live offset-pagination limitation; document it without introducing snapshot machinery here.
- Negative offset validation works; a large offset over the small fixture returns an empty terminal page. Direct fractional calls lie outside the registered integer tool contract; no blocker established through registered validation.
- The added duplicate
GrepResultMessage.truncateddeclaration (messages.py:155–158 versus existing 163–166), hardcoded “50” in the new warning/description, and remaining “narrow the search” text alongside same-query pagination instructions are small nonblocking cleanup/clarity items. The duplicate field does not establish a runtime TypeError: the latter declaration wins, and rendering tests pass. - Existing backend unsupported-context/encoding policies are not pagination regressions; no unrelated behavior rewrite or security-tripwire weakening requested.
Validation and limitations
253 focused tests passed locally: 223 across test_grep_live_behavior, test_file_operations_coverage, test_fs_backend and test_rich_renderer; 30 in test_i18n_audit. Ruff lint and format checks passed on all eight changed Python files; diff whitespace check passed. The separate synthetic probe and exact-live-base comparison reproduce the context defect despite those passing tests.
Tests ran on macOS arm64 / Python 3.13.5, in an isolated uv environment from the frozen lock, with isolated HOME/configuration, socket/keyring blockers, and no real LLM/credential calls. Candidate source was not edited. Probes were deliberately bounded, not unbounded benchmarks.
Fresh live checks are genuinely green on this exact head: quality, test (macos-latest, 3.13), windows-encoding. No Linux test context was reported. Windows encoding success does not establish Windows pagination execution; Windows/Linux runtimes were not locally run. The reproduced context failure is platform-independent Python control flow. Nested cross-platform collation, invalid-UTF-8 event parsing and full-suite behavior were not exhaustively validated.
Recommendation: request changes for the single demonstrated trailing-context regression; scaling, changing-file limitations and cleanup nits remain nonblocking. No code, merge, assignment/label or thread-resolution actions accompany this review.
| if context_row_count >= _MAX_GREP_CONTEXT_ROWS: | ||
| if ( | ||
| seen_match_count < offset | ||
| or page_match_count >= max_matches |
There was a problem hiding this comment.
P2: This condition drops requested trailing -A context after the final budgeted match. With budget 3 and rows hit 1 / after1 / hit 2 / after2 / hit 3 / after3, _grep(None, "-A 1 hit", directory) returns rows 1–5, truncated=False and next_offset=None; exact live base returns rows 1–6. Please retain context for the final returned match without consuming real-match offset/budget or weakening _MAX_GREP_CONTEXT_ROWS, and add an exactly-at-budget trailing-context regression test.
Summary
Follow-up to #904. That PR made the 50-match cap visible through
truncated; this PR makes the remaining results recoverable without changing query semantics.next_offsetto bounded grep resultsoffseton the registered grep tool and both local/backend implementationsnext_offsetuntil it is nullWhy
truncated=Trueprevents a false completeness claim, but “narrow the search” changes the query and cannot prove that every result from the original search was observed. Offset pagination preserves the bounded response while allowing exhaustive traversal of the unchanged search field.Verification
287 passed in 5.68sruff format --checkcleanruff check --fix --select Icleangit diff --checkcleanDirect 441-match A/B receipt against current
origin/maintruncated=Truetruncated=True,next_offset=50TypeError)50 x 8 + 41truncated=False,next_offset=NoneEnvironment notes
Verification ran on Android/Termux with Python 3.14.6. Ruff 0.15.13 (the repository CI pin) cannot build on Android; the available Ruff 0.16.3 reports pre-existing full-tree findings, so Linux CI remains authoritative for the pinned lint job. A broad non-browser test run reached platform-specific failures unrelated to grep; all directly affected coverage passes in the focused 287-test run.