You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(read-file): propagate beyond-EOF error instead of reporting file as empty - #1734
read_file with an offset beyond the end of a non-empty file returned "Note: File is empty" — the producer's real error string (Error: offset N is beyond file end (M lines)) was discarded by a consumer branch that was also dead code for genuinely empty files (an empty file parses to one blank line, returnedLines: 1). The model, told the file is empty, can regenerate and overwrite content from memory.
This PR gates the empty-note on an actually-empty file (returnedLines === 0 && totalLines === 0) so the beyond-EOF error propagates to the model as an error.
Test Procedure
New regression test: 5-line file, {path, mode:"slice", offset:100} → result contains "beyond file end" and not "Note: File is empty"; also asserts the 1-based→0-based offset conversion. Verified failing before the fix.
Existing empty-file test kept passing (tightened to assert the exact note text).
vitest run core/tools/__tests__/readFileTool.spec.ts integrations/misc/__tests__/indentation-reader.spec.ts → 116/116.
The empty-note branch is now unreachable via the real producer (it always returns totalLines >= 1); a pre-existing spec mocks an impossible totalLines: 0 shape to exercise it — kept as-is to avoid scope creep.
Get in Touch
GitHub: @myk1yt — please tag me here; I monitor notifications.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review.
Recent review details
Additional context used
Path-based instructions: 5
Treat model, provider, MCP, path, command, and tool data as untrusted.
CodeRabbit configuration file
Files:
src/core/tools/ReadFileTool.ts
src/core/tools/__tests__/readFileTool.spec.ts
Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.
[warning] 326-326: Mutation test advisory
src/core/tools/ReadFileTool.ts:326: Survived ConditionalExpression mutant (replacement: true). See the job summary for the complete list and resolution guidance.
File reads with an offset beyond the end of a non-empty file now report that the requested position is beyond the file, instead of incorrectly indicating the file is empty.
Empty-file messages are shown only when the file contains no lines.
Out-of-range positions are reported using 1-based line numbers.
Tests
Updated coverage for empty files and slice requests beyond the end of a file.
The slice reader now reports beyond-end offsets as 1-based line numbers. ReadFileTool now emits the empty-file note only when the total line count is zero. Tests cover both behaviors.
Out-of-range errors now report the 1-based offset. Tests check the updated offset and error text.
Empty-file note and error propagation src/core/tools/ReadFileTool.ts, src/core/tools/__tests__/readFileTool.spec.ts
The empty-file note now requires zero returned lines and zero total lines. Mocked and real-file tests verify that beyond-end errors are returned without the empty-file note. The mocked test also verifies conversion of offset 100 to 99 for readWithSlice.
Beyond-end errors now reach the caller with the requested line number. No merge-blocking risk was identified.
Pre-merge checks: 7 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Lifecycle Resource Cleanup
The new integration test creates a temporary directory at src/core/tools/__tests__/readFileTool.spec.ts:618, then writes the fixture at line 620 before entering the try block at line 628. If `writ…
Start the try block immediately after mkdtemp succeeds, and move fixture creation and module setup inside it. Keep realFs.rm(tempDir, { recursive: true, force: true }) in the finally block so every post-creation failure path cleans …
Passed checks: 7 passed
Check name
Status
Explanation
Title check
The title clearly summarizes the main change: beyond-end-of-file errors now propagate instead of being reported as an empty file.
Description check
The description includes the linked issue, implementation details, test procedure, checklist, documentation status, additional notes, and contact information. It explains the regression and reports re…
Linked Issues check
Issue #1728 requires two things. First, an out-of-range offset on a non-empty file must return the beyond-EOF error. Second, the empty-file note must appear only for an actually empty file. Current he…
Out of Scope Changes check
The whole-PR production changes are limited to two lines. The ReadFileTool.ts line gates the empty-file note, which implements #1728 directly. The indentation-reader.ts line changes the text of the sa…
Regression Evidence
Focused regression coverage is present at the relevant layers. The ReadFileTool test covers the changed consumer branch with returnedLines: 0, totalLines: 5, verifies offset 100 is converted to `9…
Security Boundaries
No changed path matches the security failure conditions. src/core/tools/ReadFileTool.ts only changes when the empty-file note is selected, and src/integrations/misc/indentation-reader.ts only chan…
Persistence Integrity
No changed persistence path exists. The production changes only alter read-result formatting in ReadFileTool.processTextFile and the error text from readWithSlice; they do not add or modify state …
Full details: Description check
Explanation
The description includes the linked issue, implementation details, test procedure, checklist, documentation status, additional notes, and contact information. It explains the regression and reports relevant test results.
Full details: Linked Issues check
Explanation
Issue #1728 requires two things. First, an out-of-range offset on a non-empty file must return the beyond-EOF error. Second, the empty-file note must appear only for an actually empty file. Current head implements both. In src/core/tools/ReadFileTool.ts, the note condition is now result.returnedLines === 0 && result.totalLines === 0, so a result with totalLines 5 keeps its Error: offset ... is beyond file end (5 lines) content. In src/integrations/misc/indentation-reader.ts, readWithSlice now reports offset + 1 in that error. This is the model-facing 1-based offset, so the model sees the value it sent (100, not 99). The regression test in readFileTool.spec.ts covers a 5-line file with offset 100 and asserts the error text and the absence of the note. The indentation-reader and unicode specs were updated to the new message text. The change since the previous review does not alter the issue requirement, which the previous PASS already covered. Test results in the PR description are author-reported; this review confirmed the production diff and the spec diffs by reading them, not by running tests. No coding requirement from #1728 is unmet.
Full details: Out of Scope Changes check
Explanation
The whole-PR production changes are limited to two lines. The ReadFileTool.ts line gates the empty-file note, which implements #1728 directly. The indentation-reader.ts line changes the text of the same beyond-EOF error that #1728 concerns. It does not change the validation condition or the returned result fields. The offset+1 change is explained by review feedback recorded in the PR objectives, and the specs pin it. The spec changes in readFileTool.spec.ts, indentation-reader.spec.ts, and indentation-reader-unicode.spec.ts support the new behavior or update expectations to the new message text. The unicode spec still passes offset 1 to a one-line file and now expects the 1-based text, which is consistent. No unrelated behavior or refactoring was found in the diff.
Full details: Regression Evidence
Explanation
Focused regression coverage is present at the relevant layers. The ReadFileTool test covers the changed consumer branch with returnedLines: 0, totalLines: 5, verifies offset 100 is converted to 99, preserves the beyond-EOF error, and rejects Note: File is empty. The same suite covers the empty-note branch. A real-file integration case exercises the producer and consumer together. Producer tests cover the 1-based error offset for ordinary and Unicode input.
Full details: Security Boundaries
Explanation
No changed path matches the security failure conditions. src/core/tools/ReadFileTool.ts only changes when the empty-file note is selected, and src/integrations/misc/indentation-reader.ts only changes the displayed offset from 0-based to 1-based. These changes do not expose secrets or PII, execute input, or alter path validation, .rooignore checks, or approval flow. The remaining changes are tests.
Full details: Persistence Integrity
Explanation
No changed persistence path exists. The production changes only alter read-result formatting in ReadFileTool.processTextFile and the error text from readWithSlice; they do not add or modify state writes, atomicity, awaiting, rollback, or default propagation. The added filesystem write is test setup only and is awaited.
Full details: Lifecycle Resource Cleanup
Explanation
The new integration test creates a temporary directory at src/core/tools/__tests__/readFileTool.spec.ts:618, then writes the fixture at line 620 before entering the try block at line 628. If writeFile or the intervening setup fails, the finally block does not run and the temporary directory remains on disk.
Resolution
Start the try block immediately after mkdtemp succeeds, and move fixture creation and module setup inside it. Keep realFs.rm(tempDir, { recursive: true, force: true }) in the finally block so every post-creation failure path cleans up the temporary directory.
Finishing Touches
🧪 Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ESLint
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
src/core/tools/ReadFileTool.ts
ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.
src/core/tools/__tests__/readFileTool.spec.ts
ESLint skipped: the matched ESLint configuration already failed (missing-dependency).
Thanks for contributing. This comment tracks the review sequence and the next action.
Current step: Awaiting fresh human maintainer or CODEOWNER approval.
Automated review is complete for the latest commit but does not replace human approval.
Review-state labels are managed by this workflow; do not edit them manually. community-approved is managed the same way — do not add or remove it manually. It signals a fresh community code approval for the current head as an advisory priority only; maintainer review is still required.
What: readWithSlice now prints offset + 1 in its beyond-file-end error and
both spec fixtures assert the 1-based value.
Why: read_file takes a 1-based offset from the model, but the error text
showed the 0-based value passed to readWithSlice (offset 100 read back as
"offset 99"), disagreeing with indentation mode which reports 1-based lines.
Impact: the model sees the same line number it asked for, so both read modes
report consistent line numbers in errors.
Thanks for the review, @edelauna. Addressed the offset nit in 5e4c7b9:
`readWithSlice` now reports the 1-based offset in the beyond-EOF error (`offset + 1`), so the line number matches what the model sent and agrees with indentation mode (1-based line numbers).
Updated both fixtures and pinned the exact 1-based message in the indentation-reader spec.
Also merged current upstream/main (2681ccb); the branch is MERGEABLE with a minimal diff (original fix + this nit).
Verification: `pnpm --dir src run check-types` passes; `vitest run integrations/misc` (186 tests) and `readFileTool.spec.ts` pass; eslint clean on touched files (suppression counts unchanged).
@edelauna thanks for the precise catch — fixed. readWithSlice now reports the model-facing 1-based offset in the beyond-EOF message (so offset: 6 on a 5-line file reads back "offset 6"), and both fixtures pin the exact message. All 17 checks are green and the thread is resolved. Could you take another look when you have a moment?
This PR has been awaiting author changes for 14 days and will be automatically closed in 7 days. Please address the review comments or leave a comment if you need more time.
…message
- src/core/tools/__tests__/readFileTool.spec.ts: kept main's import block
(main already imports `type Task` alongside ToolExecutionContext, so the
branch's duplicate `import type { Task }` was dropped); re-applied the
branch's beyond-EOF regression test and tightened empty-file assertion on
top of main's reworked mocks (activeToolContexts / toolCallId).
- src/integrations/misc/__tests__/indentation-reader-unicode.spec.ts: main
added this spec pinning the old 0-based beyond-EOF message; updated the
expectation to the branch's 1-based offset (offset 1 -> "offset 2").
- Production hunks auto-merged unchanged: the empty-note guard
(returnedLines === 0 && totalLines === 0) and the 1-based beyond-EOF
message in readWithSlice.
@edelauna Two updates on this branch, both verified at head e058ba9b0.
1. has-conflicts is gone. GitHub now reports mergeable: MERGEABLE (it previously reported CONFLICTING). Upstream main (0952a8999) is merged in e058ba9b0, and the reconcile workflow dropped the has-conflicts and stale-awaiting-author labels; only awaiting-author remains.
2. Your offset nit is fixed.src/integrations/misc/indentation-reader.ts:384 now reports the model-facing 1-based offset:
content: `Error: offset ${offset+1} is beyond file end (${totalLines} lines)`,
readWithSlice still takes a 0-based offset (src/core/tools/ReadFileTool.ts:309 converts with offset1 - 1), so the message matches the number the model sent and agrees with indentation mode's 1-based line numbers. Specs pin the exact string:
src/integrations/misc/__tests__/indentation-reader.spec.ts:285 — offset 1001 for readWithSlice(SIMPLE_CODE, 1000, 10)
src/integrations/misc/__tests__/indentation-reader-unicode.spec.ts:130 — offset 2 for readWithSlice(longLine, 1)
src/core/tools/__tests__/readFileTool.spec.ts:607 — the regression test asserts beyond file end reaches the model and Note: File is empty does not
Verification at e058ba9b0: targeted specs 156/156 pass; full src suite 11771 passed / 40 failed, where all 40 are the pre-existing __tests__/dist_assets.spec.ts failures that need a built src/dist/ bundle (same count on untouched main); check-types clean; eslint clean on all five touched files with no suppression-count increase.
CI: 19 checks pass, CodeRabbit skipping (auto-review is label-gated on coderabbit-review-active), and PR review gate stays pending until a maintainer re-reviews. No failing checks.
One advisory worth naming: mutation-diff passes but annotates a survived ConditionalExpression mutant at src/core/tools/ReadFileTool.ts:326 (the result.returnedLines === 0 half of the new guard replaced by true). It is equivalent under the producer's invariant: both readWithSlice and readWithIndentation derive totalLines from content.split("\n"), which is at least 1 for any string, so totalLines === 0 is unreachable and the second conjunct alone decides the branch. Killing it would require asserting behavior for {returnedLines > 0, totalLines: 0}, a shape the producer cannot emit.
…hSlice producer
What: add one integration test to readFileTool.spec.ts that unmocks the
indentation-reader, fs/promises, path and isbinaryfile modules, builds a
ReadFileTool against a real temp file with 5 lines, and calls it with the
1-based offset 100.
Why: the existing beyond-EOF regression test mocks readWithSlice, so it
only proved that the tool forwards whatever string the stub returns. The
1-based offset in the user-facing message ("offset 100 is beyond file
end") was asserted against a hand-written stub whose shape had to be kept
in sync with indentation-reader.ts by hand. A change to the real producer
would not have failed that test.
Impact: the beyond-EOF error path is now covered end-to-end through the
real producer against a real file. Verified by mutating
indentation-reader.ts to emit the 0-based offset: the new test fails with
"offset 99", and passes again once reverted. Test count 84 -> 85 in the
core vitest config. No production code changed.
Ping for reviewer: all feedback on this PR is addressed at the current head (5cc4220b0). The 1-based offset ask is implemented at indentation-reader.ts:384 (offset + 1 in the beyond-EOF message), the 1-based external contract is documented at ReadFileTool.ts:307, and fixtures at spec :594/:634 assert the 1-based form end-to-end through the real readWithSlice producer. CI is green. No further code changes pending on our side - re-review when convenient, thanks!
I have re-requested my review. For future rounds, after addressing requested changes and pushing fixes, please use the Re-request review button beside the reviewer who requested changes. New commits, resolved threads and comment mentions do not clear the awaiting-author blocker; a formal review re-request does. Please include a brief summary of the fixes when ready.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
coderabbit-review-active
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
edelauna
left a comment
Related GitHub Issue
Closes: #1728
Description
read_filewith anoffsetbeyond the end of a non-empty file returned "Note: File is empty" — the producer's real error string (Error: offset N is beyond file end (M lines)) was discarded by a consumer branch that was also dead code for genuinely empty files (an empty file parses to one blank line,returnedLines: 1). The model, told the file is empty, can regenerate and overwrite content from memory.This PR gates the empty-note on an actually-empty file (
returnedLines === 0 && totalLines === 0) so the beyond-EOF error propagates to the model as an error.Test Procedure
{path, mode:"slice", offset:100}→ result contains "beyond file end" and not "Note: File is empty"; also asserts the 1-based→0-based offset conversion. Verified failing before the fix.vitest run core/tools/__tests__/readFileTool.spec.ts integrations/misc/__tests__/indentation-reader.spec.ts→ 116/116.pnpm run test:coverage:core→ exit 0 (162 files / 3016 passed).pnpm run check-types→ 0 errors. ESLint → clean, suppression counts unchanged.Pre-Submission Checklist
Visual Snapshots
N/A.
Videos (interaction / animation only)
N/A.
Documentation Updates
Additional Notes
The empty-note branch is now unreachable via the real producer (it always returns
totalLines >= 1); a pre-existing spec mocks an impossibletotalLines: 0shape to exercise it — kept as-is to avoid scope creep.Get in Touch
GitHub: @myk1yt — please tag me here; I monitor notifications.