Improve source read efficiency and model compatibility - #867
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe change adds bounded Go-fragment minification, canonical ChangesSource reading workflow
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant read_minified_file
participant SourceRangeSelector
participant ContextualFragment
Caller->>read_minified_file: Provide path, offset, and limit
read_minified_file->>SourceRangeSelector: Select source lines
SourceRangeSelector-->>read_minified_file: Return selected content
read_minified_file->>ContextualFragment: Minify selected content
ContextualFragment-->>read_minified_file: Return result and metrics
read_minified_file-->>Caller: Return ranged output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 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/minify/minify.go`:
- Around line 59-61: Update the line-count logic in the minification flow around
strings.Split to remove one trailing empty element when the normalized content
ends with a newline before enforcing the 512-line limit. Preserve rejection
above 512 source lines, and add a regression test covering 512 statements
followed by a final newline that remains eligible for minification.
In `@internal/tools/read_file.go`:
- Around line 34-40: The renderReadFileRange out-of-range error still refers to
the removed start_line field, conflicting with the public schema. Update
renderReadFileRange to name the canonical offset parameter in that user-visible
message while preserving the existing range validation behavior.
In `@internal/tools/read_minified_file.go`:
- Around line 87-88: The ranged minification flow must preserve lexical state
instead of passing context-free fragments to minify.File. Update
selectSourceLines and the surrounding read logic in
internal/tools/read_minified_file.go at lines 87-88 and 131-144 to use only safe
ranges, retain scanner context, or apply whitespace-only normalization for
unsafe ranges; add or update coverage in
internal/tools/read_minified_file_test.go at lines 48-60 for ranges beginning
inside multiline strings, template literals, and block comments.
🪄 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: 794b02e9-666a-47f6-af12-98f2ac92e34f
📒 Files selected for processing (14)
internal/agent/system_prompt.mdinternal/agent/system_prompt_models.gointernal/cli/deferred_wiring_test.gointernal/minify/minify.gointernal/minify/minify_test.gointernal/tools/file_tools_test.gointernal/tools/file_tracker.gointernal/tools/grep.gointernal/tools/read_file.gointernal/tools/read_minified_file.gointernal/tools/read_minified_file_test.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/update_plan.go
💤 Files with no reviewable changes (1)
- internal/tools/grep.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes: the new offset/limit pair cannot be used before an edit.
limit caps the range [offset, EOF] rather than defining the range, so any interior slice comes back flagged truncated. The registry then skips RecordSeenRange and the edit is refused, with a message telling the model to re-read using offset/limit, which is the call that just failed. The start_line/end_line pair this PR deletes from the schema is the only shape that still works:
offset=500 limit=10 truncated=true reason="limit" seen="" edit refused
offset=500 (runs to EOF) truncated=false seen=500-1000 edit ok
start_line=500 end_line=509 truncated=false seen=500-509 edit ok
1000 line file, driven through Registry.RunWithOptions.
Three others, all reproducible:
Ranged Go reads corrupt raw strings. A range that starts inside a raw string literal is parsed as code, so string content is deleted and identifiers are rewritten. SECRET-MARKER-ONE comes back as SECRET - MARKER - ONE, a line inside the string vanishes, and the header reports a 49% saving. Fragment's own doc says non-Go takes the conservative path because a range may begin inside a multiline string. Go needs the same guard.
Dropping byte_offset/byte_limit from the schema and from the unseen-file message leaves any file with a line over 128KB unreadable, and so uneditable. The byte path still works, and file_tracker_largefile_test.go still asserts it is what makes those files writable again.
Mixing one canonical and one legacy argument silently reads the wrong lines: {start_line: 5, limit: 2} returns lines 1 and 2.
Minor: read_minified_file with an offset past EOF returns an empty body with status ok, where read_file says the offset is past the end.
|
@anandh8x the blocking one is the Two ways out and I would take the first: make I have the probe that produced the table in the review, driven through |
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_safety_test.go`:
- Around line 318-323: Update the read_file line-range accounting so an offset
of 500 with limit 10 produces the inclusive range 500-509. In the relevant
read_file range-processing flow, calculate the bounded end as offset + limit - 1
and apply it before rendering, ensuring exact bounded reads receive seen_lines
metadata of 500-509.
In `@internal/tools/read_minified_file.go`:
- Around line 163-164: Update the range-end calculation in the
read-minified-file logic to compare limit against end-start before adding,
preventing integer overflow while preserving the existing limit behavior. Add a
regression test covering a maximum-int limit with a nonzero offset and verify
the resulting slice does not panic.
🪄 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: e278e93d-5cd5-485d-b4fd-949bdb05bbe6
📒 Files selected for processing (7)
internal/minify/minify.gointernal/tools/file_safety_test.gointernal/tools/file_tools_test.gointernal/tools/file_tracker.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 (3)
- internal/tools/file_tracker.go
- internal/tools/file_tools_test.go
- internal/tools/read_file.go
|
@coderabbitai The latest review found no actionable comments, but the previous changes-requested verdict is still active. Please review the latest commit and clear it if no blockers remain. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata and latest commits ==='
gh pr view 867 --repo Gitlawb/zero --json headRefOid,baseRefOid,commits,reviewDecision \
--jq '{headRefOid,baseRefOid,reviewDecision,commits:[.commits[]|{oid,messageHeadline,authoredDate}]}'
echo
echo '=== Changed files in the latest commit ==='
git show --stat --oneline --decorate HEAD
git show --format=fuller --find-renames --find-copies -- \
internal/tools/read_file.go \
internal/tools/file_safety_test.go \
internal/tools/file_tools_test.go \
internal/tools/registry.go \
internal/tools/read_minified_file.go \
internal/tools/read_minified_file_test.go
echo
echo '=== Relevant source outlines ==='
ast-grep outline internal/tools/read_file.go --items all --view expanded
ast-grep outline internal/tools/file_safety_test.go --items all --view expanded
ast-grep outline internal/tools/registry.go --items all --view expanded
echo
echo '=== Range, truncation, and observation call sites ==='
rg -n -C 5 -P '\b(offset|limit|endLine|truncated|Observe|observation|RecordRead|read credit)\b' \
internal/tools/read_file.go \
internal/tools/file_safety_test.go \
internal/tools/file_tools_test.go \
internal/tools/registry.go \
internal/tools/read_minified_file.go \
internal/tools/read_minified_file_test.goLength of output: 54503 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== End-to-end regression test ==='
sed -n '299,333p' internal/tools/file_safety_test.go
echo
echo '=== Registry execution and observation commit ==='
sed -n '129,259p' internal/tools/registry.go
echo
echo '=== Canonical range calculation and observation creation ==='
sed -n '66,177p' internal/tools/read_file.go
sed -n '179,278p' internal/tools/read_file.go
echo
echo '=== Corrective commit for the original blocker ==='
git show --format=fuller --find-renames --find-copies 3aa190bd7f56266325a196b4985c56c413c4d4a5 -- \
internal/tools/read_file.go \
internal/tools/file_safety_test.go \
internal/tools/file_tools_test.go \
internal/tools/registry.goLength of output: 24078
The latest commit also prevents overflow for a maximum I found no remaining blockers. I did not run tests because the review sandbox does not execute repository code. [approve] You are interacting with an AI system. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Checked out and exercised. The diagnosis is the strong part: two overlapping pagination systems in one schema is an ambiguity the model resolves by combining them, and the old behaviour answered that with a hard error and a retry loop until the guard stopped the run. Six invalid calls then a dead run is a real cost, and the benchmark showing it complete afterwards is the right evidence for it.
Compatibility is genuinely preserved, not just claimed. byte_offset/byte_limit are still in the schema and still work; the canonical offset/limit sit alongside rather than replacing them. Conflicting spellings of the same axis (offset + start_line, limit + end_line/max_lines) still error, which is right — those are the same question asked twice, and picking one would be a guess.
The mixed line+byte case resolves to lines (byteMode := (hasByteOffset || hasByteLimit) && !hasLineRange), which is the correct choice: the failure being fixed is exactly a model that sent both, and answering something beats failing. TestReadFileToolMixedLegacyRangesPreferLines pins it.
go test ./internal/tools/ ./internal/minify/ ./internal/agent/ green on the branch.
One inconsistency worth a follow-up, not blocking
This file already establishes the principle, in its own words at read_file.go:208:
The recovery is surfaced in the output (rangeNote) so it is never silent
The end_line < start_line recovery follows it and emits a note. The new byte-args-ignored recovery does not — a caller that asked for a byte range gets a line range with nothing saying its byte arguments were dropped. Same class of recovery, one discloses and one does not.
I am not blocking on it for one reason: the output is line-numbered (2 | beta), so a byte read and a line read are visually distinguishable, and a model can tell what it got. That makes this a clarity gap rather than a correctness one. Still, a one-line note in the same shape as rangeNote would close it and keep the file consistent with the rule it states about itself.
Unrelated, for the record
TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider fail on this branch — and I verified they fail identically on pristine origin/main, so they are pre-existing and not yours. Worth someone filing separately; they will keep showing up in every reviewer's local run until then.
|
@coderabbitai The latest review found no actionable comments and confirmed the previous blocker is fixed. Please clear the stale changes-requested review and approve the latest commit if no issues remain. |
|
✅ Action performedComments resolved and changes approved. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. All four are fixed, and I re-ran the same probes that found them rather than reading the diff.
The blocking one, an interior offset/limit read earning no read-before-edit credit, is gone: on a 1000-line file offset=500 limit=10 now comes back truncated=false with seen_lines=500-509 and the follow-up edit succeeds. It used to be truncated=true, reason="limit", empty seen_lines, edit refused.
The other three as well. Mixing argument families no longer reads the wrong lines: start_line+limit and offset+end_line both return 5-6 now, where they gave 1-2 and 5-8. Byte mode is back in the schema ([offset limit byte_offset byte_limit path]), so a file with a line past the read budget is reachable again. And a ranged Go read starting inside a raw string keeps its content: SECRET-MARKER-ONE survives intact instead of arriving as SECRET - MARKER - ONE, and the line that looked like a comment is no longer deleted.
internal/tools and internal/minify both pass, build and vet clean.
One note for whoever merges rather than for you: this branch carries a merge of main from earlier today, so the diff reads larger than the change.
|
@anandh8x approved. All four are fixed and I re-ran the probes that found them rather than reading the diff, so this is verified rather than assumed.
The one I care most about is the interior range: Thanks for taking the raw-string one seriously too. That was the quietest of the four and the one most likely to have shipped unnoticed, since it corrupted content while reporting a successful compaction. |
Gitlawb#867 landed on main while this branch was being rebased, reshaping read_file's schema to the canonical path/offset/limit contract and rewording its description. The golden hashes the tool DEFINITION bytes, so a description change on main moves it — the same way Gitlawb#838's rewording did before. The new value is what the fixture's tools actually hash to on this base, and it is the value CI computed independently: the macOS and Ubuntu smoke jobs both reported got ea7b4e64… against the old constant, which is the figure recorded here now. This is main's schema change, not a posture leak from this branch. The byte-identical guarantee is unaffected: what the guard protects is that a posture-off run's definitions match a binary built from the same main, and both sides moved together.
Gitlawb#867 landed on main while this branch was being rebased, reshaping read_file's schema to the canonical path/offset/limit contract and rewording its description. The golden hashes the tool DEFINITION bytes, so a description change on main moves it — the same way Gitlawb#838's rewording did before. The new value is what the fixture's tools actually hash to on this base, and it is the value CI computed independently: the macOS and Ubuntu smoke jobs both reported got ea7b4e64… against the old constant, which is the figure recorded here now. This is main's schema change, not a posture leak from this branch. The byte-identical guarantee is unaffected: what the guard protects is that a posture-off run's definitions match a binary built from the same main, and both sides moved together.
3 participants
What changed
read_filecontract onpath,offset, andlimitgrepWhy
The previous
read_fileschema exposed overlapping line and byte pagination systems. Some models combined both systems in one request, producing deterministic argument failures and repeated retries until the loop guard stopped the run. The canonical contract removes that ambiguity while preserving compatibility for existing callers.GPT-5.5 benchmark
Identical read-only inspection prompt against the Zero source tree:
read_filecallsThe pre-change token total is lower only because the task halted without producing the requested answer; the primary measured improvement is successful completion without the invalid-call loop.
Validation
make fmt-checkgo vet ./...go test ./...with an isolated empty user config rootgo run ./cmd/zero-release buildgo run ./cmd/zero-release smokemake lint-staticmake vulncheckgit diff HEAD --checkSummary by CodeRabbit
offset/limitline-range selection for standard and minified source reading.