-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix grok code fast can't read parts of files #8147
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
Fix grok code fast can't read parts of files #8147
Conversation
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.
Thank you for your contribution! I've reviewed the changes and found several issues that need attention. While this fix addresses the immediate problem by disabling the single-file read mode for all models (allowing Grok to use the full read_file tool with line range support), there are some concerns about the implementation approach and missing documentation.
| export function shouldUseSingleFileRead(modelId: string): boolean { | ||
| return modelId.includes("grok-code-fast-1") | ||
| export function shouldUseSingleFileRead(_modelId: string): boolean { | ||
| return false |
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.
Is this intended as a permanent fix? With this change, shouldUseSingleFileRead() always returns false, making this function and the entire simpleReadFileTool effectively dead code. If this is the final solution, should we consider removing this function and all related code entirely?
Alternatively, would it be better to fix the simpleReadFileTool to support line ranges for models that truly need the simplified format?
|
|
||
| /** | ||
| * Check if a model should use single file read format | ||
| * @param modelId The model ID to check |
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 JSDoc comment still says "Check if a model should use single file read format" and "@returns true if the model should use single file reads", but the function now always returns false. Could you update the documentation to reflect the current behavior?
| * @param modelId The model ID to check | |
| /** | |
| * Check if a model should use single file read format | |
| * @param modelId The model ID to check | |
| * @returns false - Single file read mode is currently disabled for all models | |
| */ |
| */ | ||
| export function shouldUseSingleFileRead(modelId: string): boolean { | ||
| return modelId.includes("grok-code-fast-1") | ||
| export function shouldUseSingleFileRead(_modelId: string): boolean { |
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 modelId parameter is now prefixed with underscore to indicate it's unused, but the function still accepts it. If we're keeping this function, should we deprecate it properly with a @deprecated tag? Or if this is temporary, could you add a TODO comment explaining the plan?
|
Closing, see #8146 (comment) |
Related GitHub Issue
Closes:
#8146
Roo Code Task Context (Optional)
Description
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
shouldUseSingleFileReadinsingle-file-read-models.tsnow always returnsfalse, disabling single file read for all models.shouldUseSingleFileReadinsingle-file-read-models.tsnow always returnsfalse, disabling single file read for all models.modelIdfromshouldUseSingleFileRead.This description was created by
for 929a68f. You can customize this summary. It will automatically update as commits are pushed.