-
Notifications
You must be signed in to change notification settings - Fork 0
release/v5.0.2 #30
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
release/v5.0.2 #30
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧪 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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, |