Cut read_file junk-context token waste with ceilings and path heuristics - #888
Cut read_file junk-context token waste with ceilings and path heuristics#888definitelynotguru wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe file-reading tools now repair paths, apply bounded line and byte windows, clamp oversized lines, reject special files, stream large minified files, and report file and truncation metadata. Tests cover regular, minified, empty, low-value, repaired, and unsafe paths. ChangesFile reading behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant read_file
participant read_path
participant Filesystem
Caller->>read_file: request file range
read_file->>read_path: resolve and classify path
read_path->>Filesystem: validate workspace path and file type
Filesystem-->>read_path: readable regular file
read_file->>Filesystem: scan and read bounded range
Filesystem-->>read_file: content, notices, and metadata
read_file-->>Caller: content and truncation metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tools/read_file.go (1)
569-611: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftTwo new line readers disagree about the
readRawLinecontract atio.EOF. One helper throws away data returned withio.EOF; the other keeps it and also claims a line break exists. At most one can matchreadRawLine, so a file whose last line has no trailing newline renders wrong in one of the two tools. Pin the contract in one place and make both callers use it.
internal/tools/read_file.go#L569-L611: stop settingraw = nilonio.EOF; emit any returned data, then break, and cover a file with no trailing newline in a regression test.internal/tools/read_minified_file.go#L229-L273: stop passingended || err == io.EOFtotrimLineBreak; pass the realendedflag so no byte is removed from a final line that has no break.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 569 - 611, Align both line readers with the readRawLine contract: in internal/tools/read_file.go lines 569-611, preserve raw data returned alongside io.EOF, emit it, then stop reading, and add a regression test for a final line without a newline; in internal/tools/read_minified_file.go lines 229-273, pass only the actual ended flag to trimLineBreak instead of ended || err == io.EOF, so the final unbroken line remains intact.
🧹 Nitpick comments (9)
internal/tools/read_file.go (4)
438-440: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
bytes.IndexByteinstead of converting each line to a string.
scanReadFileStatscallsbytesContainNULfor every line of the file. Thestring(raw)conversion is an unnecessary allocation on that hot path.♻️ Byte-level NUL check
func bytesContainNUL(raw []byte) bool { - return strings.IndexByte(string(raw), 0) >= 0 + return bytes.IndexByte(raw, 0) >= 0 }This needs
"bytes"in the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 438 - 440, Update bytesContainNUL to use bytes.IndexByte directly on the raw byte slice instead of converting it to a string, and add the bytes import required for the byte-level check.
613-630: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe trailing
return s, falseis unreachable.The rune count check above guarantees the loop finds index
max. Return an explicit result or a panic-free fallback comment so a later reader does not treat this as a real path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 613 - 630, The trailing return in clampRunes is unreachable after the RuneCountInString check; replace it with an explicit result or a panic-free fallback that documents the invariant, while preserving truncation behavior and avoiding a misleading normal return path.
349-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
truncation_reasoncascade is easier to read as one decision.Lines 351-361 set a reason, then lines 363-366 overwrite it for the
limit + clampcase. Collapse both into a singleswitchso no branch writes a value another branch replaces.♻️ Single decision for the reason
- switch { - case budgeted.Truncated: - meta["truncation_reason"] = "byte_budget" - case emit.clampedLines > 0 && !truncated: - // Line-clamp-only: exact text of those lines was not returned, so the - // ledger must not credit a full-line view. - meta["truncation_reason"] = "line_clamp" - meta["clamped_lines"] = strconv.Itoa(emit.clampedLines) - case truncated: - meta["truncation_reason"] = "limit" - } - // Prefer reporting both when a default/limit page also hit a mega-line. - if emit.clampedLines > 0 && truncated && !budgeted.Truncated { - meta["truncation_reason"] = "limit+line_clamp" - meta["clamped_lines"] = strconv.Itoa(emit.clampedLines) - } + switch { + case budgeted.Truncated: + meta["truncation_reason"] = "byte_budget" + case emit.clampedLines > 0 && truncated: + meta["truncation_reason"] = "limit+line_clamp" + meta["clamped_lines"] = strconv.Itoa(emit.clampedLines) + case emit.clampedLines > 0: + // Line-clamp-only: exact text of those lines was not returned, so the + // ledger must not credit a full-line view. + meta["truncation_reason"] = "line_clamp" + meta["clamped_lines"] = strconv.Itoa(emit.clampedLines) + case truncated: + meta["truncation_reason"] = "limit" + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 349 - 374, In the truncation metadata logic before the Result construction, replace the sequential truncation_reason assignments with one ordered switch that directly selects byte_budget, limit+line_clamp, line_clamp, or limit. Preserve clamped_lines metadata for any clamped-line case and keep the existing truncated condition and Result fields unchanged.
217-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe header line assumption in the low-value notice is fragile.
The notice is inserted after the first
\nofresult.Output.renderReadFileRangemay emit arangeNoteas the second line, so the notice lands between the header and the range note. The dedup check also matches on the substring"looks like a lock/build", which couples this block to the exact wording a few lines above. Build the notice intorenderReadFileRangeoutput, or pass a flag, instead of string-editing the rendered result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 217 - 241, The low-value notice insertion in the read flow should not modify rendered output by locating the first newline or depend on its wording for deduplication. Update renderReadFileRange or its caller to receive the low-value-path context and emit the notice in the correct output order alongside rangeNote, while preserving the existing metadata fields and only applying the notice for non-explicit, successful reads.internal/tools/read_path.go (3)
257-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
stripInvisiblePrefixhelper.Nothing calls it, and the comment says it is reserved for future use. Dead code added in this PR widens the diff beyond the approved scope. Delete it and add it when a caller exists.
As per coding guidelines: "Keep changes focused on the approved scope; do not include unrelated fixes, refactors, formatting churn, generated output, or unrelated working-tree changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_path.go` around lines 257 - 263, Remove the unused stripInvisiblePrefix function and its associated comment from the diff, along with any imports used only by that helper; leave surrounding path-processing code unchanged.Source: Coding guidelines
147-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFix byte slicing on the first rune of the base name.
base[:1]andbase[1:]slice bytes, not runes. For a base name that starts with a multibyte rune, this splits the UTF-8 sequence and produces an invalid candidate string. Use a rune-aware split.♻️ Rune-aware capitalization candidate
if len(base) > 0 { lower := strings.ToLower(base) if dir == "." || dir == "" { add(lower) - add(strings.ToUpper(base[:1]) + base[1:]) + first, size := utf8.DecodeRuneInString(base) + if size > 0 && first != utf8.RuneError { + add(strings.ToUpper(string(first)) + base[size:]) + } } else { add(filepath.Join(dir, lower)) } }This needs
"unicode/utf8"in the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_path.go` around lines 147 - 156, Update the capitalization candidate construction in the read-path logic to split base at the first UTF-8 rune rather than using byte slices. Use unicode/utf8 to determine the first rune width, then preserve the existing uppercase-first-rune and remaining-suffix behavior without producing invalid strings.
223-232: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe containment branch adds no reach beyond the explicit distance bound.
bestDiststarts atreadPathLevenshteinMax + 1, sod < bestDistalready restricts the containment branch tod <= 2. A basename that contains the wanted name but differs by more than 2 characters is rejected, which is likely not the intent of theAGENT.mdvsAGENTS.mdcomment. Either accept containment matches independent of the distance cap, or drop the branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_path.go` around lines 223 - 232, Update the containment handling in the read-path matching logic so names containing or contained by wantFold are accepted regardless of the readPathLevenshteinMax distance limit, or remove the containment branch if that behavior is not required. Do not let bestDist’s initial bound reject valid containment matches.internal/tools/file_tools_test.go (2)
255-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe nested assertion cannot fail in a useful way.
The outer condition already requires that
offset=81is absent. The inner condition repeats the same check, so the block reduces to one assertion with extra branching, and an output that contains"80 lines"but resumes at the wrong offset still passes. Assertoffset=81directly.♻️ Direct assertion
- if !strings.Contains(result.Output, "offset=81") && !strings.Contains(result.Output, "80 lines") { - // default low-value window is 80 lines → resume at 81 - if !strings.Contains(result.Output, "offset=81") { - t.Fatalf("expected 80-line window resume, got tail %q", result.Output[max(0, len(result.Output)-200):]) - } - } + // default low-value window is 80 lines → resume at 81 + if !strings.Contains(result.Output, "offset=81") { + t.Fatalf("expected 80-line window resume, got tail %q", result.Output[max(0, len(result.Output)-200):]) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/file_tools_test.go` around lines 255 - 260, In the test assertion around result.Output, remove the redundant outer and inner branching and directly require that result.Output contains "offset=81". Keep the existing failure message and diagnostic tail, ensuring output that only mentions "80 lines" does not pass.
266-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test passes whether or not path repair works.
Both branches accept a wide set of outputs, and
strings.Contains(result.Output, "agents")matches the file body, not the repaired path. On a case-insensitive filesystem the requested name resolves differently than on Linux, so the test proves little on either platform. Assert the concrete contract: a successful read whose output namesAGENTS.mdand carries the repair note.As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths; path-sensitive logic must include a non-Linux case or a hermetic equivalent exercising the same normalization."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/file_tools_test.go` around lines 266 - 282, Strengthen TestReadFileToolDidYouMean to require the concrete auto-repair contract: assert StatusOK, verify the output identifies AGENTS.md as the repaired path, and verify it includes the repair note (“did you mean” or the established equivalent). Remove the permissive error branch and avoid matching the file body alone; make the fixture or requested path hermetic so the test exercises normalization consistently across case-sensitive and case-insensitive filesystems.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/file_tools_test.go`:
- Around line 299-308: Update blockedSpecialPath in read_file.go to canonicalize
path separators into a platform-independent form before comparing against the
blocked /dev/zero, /dev/urandom, and /proc/self/fd paths. Preserve
basename-based behavior so workspace files named null remain allowed, and keep
TestBlockedSpecialPath as the cross-platform regression test.
- Around line 5-10: Move the FIFO regression test that calls syscall.Mkfifo into
a separate Unix-only test file with a //go:build unix constraint, and remove the
syscall import from internal/tools/file_tools_test.go. Keep all cross-platform
tests in the existing file so the tools test package compiles on Windows.
In `@internal/tools/read_file.go`:
- Around line 484-500: Update blockedSpecialPath so device and proc special-path
checks only match canonical absolute paths beginning with /dev/ or /proc/,
rather than any path containing those directory names. Preserve the existing
special basenames and proc fd detection, and leave Windows paths unaffected by
these slash-only prefix checks.
In `@internal/tools/read_minified_file_test.go`:
- Around line 187-208: The test TestReadMinifiedFileDefaultLineWindow must
assert that the default read window is actually bounded at 2,000 lines. Parse
res.Meta["raw_lines"] with strconv and fail when the value exceeds the expected
2,000-line window, while retaining validation for missing or invalid metadata;
add the strconv import and remove the ineffective conditional that allows values
such as 2500.
In `@internal/tools/read_minified_file.go`:
- Around line 166-177: Update the result construction in the read_minified_file
flow so default-window truncation and line-clamp truncation set Result.Truncated
and the corresponding truncation_reason metadata, not only header notes. Reuse
the existing truncation metadata conventions used by read_file, while preserving
the current byte-budget truncation behavior and notes.
- Around line 136-145: Update the minification branch around partialLoad so
streamed windows do not use minify.File while still advertising a minified Go
view. Either provide sufficient leading context for minify.ContextualFragment,
or route streamed windows through the non-applied path and ensure its
result/header explicitly states that context-sensitive stripping is disabled;
preserve contextual fragment checks for complete or adequately contextualized
content.
- Around line 110-135: Update the partialLoad branch in read_minified_file to
detect offsets beyond the file’s actual source-line count before calling
selectSourceLines, returning the same past-end message with the true total line
count. Also record an appropriate baseline for the streamed window/range through
options.FileTracker.Record so subsequent edit_file, write_file overwrite, and
apply_patch operations do not report an unseen-file conflict.
In `@internal/tools/read_path.go`:
- Around line 66-99: Update resolveReadPathWithRepair and the subsequent
read/open flow so repaired paths are containment-validated atomically at the
actual file access, rather than relying on os.Stat(abs) before os.Open or
os.ReadFile. Use a rooted/NOFOLLOW access mechanism or open the file through the
validated scope and recheck the opened handle, preserving the existing
repaired-path and suggestion behavior while preventing symlink swaps from
escaping the workspace scope.
---
Outside diff comments:
In `@internal/tools/read_file.go`:
- Around line 569-611: Align both line readers with the readRawLine contract: in
internal/tools/read_file.go lines 569-611, preserve raw data returned alongside
io.EOF, emit it, then stop reading, and add a regression test for a final line
without a newline; in internal/tools/read_minified_file.go lines 229-273, pass
only the actual ended flag to trimLineBreak instead of ended || err == io.EOF,
so the final unbroken line remains intact.
---
Nitpick comments:
In `@internal/tools/file_tools_test.go`:
- Around line 255-260: In the test assertion around result.Output, remove the
redundant outer and inner branching and directly require that result.Output
contains "offset=81". Keep the existing failure message and diagnostic tail,
ensuring output that only mentions "80 lines" does not pass.
- Around line 266-282: Strengthen TestReadFileToolDidYouMean to require the
concrete auto-repair contract: assert StatusOK, verify the output identifies
AGENTS.md as the repaired path, and verify it includes the repair note (“did you
mean” or the established equivalent). Remove the permissive error branch and
avoid matching the file body alone; make the fixture or requested path hermetic
so the test exercises normalization consistently across case-sensitive and
case-insensitive filesystems.
In `@internal/tools/read_file.go`:
- Around line 438-440: Update bytesContainNUL to use bytes.IndexByte directly on
the raw byte slice instead of converting it to a string, and add the bytes
import required for the byte-level check.
- Around line 613-630: The trailing return in clampRunes is unreachable after
the RuneCountInString check; replace it with an explicit result or a panic-free
fallback that documents the invariant, while preserving truncation behavior and
avoiding a misleading normal return path.
- Around line 349-374: In the truncation metadata logic before the Result
construction, replace the sequential truncation_reason assignments with one
ordered switch that directly selects byte_budget, limit+line_clamp, line_clamp,
or limit. Preserve clamped_lines metadata for any clamped-line case and keep the
existing truncated condition and Result fields unchanged.
- Around line 217-241: The low-value notice insertion in the read flow should
not modify rendered output by locating the first newline or depend on its
wording for deduplication. Update renderReadFileRange or its caller to receive
the low-value-path context and emit the notice in the correct output order
alongside rangeNote, while preserving the existing metadata fields and only
applying the notice for non-explicit, successful reads.
In `@internal/tools/read_path.go`:
- Around line 257-263: Remove the unused stripInvisiblePrefix function and its
associated comment from the diff, along with any imports used only by that
helper; leave surrounding path-processing code unchanged.
- Around line 147-156: Update the capitalization candidate construction in the
read-path logic to split base at the first UTF-8 rune rather than using byte
slices. Use unicode/utf8 to determine the first rune width, then preserve the
existing uppercase-first-rune and remaining-suffix behavior without producing
invalid strings.
- Around line 223-232: Update the containment handling in the read-path matching
logic so names containing or contained by wantFold are accepted regardless of
the readPathLevenshteinMax distance limit, or remove the containment branch if
that behavior is not required. Do not let bestDist’s initial bound reject valid
containment matches.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7da379d7-987e-402e-b6a3-e638987e9ca4
📒 Files selected for processing (5)
internal/tools/file_tools_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.gointernal/tools/read_path.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/file_tools_unix_test.go`:
- Around line 19-24: Update the FIFO rejection test around
NewScopedReadFileTool(...).Run to execute the call in a goroutine and await its
result through a channel, adding a short local timeout that fails the test if
Run blocks. Preserve the existing StatusOK and rejection-message assertions
after the call completes.
In `@internal/tools/read_file.go`:
- Around line 398-400: Bind file validation to the opened handle used by
scanReadFileStats, appendReadFileRange, and renderReadFileBytes instead of
relying on the earlier path-based regular-file check. Open through the existing
or newly added rooted, traversal-resistant helper, verify the returned handle’s
Mode().IsRegular(), and reuse that handle or route every reopen through the
helper; add a regression test covering replacement between validation and open.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3153a4e0-f682-4c9b-81ca-42a5daeb5004
📒 Files selected for processing (6)
internal/tools/file_tools_test.gointernal/tools/file_tools_unix_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.gointernal/tools/read_path.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/tools/read_path.go
- internal/tools/file_tools_test.go
- internal/tools/read_minified_file_test.go
- internal/tools/read_minified_file.go
CodeRabbit follow-upAddressed in latest push
Deferred (not in this PR)
|
|
@coderabbitai please do a full review of this again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/tools/read_file.go (1)
507-527: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
fileModeKindnever returns "character device", and the symlink branch is unreachable.
mode.Type()returns the full type bit set. A character device hasos.ModeDevice|os.ModeCharDevice, so neither theos.ModeDevicecase nor theos.ModeCharDevicecase matches. The function falls through tomode.String()and the refusal message shows a raw mode string such asDcrw-rw-rw-. Theos.ModeSymlinkcase is also unreachable, because the caller usesos.Stat, which follows symlinks.Test bit membership instead of comparing the whole type value.
♻️ Proposed refactor
func fileModeKind(mode os.FileMode) string { - switch mode.Type() { - case os.ModeDir: + switch { + case mode.IsDir(): return "directory" - case os.ModeNamedPipe: + case mode&os.ModeNamedPipe != 0: return "fifo" - case os.ModeSocket: + case mode&os.ModeSocket != 0: return "socket" - case os.ModeDevice: + case mode&os.ModeCharDevice != 0: + return "character device" + case mode&os.ModeDevice != 0: return "device" - case os.ModeCharDevice: - return "character device" - case os.ModeSymlink: + case mode&os.ModeSymlink != 0: return "symlink" + case mode.IsRegular(): + return "file" default: - if mode.IsRegular() { - return "file" - } return mode.String() } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 507 - 527, Update fileModeKind to test mode bit membership rather than exact mode.Type() equality, ensuring combined flags such as os.ModeDevice|os.ModeCharDevice return "character device" and other device modes retain their intended labels. Remove or otherwise avoid relying on the unreachable os.ModeSymlink case, since callers use os.Stat and follow symlinks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/read_minified_file.go`:
- Around line 110-139: Ensure read_minified_file records a FileTracker baseline
on every successful path: in internal/tools/read_minified_file.go lines 110-139,
record the streamed file’s whole-file baseline rather than only the loaded
window (or propagate the whole-file hash through readFileLineWindow); in lines
87-101, call options.FileTracker.Record(absolutePath, nil, info) before
returning for an empty file. Keep the baseline consistent with read_file so
subsequent edit_file, write_file, and apply_patch operations detect the file
correctly.
- Around line 156-168: Update the minification switch around partialLoad,
ranged, and lineClamps so any clamped content uses conservative whitespace-only
normalization, including the default read path. Ensure the full-language
minify.File(relativePath, selected.content) path is used only when content is
neither partial nor clamped.
- Around line 305-367: Update readRawLine to enforce the 2000-rune content cap
while consuming ReadSlice fragments, so oversized single lines are truncated
without allocating the complete line; preserve newline/EOF detection. Also
change readFileLineWindow to avoid scanning the entire file after emitting limit
lines by using bounded look-ahead, or explicitly expose/document that full-file
counting cost if it must remain.
In `@internal/tools/read_path.go`:
- Around line 226-242: Update the candidate-matching loop to accept substring
matches only when the shorter name has at least readPathMinContainmentLen runes
and is a meaningful portion of the longer name; add that tuning constant
alongside the existing read-path limits. Skip directory entries before
evaluating containment or Levenshtein matches so resolveReadPathWithRepair only
suggests readable non-directory paths.
---
Nitpick comments:
In `@internal/tools/read_file.go`:
- Around line 507-527: Update fileModeKind to test mode bit membership rather
than exact mode.Type() equality, ensuring combined flags such as
os.ModeDevice|os.ModeCharDevice return "character device" and other device modes
retain their intended labels. Remove or otherwise avoid relying on the
unreachable os.ModeSymlink case, since callers use os.Stat and follow symlinks.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 150587ed-8361-4d8a-b4a9-d20570cb2295
📒 Files selected for processing (6)
internal/tools/file_tools_test.gointernal/tools/file_tools_unix_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.gointernal/tools/read_path.go
|
please address coderabbit comments and failing tests |
|
Will do! |
Follow-up for @kevincodex1 / CodeRabbitAddressed the full re-review and the CI failures on this branch: CI
CodeRabbit
|
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)
internal/tools/read_file.go (1)
393-443: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
scanReadFileStatsFromstill allocates a whole mega-line.The rest of the read path now caps retained bytes per line at
readFileMaxLineRunes * 4. This scan uses the unboundedreadRawLine, so one line of N bytes allocates N bytes. A 500 MB single-line bundle therefore allocates about 500 MB before the tool decides it is binary or clamps output. The hash needs every byte, but it does not need every byte of a line at once.Stream fixed-size chunks instead: feed the hasher, count
'\n', sniff the first 512 bytes, and detect NUL per chunk. Track whether the last byte was'\n'to preserve the "final unterminated line counts" behavior.♻️ Sketch of a chunked scan
func scanReadFileStatsFrom(file *os.File) (readFileStats, error) { hasher := sha256.New() - reader := bufio.NewReader(file) lines := 0 byteCount := 0 var sniff []byte binary := false + buf := make([]byte, 64*1024) + lastByte := byte(0) for { - // Hash must cover the true on-disk bytes, so scanning is unbounded. - raw, _, err := readRawLine(reader) + n, err := file.Read(buf) + if n > 0 { + chunk := buf[:n] + if _, werr := hasher.Write(chunk); werr != nil { + return readFileStats{}, werr + } + if len(sniff) < 512 { + sniff = append(sniff, chunk[:min(512-len(sniff), n)]...) + } + if !binary && bytesContainNUL(chunk) { + binary = true + } + lines += bytes.Count(chunk, []byte{'\n'}) + byteCount += n + lastByte = chunk[n-1] + } + if err == io.EOF { + break + } if err != nil { return readFileStats{}, err } - ... } + if byteCount > 0 && lastByte != '\n' { + lines++ + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 393 - 443, Replace the unbounded readRawLine loop in scanReadFileStatsFrom with fixed-size chunk reads that stream every chunk into the hasher, increment the line count for each '\n', collect only the first 512 bytes for sniffing, and detect NUL bytes per chunk. Track whether the final processed byte is '\n' so a non-empty unterminated final line still counts, while preserving empty-file handling, MIME detection, binary classification, byte totals, and hash results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/read_file.go`:
- Around line 487-519: Update the comment above openReadableRegularFile to
describe only the guarantee provided by the current Stat-then-Open sequence: it
rejects FIFOs detected before opening, while a concurrent replacement can still
cause os.Open to block before the handle re-check. Do not claim that Stat
prevents the process from hanging unless the implementation is also changed to
use non-blocking Unix open semantics.
---
Outside diff comments:
In `@internal/tools/read_file.go`:
- Around line 393-443: Replace the unbounded readRawLine loop in
scanReadFileStatsFrom with fixed-size chunk reads that stream every chunk into
the hasher, increment the line count for each '\n', collect only the first 512
bytes for sniffing, and detect NUL bytes per chunk. Track whether the final
processed byte is '\n' so a non-empty unterminated final line still counts,
while preserving empty-file handling, MIME detection, binary classification,
byte totals, and hash results.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74ad8d00-fcde-4b9f-9be7-7db89d482394
📒 Files selected for processing (7)
internal/tools/file_tools_test.gointernal/tools/file_tracker_largefile_test.gointernal/tools/line_reader.gointernal/tools/output_boundary_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_path.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/tools/file_tools_test.go
- internal/tools/read_minified_file.go
- internal/tools/read_path.go
|
@coderabbitai please do a full review of this again. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
internal/tools/read_path.go (1)
254-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe final return branch ignores
parentReland re-derives the directory from the request.The three earlier branches build the suggestion from
parentRel, which is the scope-resolved parent. The last branch usesfilepath.Dir(clean)instead, which is the raw requested spelling. When the requested path is absolute, or when scope resolution remaps the parent, the two differ and the suggestion carries the requested spelling rather than the workspace-relative one.The suggestion is re-validated through
resolveScopedReadPathat Line 95, so a mismatch degrades to the "did you mean" hint instead of opening the wrong file. The behavior is safe. The branch is just inconsistent with the ones above it.♻️ Use the resolved parent everywhere
if dir == "." { // parentRel may be "." if parentRel == "." || parentRel == "" { return bestName, nil } return filepath.ToSlash(filepath.Join(parentRel, bestName)), nil } if parentRel == "." || parentRel == "" { return filepath.ToSlash(filepath.Join(dir, bestName)), nil } - // When parent resolved with a relative path, join with best name. - return filepath.ToSlash(filepath.Join(filepath.Dir(clean), bestName)), nil + // Parent resolved inside the scope; join the resolved relative parent. + return filepath.ToSlash(filepath.Join(parentRel, bestName)), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_path.go` around lines 254 - 266, Update the final return branch in the surrounding path-resolution function to build the suggestion from the resolved parentRel, matching the preceding branches, instead of deriving the directory with filepath.Dir(clean). Preserve the existing slash normalization and bestName handling.internal/tools/read_file.go (2)
418-426: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
bytes.Countfor the newline scan.This loop counts newlines one byte at a time.
scanReadFileStatsFromruns on the whole file for everyread_filecall, so this is a hot path.bytes.Countuses an assembly-optimized scan and thebytespackage is already imported forbytesContainNUL.♻️ Vectorized newline count
if !binary && bytesContainNUL(chunk) { binary = true } byteCount += n - for i := 0; i < n; i++ { - if chunk[i] == '\n' { - lines++ - } - } + lines += bytes.Count(chunk, []byte{'\n'}) lastWasNL = chunk[n-1] == '\n'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 418 - 426, Replace the per-byte newline loop in scanReadFileStatsFrom with bytes.Count over the valid chunk slice, preserving the existing lines accumulation and chunk bounds. Reuse the existing bytes import and leave the binary detection logic unchanged.
647-653: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueA byte-clipped line can be emitted without the inline ellipsis marker.
readRawLineLimitedstops retaining atmaxKeepbytes and reportsclipped. For multibyte content, 8000 bytes can hold fewer than 2000 runes.clampRunesthen returnswasClamped == falseand appends no…, but the line body is still missing its tail.The counter at Line 650 is correct and the aggregate note at Line 349 still fires, so the model learns that lines were cut. Only the inline marker on that specific line is missing.
♻️ Mark clipped lines inline as well
body := string(trimLineBreak(raw, ended)) clamped, wasClamped := clampRunes(body, readFileMaxLineRunes) if wasClamped || clipped { stats.clampedLines++ } + if clipped && !wasClamped { + clamped += "…" + } output.WriteString(clamped)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 647 - 653, Update the line-emission logic around clampRunes so byte-clipped input also receives the inline ellipsis marker when clipped is true, even if wasClamped is false. Preserve the existing clampedLines counter behavior and avoid adding a second marker when clampRunes already appended one.internal/tools/line_reader.go (3)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiscard the fragment directly instead of assigning and voiding it.
ReadSlicereturns two values, sofragmentis declared only to be voided on the next line. Use the blank identifier in the assignment.♻️ Drop the placeholder variable
func discardThroughNewline(reader *bufio.Reader) error { for { - fragment, err := reader.ReadSlice('\n') - _ = fragment + _, err := reader.ReadSlice('\n') switch err {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/line_reader.go` around lines 104 - 108, Update discardThroughNewline to assign the first return value of reader.ReadSlice('\n') directly to the blank identifier, removing the unused fragment declaration and its separate discard statement while preserving the existing error handling.
59-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe named return
clippedis never assigned in the function body.Every early return passes an explicit
trueorfalse. The two returns at Line 61 and Line 66 pass the namedclipped, which still holds its zero value. Both sites are reached only through the branch where the fragment fit insideroom, sofalseis the correct value today.The risk is future edits. If a discard path is later added inside the loop before these returns, they will keep returning
falseand silently under-report clipping. Use the literal so the intent is explicit.♻️ Return the literal at the non-clipping exits
switch readErr { case nil: - return kept, true, clipped, nil + return kept, true, false, nil case bufio.ErrBufferFull: continue case io.EOF: if len(kept) > 0 { - return kept, false, clipped, nil + return kept, false, false, nil } return nil, false, false, io.EOF🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/line_reader.go` around lines 59 - 71, In the line-reading function’s switch on readErr, replace the named return value clipped with the explicit false literal at the non-clipping returns for the nil and io.EOF-with-kept-data cases. Leave the other return paths unchanged.
16-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct regression tests for
readRawLineLimited.Existing tests cover tool-level truncation, but no test calls this helper directly. Add cases for the exact-fill boundary, multibyte input clipped by
maxKeep, and an unterminated line longer thanmaxKeep. Assert the returned bytes,ended,clipped, anderrvalues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/line_reader.go` around lines 16 - 73, Add direct table-driven regression tests for readRawLineLimited covering exact maxKeep filling, multibyte input clipped at the byte limit, and an unterminated line exceeding maxKeep. For each case, assert the returned line bytes plus ended, clipped, and err, using bufio.Reader inputs and preserving the helper’s byte-based truncation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/read_minified_file.go`:
- Around line 343-356: Update the caller that writes meta["source_total_lines"]
to avoid exporting it as an exact count when fileLineWindow.hitLimit is true;
either omit the key in that case or use the established metadata convention to
mark the value as a lower bound, while preserving exact totals for non-truncated
windows. Use the fileLineWindow result and its hitLimit field as the change
point.
- Around line 83-90: In the file-reading flow containing
assertReadableRegularFile and os.Stat, replace both calls with
openReadableRegularFile and retain its *os.File and os.FileInfo results, closing
the file after all reading completes. Pass the open handle to both the full-load
path and readFileLineWindow, and refactor readFileLineWindow to accept an
io.Reader while preserving its existing behavior. Avoid reopening absolutePath
or calling os.Stat after validation.
---
Nitpick comments:
In `@internal/tools/line_reader.go`:
- Around line 104-108: Update discardThroughNewline to assign the first return
value of reader.ReadSlice('\n') directly to the blank identifier, removing the
unused fragment declaration and its separate discard statement while preserving
the existing error handling.
- Around line 59-71: In the line-reading function’s switch on readErr, replace
the named return value clipped with the explicit false literal at the
non-clipping returns for the nil and io.EOF-with-kept-data cases. Leave the
other return paths unchanged.
- Around line 16-73: Add direct table-driven regression tests for
readRawLineLimited covering exact maxKeep filling, multibyte input clipped at
the byte limit, and an unterminated line exceeding maxKeep. For each case,
assert the returned line bytes plus ended, clipped, and err, using bufio.Reader
inputs and preserving the helper’s byte-based truncation behavior.
In `@internal/tools/read_file.go`:
- Around line 418-426: Replace the per-byte newline loop in
scanReadFileStatsFrom with bytes.Count over the valid chunk slice, preserving
the existing lines accumulation and chunk bounds. Reuse the existing bytes
import and leave the binary detection logic unchanged.
- Around line 647-653: Update the line-emission logic around clampRunes so
byte-clipped input also receives the inline ellipsis marker when clipped is
true, even if wasClamped is false. Preserve the existing clampedLines counter
behavior and avoid adding a second marker when clampRunes already appended one.
In `@internal/tools/read_path.go`:
- Around line 254-266: Update the final return branch in the surrounding
path-resolution function to build the suggestion from the resolved parentRel,
matching the preceding branches, instead of deriving the directory with
filepath.Dir(clean). Preserve the existing slash normalization and bestName
handling.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 978dc0e0-abb3-438a-8128-452c8b09551b
📒 Files selected for processing (9)
internal/tools/file_tools_test.gointernal/tools/file_tools_unix_test.gointernal/tools/file_tracker_largefile_test.gointernal/tools/line_reader.gointernal/tools/output_boundary_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.gointernal/tools/read_path.go
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@IGNORETHIS/measure_lowvalue.go`:
- Around line 16-18: Replace the shared os.TempDir-based root setup with
os.MkdirTemp to create a unique fixture directory, and defer cleanup of only
that returned path. In the setup and cleanup flow around root, propagate or fail
on errors from MkdirTemp, MkdirAll, and RemoveAll instead of ignoring them.
In `@IGNORETHIS/measure_read.go`:
- Around line 28-30: Propagate filesystem errors across the diagnostic
utilities: in IGNORETHIS/measure_read.go lines 28-30, handle the error from
DirEntry.Info before calling FileInfo.Size; in IGNORETHIS/measure_read.go lines
34-36, handle os.Stat errors before using FileInfo.Size; in
IGNORETHIS/measure_lowvalue.go lines 18, 23-27, and 38, stop execution when
directory creation or either fixture write fails.
- Around line 65-70: Update the preview truncation in the output formatting
block to count characters by converting first to []rune before applying the
70-character limit, then append the ellipsis without splitting UTF-8 code
points; preserve the existing formatting and output behavior.
- Line 16: Update the fixture path setup around root to read ZERO_READ_FIXTURES,
defaulting to filepath.Join(os.TempDir(), "zero-read-fixtures") when unset.
Replace root-based slash concatenation for all child paths with filepath.Join so
the fixture paths remain platform-neutral.
In `@IGNORETHIS/readthisdeepdive.txt`:
- Around line 68-91: Update the section describing the partial-view ledger and
read/write interaction so it is explicitly historical or reflects the shipped
resolution. Do not present the denied-write and endless deduplication loop as
current behavior; align the wording with the repository’s distinctions between
byte-truncated reads, scoped reads that later cover the file, genuine
partial-read errors, and the full-read guard before deduplication.
In `@internal/tools/line_reader_test.go`:
- Around line 11-85: Extend TestReadRawLineLimited with a sequential-read case
where an oversized first line is followed by a short line, asserting the second
call starts at the next line after discard alignment. Add an injected
failing-reader case that invokes readRawLineLimited and verifies the original
non-EOF error is returned unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 469db66e-79d9-4104-939b-5cd4a1fa0725
📒 Files selected for processing (9)
IGNORETHIS/measure_lowvalue.goIGNORETHIS/measure_read.goIGNORETHIS/readthisdeepdive.txtinternal/tools/line_reader.gointernal/tools/line_reader_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_path.gozero-patched
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/tools/read_path.go
- internal/tools/line_reader.go
- internal/tools/read_minified_file.go
- internal/tools/read_file.go
|
removed ignorethis, no clue how it got in.. my bot hallucinated. fixed now |
|
|
|
@coderabbitai please do a full review. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/tools/read_path.go (1)
254-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo of the three return branches are identical.
Line 259 and Line 265 both return
filepath.Join(parentRel, bestName). Only the middle branch differs, and it triggers only whendir != "."andparentRelis empty or".".resolveScopedReadPathreturns a workspace-relative path for the resolved parent, so that combination means the parent is the workspace root, anddiris then a stale request string rather than the resolved location. Building fromdirthere can emit a suggestion outside the resolved parent.Prefer the resolved parent in every branch.
♻️ Proposed simplification
- if dir == "." { - // parentRel may be "." - if parentRel == "." || parentRel == "" { - return bestName, nil - } - return filepath.ToSlash(filepath.Join(parentRel, bestName)), nil - } - if parentRel == "." || parentRel == "" { - return filepath.ToSlash(filepath.Join(dir, bestName)), nil - } - // Build from the resolved parent path so the suggestion stays in-scope. - return filepath.ToSlash(filepath.Join(parentRel, bestName)), nil + // Always build from the resolved parent so the suggestion stays in-scope. + if parentRel == "." || parentRel == "" { + return bestName, nil + } + return filepath.ToSlash(filepath.Join(parentRel, bestName)), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_path.go` around lines 254 - 265, Update the suggestion path construction in the surrounding read-path logic so every return branch uses the resolved parent path (`parentRel`) with `bestName`; remove the special-case join with `dir` when `parentRel` is empty or `"."`, while preserving the existing `dir == "."` handling and slash normalization.internal/tools/read_file.go (1)
249-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the truncation-reason strings into constants.
"byte_budget","line_clamp","limit+line_clamp", and"limit"are now literal strings inread_file.go,read_minified_file.go,output_boundary_test.go,file_tools_test.go,file_tracker_largefile_test.go, andread_minified_file_test.go. The ledger decision at Line 251 depends on an exact match. A typo in one producer silently credits a partial view as exact.Declare the values once next to
readFileMaxLineRunesand reference them from both tools and the tests.♻️ Proposed constants
const ( truncationReasonByteBudget = "byte_budget" truncationReasonLimit = "limit" truncationReasonLineClamp = "line_clamp" truncationReasonLimitAndClamp = "limit+line_clamp" )Also applies to: 360-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/read_file.go` around lines 249 - 251, Define shared truncation-reason constants next to readFileMaxLineRunes for byte_budget, limit, line_clamp, and limit+line_clamp. Replace the corresponding string literals throughout read_file.go, read_minified_file.go, and the named tests, including the skipSeen comparison, so producers, consumers, and assertions use the same symbols.internal/tools/file_tools_test.go (1)
196-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the tail slices in the failure messages.
result.Output[len(result.Output)-180:]at Line 197 and[len(result.Output)-200:]at Line 200 panic when the output is shorter than the offset. A panic replaces the intended assertion message, which makes the failure harder to diagnose. Line 256 in this file already usesmax(0, ...).Apply the same guard here.
♻️ Proposed change
- t.Fatalf("expected resume at offset 2001, got %q", result.Output[len(result.Output)-180:]) + t.Fatalf("expected resume at offset 2001, got %q", result.Output[max(0, len(result.Output)-180):]) } if strings.Contains(result.Output, "line 2001") { - t.Fatalf("default window leaked past 2000 lines: %q", result.Output[len(result.Output)-200:]) + t.Fatalf("default window leaked past 2000 lines: %q", result.Output[max(0, len(result.Output)-200):])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/file_tools_test.go` around lines 196 - 201, Guard the failure-message tail slices in the assertions around result.Output by clamping each start index to zero, matching the existing max(0, ...) pattern used elsewhere in the test file. Update both slices for the offset=2001 and line 2001 checks while preserving their assertion conditions and messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/line_reader.go`:
- Around line 46-51: Update the limited-line handling in the line-reader branch
around the kept fragment so a discarded remainder consisting only of the line’s
newline returns clipped=false while still reporting that the line ended. Add a
TestReadRawLineLimited case for "abcdefghij\n" with maxKeep 10, and preserve
clipped=true when any non-newline content is discarded.
In `@internal/tools/read_minified_file_test.go`:
- Around line 188-229: Add a regression test for the partialLoad path in
TestReadMinifiedFile or a nearby test, writing a file larger than
readMinifiedMaxLoadBytes and requesting an offset beyond its total lines. Assert
successful output includes the “loaded only lines from offset” note, metadata
contains source_total_lines_min rather than source_total_lines, and the past-end
message is returned for the beyond-file offset.
In `@internal/tools/read_minified_file.go`:
- Around line 137-164: Update the ranged determination in the read/minification
flow around selectSourceLines so any truncated or otherwise applied window is
treated as ranged, including the implicit default limit. Use the selection’s
actual windowing state (such as whether the selected content is incomplete)
alongside the existing offset, explicitLimit, and partialLoad conditions,
ensuring truncated fragments use the conservative contextual or whitespace-only
minification path instead of minify.File as a full file.
In `@internal/tools/read_path.go`:
- Around line 84-100: Update both repaired-candidate checks in the path
resolution flow to accept a result only when os.Stat confirms the target is a
regular file, using FileInfo.Mode().IsRegular(). Apply this to the
spelling-repair branch and the suggestReadPath branch before returning the
repaired path, while preserving the existing hint messages and fallback
behavior.
---
Nitpick comments:
In `@internal/tools/file_tools_test.go`:
- Around line 196-201: Guard the failure-message tail slices in the assertions
around result.Output by clamping each start index to zero, matching the existing
max(0, ...) pattern used elsewhere in the test file. Update both slices for the
offset=2001 and line 2001 checks while preserving their assertion conditions and
messages.
In `@internal/tools/read_file.go`:
- Around line 249-251: Define shared truncation-reason constants next to
readFileMaxLineRunes for byte_budget, limit, line_clamp, and limit+line_clamp.
Replace the corresponding string literals throughout read_file.go,
read_minified_file.go, and the named tests, including the skipSeen comparison,
so producers, consumers, and assertions use the same symbols.
In `@internal/tools/read_path.go`:
- Around line 254-265: Update the suggestion path construction in the
surrounding read-path logic so every return branch uses the resolved parent path
(`parentRel`) with `bestName`; remove the special-case join with `dir` when
`parentRel` is empty or `"."`, while preserving the existing `dir == "."`
handling and slash normalization.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c20315f-5aea-4140-8439-56924ac5b0b0
📒 Files selected for processing (10)
internal/tools/file_tools_test.gointernal/tools/file_tools_unix_test.gointernal/tools/file_tracker_largefile_test.gointernal/tools/line_reader.gointernal/tools/line_reader_test.gointernal/tools/output_boundary_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.gointernal/tools/read_path.go
Add default line windows, per-line clamps, low-value path heuristics, device/FIFO refusal, empty/binary notes, and scoped path repair so naive full-file reads of locks and build output stop flooding the context window.
Move FIFO tests to unix build tags, fix device path prefix matching so workspace packages/dev paths stay readable, report Truncated from read_minified windows, force safe whitespace minify on streamed loads, and tighten tests plus small correctness nits.
Run the read in a goroutine and fail after 2s if open blocks on a writerless FIFO when non-regular refusal is broken.
Open regular files once with Stat-before-Open so FIFOs cannot hang, then re-check the handle. Cap per-line retention while scanning so mega-lines do not allocate fully, stop full-file scans after a minified window is filled, tighten path suggestions (min length, skip dirs), and update tests that assumed unbounded full-file dumps or head/tail of one-liners.
Document that Stat-before-Open only rejects pre-existing FIFOs and that a concurrent swap can still block Open. Hash and line-count the file in fixed-size chunks so mega-lines do not allocate full line buffers during scanReadFileStatsFrom.
Share openReadableRegularFile for read_minified, avoid claiming exact source_total_lines when the window hitLimit is a lower bound, stream line windows from the open handle, and polish line-reader / path / scan nits with regression tests for readRawLineLimited.
Fix diagnostic harness error handling and fixture paths, mark the partial-view loop section of the deep-dive notes as historical, extend readRawLineLimited tests for sequential discard alignment and non-EOF errors, and drop the accidental zero-patched binary from the branch.
- Preserve exact lines when only a trailing newline is dropped - Use conservative minification for incomplete source windows - Restrict repaired read paths to regular files - Share read truncation reason values across tools and tests
17242f6 to
da52e27
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/line_reader.go`:
- Around line 16-21: Update readFileLineWindow to preserve the clipped result
from readRawLineLimited on every streamed line and propagate it into the
minified output, merging byte-clipping with existing readFileMaxLineRunes
rune-clamp reporting so lineClamps increments whenever either limit truncates
content. Add a regression test covering a large multibyte line whose retained
bytes contain fewer than readFileMaxLineRunes runes.
- Around line 30-64: The oversized-fragment branches in the line-reading
function must propagate a non-EOF readErr before calling discardThroughNewline.
Check readErr after retaining the fragment and before either
discardThroughNewline call, returning the kept data with the existing error
metadata; preserve normal discarding for nil and io.EOF cases. Add regression
coverage using a reader that returns fragment data together with an injected
non-EOF error.
In `@internal/tools/read_minified_file.go`:
- Around line 325-340: Preserve the clipped result from readRawLineLimited in
the window-reading loop and count each clipped line in the partialLoad branch’s
lineClamps total. Update the relevant return/state handling around the window
reader and clampContentLineRunes so byte-level clipping contributes to the clamp
note and truncation metadata.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a20195a-9d7e-4373-b0a7-536fa7fda2c6
📒 Files selected for processing (9)
internal/tools/file_tools_test.gointernal/tools/file_tracker_largefile_test.gointernal/tools/line_reader.gointernal/tools/line_reader_test.gointernal/tools/output_boundary_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.gointernal/tools/read_path.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/tools/read_path.go
- internal/tools/read_minified_file_test.go
- internal/tools/file_tools_test.go
- internal/tools/output_boundary_test.go
- internal/tools/read_file.go
- internal/tools/line_reader_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/read_minified_file_test.go`:
- Around line 286-288: Update the assertion in the read-minified-file test to
match a phrase unique to the line-clamp branch rather than the generic “1
line(s)” text. Keep the existing failure diagnostic and ensure the test
specifically verifies the clamp note emitted during the line-clamp path.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f299795c-68b7-418e-987e-e5e24f58a7f6
📒 Files selected for processing (6)
internal/tools/file_tools_test.gointernal/tools/line_reader.gointernal/tools/line_reader_test.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/tools/line_reader.go
- internal/tools/read_minified_file.go
- internal/tools/file_tools_test.go
- internal/tools/read_file.go
- internal/tools/line_reader_test.go
|
@kevincodex1 we're good to go now! |
2 participants
Summary
read_file/read_minified_filenow treat full-file reads as a token budget decision, not an unbounded dump. Defaults and path-class heuristics stop lockfiles,.nextchunks, and minified one-liners from eating the context window on naive tool calls, while normal source still pages usefully.Ideas adapted from the harness-engineering write-up on Command Code’s read tool (three ceilings, recovery notes, partial-view safety, hostile-file posture), with feedback and pressure-testing from @totallynotparth on X. Zero keeps its existing ledger +
read_minified_filearchitecture rather than porting that design wholesale.Big wins
limitis omitted (normal source).byte_offset/byte_limit. Clamped / byte-budget views are not credited on the exact-view write ledger.node_modules,.next,dist/build,*.min.js, etc. default to 80 lines (not 2000), with an explicit note. Explicitlimitstill wins.read_minified_filememory bound — no unboundedos.ReadFileon multi-hundred-MB blobs; default window + stream; same line clamp./dev/*/ FIFO //proc/.../fdrefusal before hang-prone I/O.AGENT.md→AGENTS.md).Measured impact (real monorepo stress)
Harness: forced naive
read_file(no offset/limit) on lockfile +.nextchunks + real Kotlin source, plus/dev/zero. Model:deepseek-ai/DeepSeek-V4-Flash-0731.read_fileTool results — stock (before)
web/package-lock.json.next/.../node_modules_0_-y.vy._.jsNotesRepository.kt.nextchunk/dev/zeroTool results — after this PR (low-value heuristic)
web/package-lock.jsonlow_value_path.next/.../node_modules_0_-y.vy._.jsNotesRepository.kt.nextroute.js/dev/zeroOffline fixture check (same clamps): a ~100 KB single-line minified bundle dropped from ~25k est. tokens to ~0.5k under
read_file/read_minified_file.Smart architecture explores on real source stayed productive; costs there are driven by how many tools the model chooses, not by the junk dump path.
Test plan
go test ./internal/tools -run 'TestReadFile|TestReadMinified|TestBlocked'package-lock,.next, Kotlin source) comparing truncated / emitted / est. tokensread_fileon normal source still returns full files under 2000 lineslimiton a lockfile overrides the 80-line defaultAGENT.mdrepairs or suggestsAGENTS.mdSummary by CodeRabbit
New Features
Bug Fixes