Skip to content

fix(tools): detect content-search truncation so the model paginates - #41439

Open
pprism13 wants to merge 1 commit into
NousResearch:mainfrom
pprism13:fix/search-truncation-flag
Open

fix(tools): detect content-search truncation so the model paginates#41439
pprism13 wants to merge 1 commit into
NousResearch:mainfrom
pprism13:fix/search-truncation-flag

Conversation

@pprism13

@pprism13 pprism13 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

search_files (target=content) silently dropped every match past the
first page. ShellFileOperations._search_with_rg and
_search_with_grep compute fetch_limit = limit + offset for the
non-context case and cap the pipeline with | head -n {fetch_limit}, so
the parsed match list can contain at most offset + limit rows. The
truncation flag is then derived as truncated = total > offset + limit
which can never be True because total is itself bounded by that same
cap. With the flag stuck at False, search_tool in tools/file_tools.py
never appends its "Results truncated. Use offset=… to see more" hint, so
the agent has no signal that more matches exist and stops after one page.

The fix fetches one extra sentinel row past the page window
(offset + limit + 1) — the standard n+1 pagination technique — so the
existing truncated = total > offset + limit comparison can actually
fire. The sentinel row is never returned to the caller because the page
is still sliced as matches[offset:offset + limit]. This also realigns
the code with the inline comment that already claimed it fetched "extra
rows so we can report the true total before slicing" — only the
context-mode branch (+ 200) was doing so.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/file_operations.py: in _search_with_rg, change
    fetch_limit to limit + offset + 1 + (200 if context > 0 else 0)
    so the non-context content path grabs the sentinel row needed for
    truncation detection (context mode keeps its + 200 slack).
  • tools/file_operations.py: apply the same + 1 to the fetch_limit
    in _search_with_grep for parity across the rg and grep backends.
  • tests/tools/test_file_operations.py: add TestSearchContentTruncation
    driving both backends through the real local terminal env — asserts
    truncated is True when matches exceed the page and stays False on an
    exact fit, and that the sentinel row never leaks into the returned page.

How to Test

  1. Create a file with 10 matching lines, e.g.
    printf 'MATCH %d\n' $(seq 0 9) > /tmp/hits.txt.
  2. Run a content search with a page smaller than the match count:
    ShellFileOperations(env).search("MATCH", path="/tmp", target="content", limit=3).
    Before: truncated=False, total_count=3 (7 matches lost silently).
    After: truncated=True, page holds lines 1–3, and search_tool
    emits the offset=3 continuation hint.
  3. Repeat with limit=10 against the same file and confirm
    truncated stays False — the exact-fit boundary must not false-positive.
  4. pytest tests/tools/test_file_operations.py -q (the new
    TestSearchContentTruncation cases run against both rg and grep).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the file-operations and search suites; the pre-existing failures are unrelated env-isolation cases in test_file_tools.py
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.5)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

## What does this PR do?

`search_files` (target=`content`) silently dropped every match past the
first page. `ShellFileOperations._search_with_rg` and
`_search_with_grep` compute `fetch_limit = limit + offset` for the
non-context case and cap the pipeline with `| head -n {fetch_limit}`, so
the parsed match list can contain at most `offset + limit` rows. The
truncation flag is then derived as `truncated = total > offset + limit` —
which can never be True because `total` is itself bounded by that same
cap. With the flag stuck at False, `search_tool` in `tools/file_tools.py`
never appends its "Results truncated. Use offset=… to see more" hint, so
the agent has no signal that more matches exist and stops after one page.

The fix fetches one extra sentinel row past the page window
(`offset + limit + 1`) — the standard n+1 pagination technique — so the
existing `truncated = total > offset + limit` comparison can actually
fire. The sentinel row is never returned to the caller because the page
is still sliced as `matches[offset:offset + limit]`. This also realigns
the code with the inline comment that already claimed it fetched "extra
rows so we can report the true total before slicing" — only the
context-mode branch (`+ 200`) was doing so.

## Related Issue

N/A

## Type of Change

- [x] 🐛 Bug fix (non-breaking change that fixes an issue)
- [ ] ✨ New feature (non-breaking change that adds functionality)
- [ ] 🔒 Security fix
- [ ] 📝 Documentation update
- [ ] ✅ Tests (adding or improving test coverage)
- [ ] ♻️ Refactor (no behavior change)
- [ ] 🎯 New skill (bundled or hub)

## Changes Made

- `tools/file_operations.py`: in `_search_with_rg`, change
  `fetch_limit` to `limit + offset + 1 + (200 if context > 0 else 0)`
  so the non-context content path grabs the sentinel row needed for
  truncation detection (context mode keeps its `+ 200` slack).
- `tools/file_operations.py`: apply the same `+ 1` to the `fetch_limit`
  in `_search_with_grep` for parity across the rg and grep backends.
- `tests/tools/test_file_operations.py`: add `TestSearchContentTruncation`
  driving both backends through the real local terminal env — asserts
  `truncated` is True when matches exceed the page and stays False on an
  exact fit, and that the sentinel row never leaks into the returned page.

## How to Test

1. Create a file with 10 matching lines, e.g.
   `printf 'MATCH %d\n' $(seq 0 9) > /tmp/hits.txt`.
2. Run a content search with a page smaller than the match count:
   `ShellFileOperations(env).search("MATCH", path="/tmp", target="content", limit=3)`.
   Before: `truncated=False`, `total_count=3` (7 matches lost silently).
   After: `truncated=True`, page holds lines 1–3, and `search_tool`
   emits the `offset=3` continuation hint.
3. Repeat with `limit=10` against the same file and confirm
   `truncated` stays False — the exact-fit boundary must not false-positive.
4. `pytest tests/tools/test_file_operations.py -q` (the new
   `TestSearchContentTruncation` cases run against both rg and grep).

## Checklist

### Code

- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits (`fix(scope):`, `feat(scope):`, etc.)
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix/feature (no unrelated commits)
- [x] I've run the file-operations and search suites; the pre-existing failures are unrelated env-isolation cases in `test_file_tools.py`
- [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features)
- [x] I've tested on my platform: macOS 15 (Darwin 25.5)

### Documentation & Housekeeping

- [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A
- [x] I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
- [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Reviewed PR #41439: fix(tools): detect content-search truncation so the model paginates

✅ Approve

  • Well-scoped behavior change. Truncation flag enables the model to continue paginating instead of silently consuming incomplete content.
  • Tool-side test coverage added for both truncated and complete result paths.

💡 Suggestion

  • Consider surfacing the truncation reason in the returned tool output so higher-level flows don’t need to guess why pagination happened.

Reviewed in batch on 2026-06-08

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) labels Jun 7, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the default content-mode off-by-one. The premise is confirmed on current main: tools/file_operations.py:2288 caps rg at offset + limit, while :2384 checks total > offset + limit; grep mirrors this at :2416 and :2507. The n+1 sentinel fixes that default branch without false-positive exact fits.

Problems

  • The sentinel is not used by files_only or count: those branches still return truncated=bool(limit_reason) (tools/file_operations.py:2317-2342, :2443-2468). A successful head-capped result therefore remains unmarked in those content-search output modes.

Suggested changes

  • Carry the n+1 detection through both output modes for rg and grep, using parsed row count for count mode.
  • Add focused coverage for those four mode/backend combinations. This also addresses the related approach identified in the cross-reference comment on fix(search): detect truncation with overflow sentinel #58210.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants