Changed: Replace L<n>: line prefixes with space-padded <n>: across read and grep output#112
Changed: Replace L<n>: line prefixes with space-padded <n>: across read and grep output#112
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 80.98% 81.02% +0.03%
==========================================
Files 110 110
Lines 4517 4537 +20
==========================================
+ Hits 3658 3676 +18
- Misses 859 861 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThe PR introduces branding guidance documentation and updates line-number formatting across the codebase. It changes the line prefix format from Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BRANDING.md (1)
1-164:⚠️ Potential issue | 🟠 MajorThis file appears out-of-scope for the stated PR objective.
The PR objective is line-prefix formatting changes in
read/greptooling, while this adds branding strategy documentation. Please split this into a separate PR (or update the PR objective/scope) to keep review and release risk bounded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BRANDING.md` around lines 1 - 164, The PR includes an unrelated file BRAN DING.md (branding doc) that is out-of-scope for the read/grep formatting changes; remove BRAN DING.md from this branch or stage so it is not part of this commit, or move the BRANDING.md addition to a new branch/PR and update this PR's description to reflect only the read/grep line-prefix formatting changes; ensure the commit history and diff for functions/files like the read/grep tooling (the original target files) no longer contain BRANDING.md so the reviewer sees only the intended changes.
🧹 Nitpick comments (3)
src/BRANDING.md (1)
156-160: Crate-count inconsistency in migration checklist.Line 156 lists 6 target crate names, but Line 159 says “across all 5 crates.” Please clarify whether
reloaded-codeis a new additional crate or adjust the count/text to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BRANDING.md` around lines 156 - 160, The checklist text is inconsistent: you list six crate names (`reloaded-code`, `reloaded-code-core`, `reloaded-code-agents`, `reloaded-code-serde-ai`, `reloaded-code-bubblewrap`, `reloaded-code-models-dev`) but later say “across all 5 crates”; update the document to be consistent by either confirming `reloaded-code` is an additional crate and changing “5 crates” to “6 crates”, or removing `reloaded-code` from the initial list if it’s not part of the migration, and then ensure the migration instruction `llm-coding-tools-* -> reloaded-code-*` references the correct set of crates.src/llm-coding-tools-serdesai/src/tools/grep.rs (1)
350-350: Stale comment: line format no longer has the" "prefix.The comment still describes the old
" L<n>:"shape. Under the new format the prefix is just a (possibly space-padded) number followed by": ", so for a single match here it's"1: content". The test logic itself is fine because it locates"prefix_"directly, but the comment is misleading.📝 Suggested wording
- // Line format is " 1: content", so actual content is line.len() - prefix + // Line format is "<padded line_no>: content"; locate the actual content + // via the "prefix_" marker rather than a fixed offset. let content_start = line.find("prefix_").unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/src/tools/grep.rs` at line 350, The comment on the line in grep.rs describing the line format is stale—update the comment that currently reads "// Line format is " 1: content", so actual content is line.len() - prefix" to reflect the new format where the prefix is a possibly space-padded number followed by ": " (e.g., "1: content"); reference the same prefix calculation used by the test logic (look for the prefix_ variable/logic in the surrounding code) and reword the comment to say something like "Line format is '<number>: content' (number may be space-padded), so the content starts after the '<number>: ' prefix."src/llm-coding-tools-core/src/tools/read.rs (1)
254-265: Width is derived from the requested limit, not the lines actually emitted — can over-pad short reads.
line_number_widthis computed fromlast_line = offset + limit - 1, so a small file read with the defaultlimit = 2000always gets a 4-character-wide column, even when only a handful of lines come back. For a 5-line file under default settings, every emitted line gets 3 leading spaces of padding it doesn't need — which slightly works against the PR's stated goal of saving one token per line.This is a real tradeoff against the streaming design (the actual final line count isn't known until EOF without buffering), and
grep.rsdoesn't have this issue because it can iterate matches first. A couple of options if you want to tighten this up later:
- Have callers pass a more realistic "expected last line" (e.g., when the caller knows the file size or already has line-count metadata).
- Buffer the numbered lines into a
Vec<(usize, String)>and emit after EOF; only worth it if benchmarks justify the extra allocation.Happy to leave as-is for now if the simplicity is the priority — just calling out the asymmetry with
grep.rsand the fact that the documented goal isn't fully realized for short reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/read.rs` around lines 254 - 265, The current computation of line_number_width uses last_line = offset.saturating_add(limit).saturating_sub(1) which derives width from the requested limit (limit) rather than the actual number of emitted lines and over‑pads short reads; fix by basing the width on the actual lines emitted: either (A) buffer the emitted lines into a Vec<(usize, String)> while streaming, compute last_line = offset.saturating_add(lines.len()).saturating_sub(1), then compute line_number_width and build the final output String with correct padding (update uses of line_number_width, estimated_capacity, and output), or (B) add an optional parameter (e.g., expected_last_line) callers can pass when they know file size and use it instead of limit; choose one approach and remove the current last_line = offset + limit - 1 usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/BRANDING.md`:
- Line 3: The heading "Project Read" is misworded; change the Markdown heading
text from "Project Read" to a clearer option such as "Project Brief" or "Project
Overview" (update the line containing the heading "Project Read" in BRANDING.md
to the chosen wording).
---
Outside diff comments:
In `@src/BRANDING.md`:
- Around line 1-164: The PR includes an unrelated file BRAN DING.md (branding
doc) that is out-of-scope for the read/grep formatting changes; remove BRAN
DING.md from this branch or stage so it is not part of this commit, or move the
BRANDING.md addition to a new branch/PR and update this PR's description to
reflect only the read/grep line-prefix formatting changes; ensure the commit
history and diff for functions/files like the read/grep tooling (the original
target files) no longer contain BRANDING.md so the reviewer sees only the
intended changes.
---
Nitpick comments:
In `@src/BRANDING.md`:
- Around line 156-160: The checklist text is inconsistent: you list six crate
names (`reloaded-code`, `reloaded-code-core`, `reloaded-code-agents`,
`reloaded-code-serde-ai`, `reloaded-code-bubblewrap`,
`reloaded-code-models-dev`) but later say “across all 5 crates”; update the
document to be consistent by either confirming `reloaded-code` is an additional
crate and changing “5 crates” to “6 crates”, or removing `reloaded-code` from
the initial list if it’s not part of the migration, and then ensure the
migration instruction `llm-coding-tools-* -> reloaded-code-*` references the
correct set of crates.
In `@src/llm-coding-tools-core/src/tools/read.rs`:
- Around line 254-265: The current computation of line_number_width uses
last_line = offset.saturating_add(limit).saturating_sub(1) which derives width
from the requested limit (limit) rather than the actual number of emitted lines
and over‑pads short reads; fix by basing the width on the actual lines emitted:
either (A) buffer the emitted lines into a Vec<(usize, String)> while streaming,
compute last_line = offset.saturating_add(lines.len()).saturating_sub(1), then
compute line_number_width and build the final output String with correct padding
(update uses of line_number_width, estimated_capacity, and output), or (B) add
an optional parameter (e.g., expected_last_line) callers can pass when they know
file size and use it instead of limit; choose one approach and remove the
current last_line = offset + limit - 1 usage.
In `@src/llm-coding-tools-serdesai/src/tools/grep.rs`:
- Line 350: The comment on the line in grep.rs describing the line format is
stale—update the comment that currently reads "// Line format is " 1: content",
so actual content is line.len() - prefix" to reflect the new format where the
prefix is a possibly space-padded number followed by ": " (e.g., "1: content");
reference the same prefix calculation used by the test logic (look for the
prefix_ variable/logic in the surrounding code) and reword the comment to say
something like "Line format is '<number>: content' (number may be space-padded),
so the content starts after the '<number>: ' prefix."
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72292f06-f577-4553-a22d-5385214bab0f
📒 Files selected for processing (10)
src/BRANDING.mdsrc/llm-coding-tools-core/benches/tools_read.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tool_metadata/read.rssrc/llm-coding-tools-core/src/tools/grep.rssrc/llm-coding-tools-core/src/tools/read.rssrc/llm-coding-tools-core/src/util.rssrc/llm-coding-tools-serdesai/src/tools/grep.rssrc/llm-coding-tools-serdesai/src/tools/read.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Semver Checks (Serdesai Full)
- GitHub Check: Semver Checks (Serdesai Full+Linux)
- GitHub Check: Semver Checks (Core Blocking)
- GitHub Check: Semver Checks (Core Blocking+Linux)
- GitHub Check: Semver Checks (Core Async)
- GitHub Check: Semver Checks (Core Async+Linux)
- GitHub Check: Blocking macOS
- GitHub Check: Blocking Windows
- GitHub Check: Blocking Linux
- GitHub Check: Async Linux
- GitHub Check: Async Windows
- GitHub Check: Async macOS
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-04-11T21:51:51.714Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 101
File: src/llm-coding-tools-agents/src/path/resolver.rs:149-151
Timestamp: 2026-04-11T21:51:51.714Z
Learning: In `src/llm-coding-tools-agents/src/path/resolver.rs` (Rust, llm-coding-tools-agents crate), the `/**` optimization in `try_globstar_optimisation` is intentionally Unix-only behavior. On Windows, `/**` is not a valid absolute path pattern, so the literal string match will never trigger there and the resolver safely falls through to `AllowedGlobResolver`. This cross-OS degradation is by design: Unix-authored configs using `/**` do not accidentally grant unrestricted access on Windows. Do not flag this as a missing `#[cfg(unix)]` guard.
Applied to files:
src/llm-coding-tools-serdesai/src/tools/grep.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/tools/grep.rs
📚 Learning: 2026-04-08T21:35:35.581Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-04-08T21:35:35.581Z
Learning: If relevant to your review task, read `.cargo/{general,performance,documentation}.md` for guidance
Applied to files:
src/BRANDING.md
📚 Learning: 2026-04-11T21:51:39.415Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 101
File: src/llm-coding-tools-agents/src/path/resolver.rs:149-151
Timestamp: 2026-04-11T21:51:39.415Z
Learning: In `src/llm-coding-tools-agents/src/path/resolver.rs`, the `/**` pattern optimization (which selects `AbsolutePathResolver` for unrestricted access) is intentionally Unix-only. On Windows, `/**` is not a valid absolute path and therefore the optimization simply does not trigger, falling through to `AllowedGlobResolver`. This cross-platform behavior is by design, allowing a single permission config to be shared across OSes without platform-specific guards.
Applied to files:
src/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/tools/grep.rs
🔇 Additional comments (12)
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs (1)
21-21: LGTM!The 1-byte reduction matches the dropped
Lcharacter in the common-rules text, and the constant is exhaustively validated bycommon_rules_max_size_matches_all_rendered_variantsover all2^8tool-flag combinations.src/llm-coding-tools-core/src/util.rs (2)
84-106: LGTM — clean zero-allocation right-pad implementation.Buffer sizing is correct:
usize::MAXis exactly 20 decimal digits, so[b' '; 20]is the tight upper bound. Theunsafeis justified — only ASCII spaces and digits are written. Then == 0special case is necessary since the loop body would otherwise emit nothing for zero.One small note: contract violations (
width > 20orwidth < digit_count(n)) are caught only bydebug_assert!. That's fine for apub(crate)helper, but it does mean a future caller passing user-influenced width in release would either panic on the slice index (width > 20) or silently produce a malformed slice (width < digits). Worth keeping in mind if this ever becomes more public.
131-147: LGTM — good case coverage.The seven cases cover the core invariants: no padding, full padding, exact-fit, the zero edge case both with and without padding, and large numbers. Sufficient for a small utility.
src/llm-coding-tools-core/benches/tools_read.rs (1)
8-8: LGTM — doc accurately reflects new prefix format.src/llm-coding-tools-core/src/system_prompt.rs (1)
1164-1164: LGTM — assertion matches the updatedLINE_PREFIX_DISPLAYvalue.src/llm-coding-tools-serdesai/src/tools/read.rs (1)
201-300: LGTM — assertions consistently track the new prefix format.All
containschecks are still correct under the new padded format because" 1: hello"contains the substring"1: hello". Negative assertions (!text.contains("4:"), etc.) are also safe given the test inputs (line1..line4) don't include other<digit>:substrings.src/llm-coding-tools-serdesai/src/tools/grep.rs (1)
206-206: LGTM — assertions match the new padded format.For a single match
max_line_num=1giveswidth=1, so the unpadded literal"1: …"is the correct expectation.Also applies to: 229-229, 434-436
src/llm-coding-tools-core/src/tool_metadata/read.rs (1)
18-18: LGTM — consistent with the renderer intool_sections::write_read_sectionand the rendered tool output.src/llm-coding-tools-core/src/tools/read.rs (2)
663-679: Nice coverage of the multi-digit padding case.Asserts both the padded (
" 1:") and unpadded ("12:") shapes plus absence of the oldLprefix — exactly the regression cases that matter for this change.
287-329: LGTM —line_number_widththreaded cleanly throughemit_line/process_line.Both call sites pass the same width, and
process_line<const LINE_NUMBERS: bool>still monomorphizes theLINE_NUMBERS=falsebranch so the unused width parameter is optimized out.src/llm-coding-tools-core/src/tools/grep.rs (2)
707-751: LGTM — alignment test correctly exercises multi-digit padding.The expected
" 3: short"(2 leading spaces + 1 padding space +3) and" 15: longer content"(2 leading spaces +15) match the formatter's two-space indent on line 240 followed by a width-2 right-aligned number, and the negative!contains("L")assertion guards against regressing to the oldL<n>:prefix.
222-228: The suggestion to pass1as width is incorrect and would cause runtime assertion failures.The
push_padded_usizefunction enforces a strict contract:width >= digit_count(n)(see line 102's debug assertion:"width ({width}) < digit count of {n}"). If width is less than the actual digit count, the assertion fails.For example, calling
push_padded_usize(&mut output, 123, 1)would panic because 123 has 3 digits but width is 1. The width calculation (value.checked_ilog10().unwrap_or(0) as usize + 1) is necessary to ensure the width parameter meets the function's contract. The current code is correct and the three instances have no redundancy that can be safely simplified.> Likely an incorrect or invalid review comment.
| @@ -0,0 +1,163 @@ | |||
| # BRANDING.md | |||
|
|
|||
| ## Project Read | |||
There was a problem hiding this comment.
Heading wording looks incorrect (Project Read).
Consider renaming to Project Brief or Project Overview for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BRANDING.md` at line 3, The heading "Project Read" is misworded; change
the Markdown heading text from "Project Read" to a clearer option such as
"Project Brief" or "Project Overview" (update the line containing the heading
"Project Read" in BRANDING.md to the chosen wording).
…ad and grep output
- Add `push_padded_usize` to util.rs: zero-allocation, stack-buffered,
right-aligned integer formatting with leading spaces
- Remove `push_usize` — all callers now use `push_padded_usize`
- Compute `line_number_width` from max line number in read and grep;
right-align all line numbers to that width
- Remove dead `LINE_PREFIX_FORMAT` constant; update `LINE_PREFIX_DISPLAY`
from `"L{n}: "` to `"{n}: "`
- Update `COMMON_RULES_SECTION_MAX_SIZE` (494 → 493) for removed L char
- Update all test assertions and benchmark comments
- Add integration tests for multi-digit line number alignment (width ≥ 2)
1 participant
Problem
Read and grep tool output used
L<n>:line-number prefixes (e.g.L1: hello,L42: world). This wastes one token per line and, more critically, the variable-width prefix (L1:vsL42:) caused LLMs to miscalculate line widths during edits - a documented pain point (Claude Code issue #36654).Solution
Replace
L<n>:with space-padded<n>, right-aligned to the width of the largest line number in the output. Space-padding avoids the octal ambiguity risk of zero-padding.Examples:
1: hello/2: world1: hello/12: world3: short/15: longer contentChanges
Core:
push_padded_usizetoutil.rs- zero-allocation, stack-buffered[u8; 20]initialized with spaces, digits written right-to-left,unsafe from_utf8_uncheckedfor the final slice. 7 unit tests covering edge cases (zero, single/multi-digit, width > digits, width == digits).push_usize- all callers now usepush_padded_usize(grep match_count, effective_limit, errors.len() pass their own digit count as width for no-padded output).LINE_PREFIX_FORMATconstant; updateLINE_PREFIX_DISPLAYfrom"L{n}: "to"{n}: ".read.rs: computeline_number_widthfromchecked_ilog10(last_line) + 1, thread it throughemit_line/process_line, replaceoutput.push('L'); push_usize(...)withpush_padded_usize(output, line_number, line_number_width).grep.rs: computeline_number_widthviaflat_mapover match line numbers informat(), replace" L"prefix with" "+push_padded_usize.COMMON_RULES_SECTION_MAX_SIZE(494 -> 493) for the removedLcharacter.Tests:
L<n>:assertions updated to space-padded<n>:across 5 files (core read/grep, serdesai read/grep, system_prompt).grep_format_aligns_line_numbers_with_padding- verifies width=2 alignment (lines 3 and 15).reads_file_with_multi_digit_line_numbers- verifies 12-line file produces1:and12:.Benchmarking:
max_line_numfield (pre-computed in search) vsflat_mapinformat(). No measurable difference (~50 ns iterator chain buried in 2+ µs formatting, itself in 270+ µs I/O). Kept the simplerflat_mapapproach - no struct surface cost, no mutation of the search path.Files changed (10)
util.rspush_padded_usize, removepush_usizetool_metadata/read.rsLINE_PREFIX_FORMAT, updateLINE_PREFIX_DISPLAYtools/read.rsline_number_widthcomputation, threaded through emit/processtools/grep.rsline_number_widthvia flat_map,push_padded_usizefor all integerssystem_prompt.rscontext/tool_prompt/mod.rsCOMMON_RULES_SECTION_MAX_SIZE494->493benches/tools_read.rsserdesai/tools/grep.rsserdesai/tools/read.rs