Conversation
code-crusher
commented
Jan 9, 2026
- refine read tool + settings cleanup
- changelog +
|
✅ Reviewed the changes: Version bump to 5.0.2 matches PR title. No issues found. |
There was a problem hiding this comment.
🧪 PR Review is completed: Review of release/v5.0.2. Critical fix identified in readFileTool.ts where the default file read case bypasses the new 1000-line limit. Also found input validation improvements for kilocode.ts and minor style cleanup.
Skipped files
CHANGELOG.md: Skipped file pattern
⬇️ Low Priority Suggestions (1)
webview-ui/src/components/ui/button.tsx (1 suggestion)
Location:
webview-ui/src/components/ui/button.tsx(Lines 8-8)🔵 Style
Issue:
rounded-xsis likely a typo or non-standard utility class, androunded-mdis also applied at the end of the class string, which would override it anyway.Fix: Remove the redundant/incorrect
rounded-xsclass.Impact: Cleans up CSS classes and avoids confusion.
- "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-xs text-base font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0 cursor-pointer active:opacity-80 border border-[var(--color-matterai-border)] outline-none rounded-md", + "inline-flex items-center justify-center gap-2 whitespace-nowrap text-base font-medium transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0 cursor-pointer active:opacity-80 border border-[var(--color-matterai-border)] outline-none rounded-md",
| // Handle offset-only reads (no limit specified) - cap to MAX_READ_FILE_LINES | ||
| if (fileResult.offset !== undefined && fileResult.offset > 1) { | ||
| const startLine = fileResult.offset | ||
| const endLine = startLine + MAX_READ_FILE_LINES - 1 | ||
| const actualEndLine = Math.min(endLine, totalLines) | ||
| const linesRead = actualEndLine - startLine + 1 | ||
| const content = addLineNumbers( | ||
| await readLines(fullPath, actualEndLine - 1, startLine - 1), | ||
| startLine, | ||
| ) | ||
| let xmlContent = content | ||
| if (totalLines > actualEndLine) { | ||
| xmlContent = `[showing ${linesRead} lines from offset ${startLine}, capped at ${MAX_READ_FILE_LINES} lines. Total file length: ${totalLines} lines]\n${content}` | ||
| } | ||
| updateFileResult(relPath, { | ||
| xmlContent, | ||
| }) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🔴 Security & Performance
Issue: The current logic handles explicit limit and explicit offset > 1, but falls through for the default case (no params or offset=1 with no limit). This means a standard read_file call will bypass the MAX_READ_FILE_LINES cap and read the entire file, violating the tool's new constraint.
Fix: Update the logic to handle the default case (where limit is undefined) by applying the cap relative to the offset (defaulting to 1).
Impact: Prevents context window overflow and ensures consistent behavior for all file read operations.
| // Handle offset-only reads (no limit specified) - cap to MAX_READ_FILE_LINES | |
| if (fileResult.offset !== undefined && fileResult.offset > 1) { | |
| const startLine = fileResult.offset | |
| const endLine = startLine + MAX_READ_FILE_LINES - 1 | |
| const actualEndLine = Math.min(endLine, totalLines) | |
| const linesRead = actualEndLine - startLine + 1 | |
| const content = addLineNumbers( | |
| await readLines(fullPath, actualEndLine - 1, startLine - 1), | |
| startLine, | |
| ) | |
| let xmlContent = content | |
| if (totalLines > actualEndLine) { | |
| xmlContent = `[showing ${linesRead} lines from offset ${startLine}, capped at ${MAX_READ_FILE_LINES} lines. Total file length: ${totalLines} lines]\n${content}` | |
| } | |
| updateFileResult(relPath, { | |
| xmlContent, | |
| }) | |
| continue | |
| } | |
| // Handle reads with no explicit limit - cap to MAX_READ_FILE_LINES | |
| // This handles both offset-only reads and default reads (no offset, no limit) | |
| if (fileResult.limit === undefined) { | |
| const startLine = fileResult.offset ?? 1 | |
| const endLine = startLine + MAX_READ_FILE_LINES - 1 | |
| const actualEndLine = Math.min(endLine, totalLines) | |
| const linesRead = actualEndLine - startLine + 1 | |
| const content = addLineNumbers( | |
| await readLines(fullPath, actualEndLine - 1, startLine - 1), | |
| startLine, | |
| ) | |
| let xmlContent = content | |
| if (totalLines > actualEndLine) { | |
| xmlContent = `[showing ${linesRead} lines from offset ${startLine}, capped at ${MAX_READ_FILE_LINES} lines. Total file length: ${totalLines} lines]\n${content}` | |
| } | |
| updateFileResult(relPath, { | |
| xmlContent, | |
| }) | |
| continue | |
| } |
| path: filePath, | ||
| offset: file.offset ?? 1, | ||
| limit: file.limit, // undefined means read complete file | ||
| offset: !isNaN(parsedOffset as number) ? parsedOffset : 1, |
There was a problem hiding this comment.
🟡 Robustness
Issue: parseInt can return 0 or negative numbers. If offset is 0, it may cause issues with 1-based line indexing in downstream functions (potentially resulting in -1 index).
Fix: Clamp the parsed offset to be at least 1.
Impact: Prevents potential runtime errors or incorrect file reads with invalid offsets.
| offset: !isNaN(parsedOffset as number) ? parsedOffset : 1, | |
| offset: !isNaN(parsedOffset as number) ? Math.max(1, parsedOffset as number) : 1, |