Skip to content

fix(read): fill the --max-lines budget exactly instead of stopping at half - #2964

Open
xuing wants to merge 1 commit into
rtk-ai:developfrom
xuing:fix/read-max-lines-budget
Open

xuing wants to merge 1 commit into
rtk-ai:developfrom
xuing:fix/read-max-lines-budget

Conversation

@xuing

@xuing xuing commented Jul 12, 2026

Copy link
Copy Markdown

Summary

  • smart_truncate kept non-important lines only while kept < max_lines / 2 and broke the loop at max_lines - 1, so --max-lines 4 on plain text returned 2 lines (the hook maps head -4 to exactly this call)
  • The budget is now filled to exactly max_lines: important lines (signatures, imports, pub/export, braces) claim it first — unchanged priority — and the earliest remaining lines fill whatever is left, in original file order
  • [N more lines] still satisfies kept + N == total

Fixes #2961

Implementation notes

  • Same single function, same "no synthetic annotations" contract, same important-line heuristics — only the budget accounting changes (one selection pass, then fill)
  • max_lines == 0 returns just the marker instead of underflowing max_lines - 1

Test plan

  • test_smart_truncate_fills_budget_exactly — 10 plain lines, --max-lines 4 → line1..line4 + [6 more lines] (was 2 lines)
  • test_smart_truncate_important_lines_keep_priority — important lines still win the budget, remainder filled in file order
  • Updated test_smart_truncate_no_annotations / test_smart_truncate_overflow_count_exact for the new (correct) kept counts; the invariant they assert (kept + reported == total) is unchanged
  • End-to-end: rtk read ten.txt --max-lines 4 → 4 lines + [6 more lines]
  • cargo fmt --check && cargo clippy --all-targets && cargo test — 2441 passed, 0 failed, no new warnings

… half

smart_truncate kept non-important lines only while kept < max_lines/2 and
broke out of the loop at max_lines-1, so an explicit --max-lines 4 on plain
text returned 2 lines. The budget is an explicit request (rtk read
--max-lines N, and the hook's head -N mapping) — important lines still
claim it first, the earliest remaining lines now fill it to exactly N.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 12, 2026 07:25
@CLAassistant

CLAassistant commented Jul 12, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Copilot AI 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.

Pull request overview

This PR fixes core::filter::smart_truncate so --max-lines N returns exactly N content lines (plus the single [N more lines] marker), instead of stopping early (often at N/2) for plain text—matching the expected behavior for the hook’s head -N mapping.

Changes:

  • Reworks smart_truncate line selection to (1) reserve budget for “important” lines first, then (2) fill remaining budget with earliest remaining lines in original order.
  • Adds an explicit max_lines == 0 fast-path to avoid underflow and return only the marker.
  • Updates/extends unit tests to assert the exact budget fill and preserve the kept + omitted == total invariant.

Review findings (should address)

  • Performance/memory regression risk: the new approach allocates keep: Vec<bool> sized to lines.len() and does an additional full pass to fill budget. Given this is in a core filtering path, consider tracking kept indices in a Vec<usize> (capacity max_lines) and emitting in-order (e.g., sort indices, then iterate once), or using a compact bitset, to avoid an extra full-length allocation.
  • Test robustness: multiple tests identify the marker line via !l.contains("more lines"). That can accidentally drop legitimate content lines containing that substring and make the test pass incorrectly. Prefer matching the marker more precisely (e.g., l.starts_with('[') && l.ends_with(" more lines]")) or asserting the last line matches the marker format and excluding only the last line.

@KuSh

KuSh commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

#3941 (merged 2026-09-13) made the head and tail rewrites faithful (head -N now maps to rtk read --head-lines N) and added a max_lines == 0 guard to smart_truncate. What develop still lacks is the budget fix itself: smart_truncate in src/core/filter.rs still keeps non-important lines only while kept_lines < max_lines / 2, so rtk read ten.txt --max-lines 4 returns 2 lines, along with your two new tests and the corrected expectations in the existing smart_truncate tests. Could you rebase onto current develop and narrow the PR to that change? The only conflict should be at the top of the function, where your max_lines == 0 marker can give way to the guard develop already has.

One optional simplification, not a blocker: the keep vector can go. When the input is truncated the window is always exactly max_lines lines, so a first pass only needs to count the important lines (capped at max_lines), which leaves max_lines - important slots for plain lines. A second pass then emits, in file order, every important line and each plain line while those slots last, stopping after max_lines lines, and the marker becomes [{lines.len() - max_lines} more lines]. The plain-line budget has to count plain lines seen, not the line index: with }, x1, x2, } and max_lines = 3, an index cutoff drops x1. We tried this shape against your version on every combination of up to 12 short lines and max_lines 1 to 14, and the outputs were identical, so it is safe if you want it. Thanks for tracking this down.

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.

rtk read --max-lines N returns only N/2 lines of plain text (hook's head -N loses half the file)

4 participants