EXT-617: Refactor read_file tool with pagination support #10960
Conversation
Review completed. Found 2 new issues.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| // Determine truncation status | ||
| if (!reachedEof) { | ||
| truncated = true | ||
| truncationReason = "limit" | ||
| } else if (lineTruncationOccurred) { | ||
| truncated = true | ||
| truncationReason = "max_chars_per_line" | ||
| warnings.push( | ||
| `One or more lines exceeded ${maxCharsPerLine} characters and were truncated with "… [line truncated]" marker`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The line truncation warning is not added when !reachedEof is true. If a paginated chunk contains lines that were truncated due to maxCharsPerLine, users will not be notified because the code enters this branch and skips the else if. The warning should be added independently of the pagination limit check.
| // Determine truncation status | |
| if (!reachedEof) { | |
| truncated = true | |
| truncationReason = "limit" | |
| } else if (lineTruncationOccurred) { | |
| truncated = true | |
| truncationReason = "max_chars_per_line" | |
| warnings.push( | |
| `One or more lines exceeded ${maxCharsPerLine} characters and were truncated with "… [line truncated]" marker`, | |
| ) | |
| } | |
| // Determine truncation status | |
| if (!reachedEof) { | |
| truncated = true | |
| truncationReason = "limit" | |
| } | |
| if (lineTruncationOccurred) { | |
| truncated = true | |
| if (reachedEof) { | |
| truncationReason = "max_chars_per_line" | |
| } | |
| warnings.push( | |
| `One or more lines exceeded ${maxCharsPerLine} characters and were truncated with "… [line truncated]" marker`, | |
| ) | |
| } |
Fix it with Roo Code or mention @roomote and request a fix.
roomote
bot
force-pushed
the
feature/EXT-617-refactor-read-file-tool
branch
from
b3fc58b to
6b8a68b
Compare
BREAKING CHANGE: Complete rewrite of read_file tool API Changes: - New input schema: single file with offset/limit pagination - Output now returns structured JSON with metadata - Line numbering uses cat -n style (right-aligned, stable) - Default 2000 line limit per call with pagination via next_offset - Removed multi-file reads (now single file per call) - Removed user approval workflow (direct execution) - Removed image processing (to be added back in follow-up) This implements the spec from Linear issue EXT-617 for line-based pagination, reliable continuation via offset/limit, and bounded output for context budget management. Note: This is a work-in-progress draft. Tests and additional features need to be updated/added: - All existing tests need rewrite (300+ references to old API) - Image handling needs to be re-implemented - UI approval workflow needs removal - Binary file handling (PDF/DOCX) needs re-implementation
roomote
bot
force-pushed
the
feature/EXT-617-refactor-read-file-tool
branch
from
6b8a68b to
925018d
Compare
Update: Core Implementation Complete ✅I have pushed the core implementation of the refactored ✅ Implemented
|
| if (offset < 0) { | ||
| const error: ReadFileOutput = { | ||
| ok: false, | ||
| error: { | ||
| code: "io_error", | ||
| message: "Invalid offset: must be >= 0", | ||
| details: { offset }, | ||
| }, | ||
| } | ||
| pushToolResult(JSON.stringify(error, null, 2)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Inconsistent error tracking: invalid offset and limit validation (lines 152-176) don't set task.didToolFailInCurrentTurn = true, while other error paths (outside_workspace, is_directory, binary file, catch block) do. This could affect retry behavior since parameter validation failures won't be tracked the same way as other tool failures.
| if (offset < 0) { | |
| const error: ReadFileOutput = { | |
| ok: false, | |
| error: { | |
| code: "io_error", | |
| message: "Invalid offset: must be >= 0", | |
| details: { offset }, | |
| }, | |
| } | |
| pushToolResult(JSON.stringify(error, null, 2)) | |
| return | |
| } | |
| if (offset < 0) { | |
| const error: ReadFileOutput = { | |
| ok: false, | |
| error: { | |
| code: "io_error", | |
| message: "Invalid offset: must be >= 0", | |
| details: { offset }, | |
| }, | |
| } | |
| pushToolResult(JSON.stringify(error, null, 2)) | |
| task.didToolFailInCurrentTurn = true | |
| return | |
| } |
Fix it with Roo Code or mention @roomote and request a fix.
| if (limit <= 0 || limit > DEFAULT_LIMIT) { | ||
| const error: ReadFileOutput = { | ||
| ok: false, | ||
| error: { | ||
| code: "io_error", | ||
| message: `Invalid limit: must be between 1 and ${DEFAULT_LIMIT}`, | ||
| details: { limit, max: DEFAULT_LIMIT }, | ||
| }, | ||
| } | ||
| pushToolResult(JSON.stringify(error, null, 2)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Same issue here: missing task.didToolFailInCurrentTurn = true for consistency with other error paths.
| if (limit <= 0 || limit > DEFAULT_LIMIT) { | |
| const error: ReadFileOutput = { | |
| ok: false, | |
| error: { | |
| code: "io_error", | |
| message: `Invalid limit: must be between 1 and ${DEFAULT_LIMIT}`, | |
| details: { limit, max: DEFAULT_LIMIT }, | |
| }, | |
| } | |
| pushToolResult(JSON.stringify(error, null, 2)) | |
| return | |
| } | |
| if (limit <= 0 || limit > DEFAULT_LIMIT) { | |
| const error: ReadFileOutput = { | |
| ok: false, | |
| error: { | |
| code: "io_error", | |
| message: `Invalid limit: must be between 1 and ${DEFAULT_LIMIT}`, | |
| details: { limit, max: DEFAULT_LIMIT }, | |
| }, | |
| } | |
| pushToolResult(JSON.stringify(error, null, 2)) | |
| task.didToolFailInCurrentTurn = true | |
| return | |
| } |
Fix it with Roo Code or mention @roomote and request a fix.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Being worked on by @hannesrudolph |
Implements pagination support for the read_file tool with bounded reads, stable line numbering, and continuation hints.
Changes
Core Implementation
offset,limit,format, andmaxCharsPerLineto FileEntry typeformatWithCatN(): Right-aligned line numbers (cat_n style)truncateLine(): Line truncation with markerpaginateLines(): Main pagination logic with metadatageneratePaginationMessage(): User-friendly pagination messagesTool Behavior
nextOffset,reachedEof,truncated,truncationReasonBackward Compatibility
Testing
Examples
Read first 100 lines:
{ "files": [{ "path": "large-file.ts", "offset": 0, "limit": 100 }] }Continue reading next 100 lines:
{ "files": [{ "path": "large-file.ts", "offset": 100, "limit": 100 }] }Implementation Notes
This implementation takes a pragmatic approach:
View task on Roo Code Cloud
Important
Refactor
read_filetool to support pagination with bounded reads, stable line numbering, and continuation hints, ensuring backward compatibility and comprehensive testing.offset,limit,format, andmaxCharsPerLinetoReadFileInput.formatWithCatN(),truncateLine(),paginateLines(), andgeneratePaginationMessage().maxCharsPerLine(default: 2000) are truncated with a marker.nextOffset,reachedEof,truncated, andtruncationReason.ReadFileTooltests pass.This description was created by
for 925018d. You can customize this summary. It will automatically update as commits are pushed.