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: 4 (Complex) | ~45 minutes 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
| if read.Status != StatusOK || read.Truncated { | ||
| t.Fatalf("bounded canonical read must be exact: status=%s truncated=%v meta=%#v", read.Status, read.Truncated, read.Meta) | ||
| } | ||
| if read.Meta["seen_lines"] != "500-509" { | ||
| t.Fatalf("bounded canonical read did not receive exact-read credit: %#v", read.Meta) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/tools/read_file.go --items all
rg -n -C 10 'offset|limit|endLine|Truncated|RecordSeenRange' \
internal/tools/read_file.go internal/tools/file_tracker.goRepository: Gitlawb/zero
Length of output: 26265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- internal/tools/read_file.go relevant sections ---\n'
sed -n '1,290p' internal/tools/read_file.go
printf '\n--- internal/tools/file_tracker.go RecordSeenRange ---\n'
sed -n '100,210p' internal/tools/file_tracker.go
printf '\n--- tests around bounded canonical read ---\n'
sed -n '260,335p' internal/tools/file_safety_test.go
printf '\n--- file_tracker references to Result.Truncated and seen ranges ---\n'
rg -n -C 4 'Result|Truncated|RecordSeenRange|SeenRange|seen_lines|pendingFileObservation' internal/tools -g '*.go'Repository: Gitlawb/zero
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- internal/tools/read_file.go relevant sections ---'
sed -n '1,290p' internal/tools/read_file.go
echo '--- internal/tools/file_tracker.go RecordSeenRange ---'
sed -n '100,210p' internal/tools/file_tracker.go
echo '--- tests around bounded canonical read ---'
sed -n '260,335p' internal/tools/file_safety_test.go
echo '--- file_tracker references to Result.Truncated and seen ranges ---'
rg -n -C 4 'Result|Truncated|RecordSeenRange|SeenRange|seen_lines|pendingFileObservation' internal/tools -g '*.go'Repository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- internal/tools/read_file.go relevant sections ---'
sed -n '1,290p' internal/tools/read_file.go
echo '--- internal/tools/file_tracker.go RecordSeenRange ---'
sed -n '100,210p' internal/tools/file_tracker.go
echo '--- internal/tools/file_safety_test.go test body ---'
sed -n '260,335p' internal/tools/file_safety_test.go
echo '--- registry CommitFileObservation and budget helper definitions ---'
sed -n '230,265p' internal/tools/registry.go
rg -n -C 3 'type outputBudgetBuilder|func .*Output|func .*Budget|Retruncated|Truncated|Reduce' internal/tools -g '*.go' | sed -n '1,260p'Repository: Gitlawb/zero
Length of output: 37277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- output boundary implementation ---'
sed -n '1,112p' internal/tools/output_boundary.go
rg -n -C 4 'type budgetedOutput|func budgetSemanticOutput|func newHeadTailOutputBucket|func newOutputBudget|Truncated|Truncate|text' internal/tools/output_boundary.go internal/tools -g '*.go' | sed -n '1,260p'
echo '--- TestCanonicalLimitedReadAuthorizesEdit ---'
grep -n -C 25 'func TestCanonicalLimitedReadAuthorizesEdit' internal/tools/file_safety_test.goRepository: Gitlawb/zero
Length of output: 25685
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate registry RunWithOptions/apply budget sequence ---'
sed -n '120,195p' internal/tools/registry.go
sed -n '195,270p' internal/tools/registry.go
echo '--- output_budget budgetary helpers ---'
rg -n -C 5 'func budgetSemanticOutput|func newHeadTail|func newOutputBudget|type outputBudgetBuilder|func .*Truncated|func .*Result|func fitsOutputBudget' internal/tools -g '*.go'
echo '--- exact output_boundary lines for budget helpers ---'
sed -n '244,420p' internal/tools/output_boundary.goRepository: Gitlawb/zero
Length of output: 50369
Fix read_file line-range accounting.
offset: 500, limit: 10 should be the lines 500 through 509, and seen_lines should be 500-509. The bounded end is offset + limit - 1`; apply that end line before rendering so the range counts as exact.
🤖 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_safety_test.go` around lines 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.
|
@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. |
2 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 both source and minified-source reading.update_planand standardized source-exploration guidance.