-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(read_file): implement CodexCLI-inspired slice/indentation reading modes #10257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Re-review complete for e73c499. No net new issues found in the latest commit; one prior issue remains.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
b153038 to
b0362be
Compare
| } | ||
|
|
||
| // Map indentation configuration | ||
| if (file.indentation && typeof file.indentation === "object") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createReadFileTool() defines indentation config keys as anchorLine/maxLevels/includeSiblings/includeHeader, but convertFileEntries() only reads indentation.anchor_line/max_levels/etc, so a tool call that follows the schema will silently drop indentation settings. Consider accepting both casings (camelCase + snake_case) or aligning the schema and examples to the parser.
Fix it with Roo Code or mention @roomote and request a fix.
| let xmlInfo = "" | ||
| let nativeInfo = "" | ||
|
|
||
| if (result.lineCount === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readSlice() rejects when offset is beyond EOF, but ReadFileTool special-cases lineCount === 0 as “offset exceeds file length”. That branch will never be hit with the current readSlice() behavior, so callers always see an error instead of a graceful empty page. If you want “empty content” semantics, consider returning { lineCount: 0, ... } when offset is past EOF rather than throwing.
Fix it with Roo Code or mention @roomote and request a fix.
| description: "Configuration for indentation mode. Only used when mode is 'indentation'.", | ||
| properties: { | ||
| anchorLine: { | ||
| type: ["integer", "null"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The native tool schema for read_file advertises indentation.anchorLine/maxLevels/includeSiblings/includeHeader (camelCase), but the examples still show indentation: { maxLevels: 2 } while the parser expects snake_case keys like max_levels. Either the schema should match what the parser accepts, or the parser should accept the camelCase keys models are likely to send.
Fix it with Roo Code or mention @roomote and request a fix.
7ab3bf4 to
91018ae
Compare
| mode?: ReadMode | ||
| /** Configuration for indentation mode */ | ||
| indentation?: IndentationConfig | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileEntry in tool-params.ts still defines limit?: number, but ReadFileTool intentionally ignores entry.limit (uses maxReadFileLine only). This is a contract mismatch: models may send limit expecting it to work, and it will be silently ignored. Either document that limit is ignored (and remove it from schema/types), or wire it through and clamp to maxReadFileLine.
Fix it with Roo Code or mention @roomote and request a fix.
…g modes Replace line_ranges with new offset/limit/mode API for improved file reading: - Slice mode: simple line-by-line reading with offset pagination - Indentation mode: smart code block extraction based on indentation - Add rich metadata for pagination awareness (hasMoreBefore/After, etc.) - Remove deprecated truncateDefinitions helper - Update tool definition with new parameters and examples - Default maxReadFileLine from 500 to 2000 lines
- Use consistent camelCase in error messages (anchorLine, maxLines instead of snake_case) - Accept both camelCase and snake_case for indentation config in NativeToolCallParser - Continue counting total lines after hitting limit in readSlice() for accurate metadata - Return empty content instead of throwing when offset exceeds file length - Disable strict mode in tool schema when partial reads enabled to allow truly optional params - Update tests to match new behavior
…indentation mode When totalLinesInFile is 0, there are no lines before the anchor point, so hasMoreBefore must be false regardless of the offset/anchorLine value.
The limit parameter was defined in FileEntry but intentionally ignored by ReadFileTool since the line limit is controlled by the maxReadFileLine setting. This removes the unused type field and parsing code to align the types with actual behavior. - Remove limit from FileEntry interface in tool-params.ts - Remove limit parsing from NativeToolCallParser.convertFileEntries() - Remove limit from FileResult interface in ReadFileTool.ts - Update comment to remove obsolete limit mention
…leLine setting Addresses review comment about FileEntry.limit being defined but ignored. The limit parameter was intentionally not exposed to models - it's controlled by the maxReadFileLine user setting instead. Updated the comment in NativeToolCallParser.ts to clarify this design decision.
Updates tests to match the new API that replaces line_ranges with: - offset: 1-indexed starting line number - mode: 'slice' or 'indentation' reading mode - indentation: configuration for indentation mode Also updates strict mode expectations since partialReadsEnabled (default true) now sets strict to false to allow optional parameters.
c9fdfa8 to
4dce77f
Compare
| hasMoreAfter: r.number < records.length, | ||
| linesBeforeStart: r.number - 1, | ||
| linesAfterEnd: records.length - r.number, | ||
| truncatedByLimit: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In readIndentationBlock(), the finalLimit === 1 fast-path sets truncatedByLimit: true unconditionally. When the file itself is only 1 line long, that incorrectly reports truncation even though the read is complete.
| truncatedByLimit: true, | |
| truncatedByLimit: records.length > 1, |
Fix it with Roo Code or mention @roomote and request a fix.
…test The ReadFileTool's execute method requires additional Task methods that were not mocked: - sayAndCreateMissingParamError: called when files array is empty/undefined - cwd: used for path resolution - apiConfiguration: used for protocol resolution - taskToolProtocol: used for protocol resolution - rooIgnoreController: used for access validation - fileContextTracker: used for tracking file context This fix ensures the test properly mocks all dependencies required by the ReadFileTool when invoked through presentAssistantMessage.
Summary
Implements a new, more intuitive API for the
read_filetool, inspired by OpenAI's Codex CLI read_file implementation. This replaces the previousline_rangesapproach with a cleaner offset/limit/mode-based system that provides better pagination and smarter code extraction capabilities.closes #10239
Changes
New API Parameters: Replace
line_rangeswith:offset: 1-indexed starting line number (default: 1)limit: Maximum lines to return (controlled by maxReadFileLine setting)mode: Eitherslice(simple reading) orindentation(smart block extraction)indentation: Configuration for indentation mode (anchorLine, maxLevels, includeSiblings, includeHeader)New
read-file-content.tsmodule: Core implementation with:readSlice(): Simple line-by-line reading with offset paginationreadIndentationBlock(): Smart extraction of code blocks based on indentation levelsEnhanced Model Awareness: Tool descriptions now include the configured line limit so models understand pagination constraints
Improved Default Limit: Changed default maxReadFileLine from 500 to 2000 lines for better out-of-box experience
Removed Deprecated Code:
truncateDefinitions.tshelper and its testsCredits
This implementation is inspired by and adapts concepts from OpenAI's Codex CLI, particularly their approach to file reading with offset-based pagination and indentation-aware code block extraction.
Important
Introduces new file reading modes with
offset,limit, andmodeparameters, replacingline_rangesin theread_filetool, and updates related settings and localization.line_rangeswithoffset,limit, andmodeparameters inread_filetool.sliceandindentationmodes for reading files.maxReadFileLinedefault from 500 to 2000.readSlice()andreadIndentationBlock()inread-file-content.ts.truncateDefinitions.tsand related code.ContextManagementSettings.tsxto reflect newmaxReadFileLinedefault.readFileparameters and descriptions.This description was created by
for e73c499. You can customize this summary. It will automatically update as commits are pushed.