Skip to content

fix: make grep truncation exhaustively recoverable - #905

Open
kvandre12-commits wants to merge 1 commit into
mpfaffenberger:mainfrom
kvandre12-commits:fix/exhaustive-grep-pagination
Open

kvandre12-commits wants to merge 1 commit into
mpfaffenberger:mainfrom
kvandre12-commits:fix/exhaustive-grep-pagination

Conversation

@kvandre12-commits

@kvandre12-commits kvandre12-commits commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

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.

  • add next_offset to bounded grep results
  • accept offset on the registered grep tool and both local/backend implementations
  • sort local ripgrep output by path so repeated calls traverse deterministically
  • keep context rows outside the 50-real-match page budget
  • define completeness precisely: repeat the same query/directory with next_offset until it is null

Why

truncated=True prevents 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

  • focused grep/backend/rendering/i18n suite: 287 passed in 5.68s
  • changed Python files: ruff format --check clean
  • scoped import autofix: ruff check --fix --select I clean
  • git diff --check clean

Direct 441-match A/B receipt against current origin/main

Observation Current main (#904) Candidate
First response 50, truncated=True 50, truncated=True, next_offset=50
Continuation call rejected (TypeError) accepted
Page sizes 50 only 50 x 8 + 41
Total recovered 50/441 441/441
Omissions 391 0
Duplicates 0 0
Exact deterministic order not applicable yes
Terminal state no continuation truncated=False, next_offset=None

Environment 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.

# 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
@kvandre12-commits
kvandre12-commits force-pushed the fix/exhaustive-grep-pagination branch from cc7c204 to 61fb011 Compare September 30, 2026 00:57
@kvandre12-commits

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (306 commits had landed since this was last synced, including the configurable grep_max_matches budget from #906 and the --sort path/deterministic-ordering groundwork).

Notable conflict resolutions, not just mechanical:

  • The old hardcoded 50-match cap is gone in favor of the configurable get_grep_max_matches() budget everywhere (backend path, ripgrep path, context-row guard) -- this PR's pagination now respects whatever budget is configured instead of reintroducing a fixed 50.
  • Main had independently grown its own truncation warning in the renderer (renderer.truncated) while this branch had its own, more actionable one (grep.results_truncated, which tells the agent to page with next_offset instead of just saying more exists). The merge briefly printed both; kept the more actionable one and removed the duplicate.
  • Caught and removed a duplicate truncated=truncated keyword argument that the 3-way merge introduced into the GrepResultMessage(...) call (would have raised TypeError at runtime -- good thing tests catch that kind of thing).
  • Kept the exhaustive multi-page test (test_grep_exhausts_all_pages_without_duplicates_or_omissions) and folded the redundant hardcoded-50 duplicate test into the existing configurable-budget equivalent.

Local: 228 passed across the touched test files, ruff format clean.

@thomwebb thomwebb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.truncated declaration (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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

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.

2 participants