Utilize pipelining of grep_search -> read_file to eliminate turns#19574
Utilize pipelining of grep_search -> read_file to eliminate turns#19574gundermanc merged 23 commits intomainfrom
Conversation
…diate verification
…or modified files
…e efficiency Analysis of Experiment 2 vs 3 showed that pruning grep_search results caused the agent to lose context and enter redundant tool loops, increasing turn counts by ~28%. This change restores full history retention while keeping the readCache improvements.
Summary of ChangesHello @gundermanc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience and efficiency of several core tools. It improves the clarity of file modification outputs by providing contextual code snippets, makes search results more informative by automatically including surrounding lines for a small number of matches, and optimizes file read operations through caching. These changes aim to provide more actionable and readable feedback from tool executions. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Size Change: +4.65 kB (+0.02%) Total Size: 25.2 MB
ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a "Greedy Grep" feature, enhances edit and write-file tools, and adds caching to the read-file tool. A security audit identified high-severity Denial of Service (DoS) vulnerabilities. The new caching mechanism in ReadFileTool uses an unbounded global map, which can lead to memory exhaustion, and should be replaced with an LruCache as per established guidelines. It also bypasses the Policy Engine. Furthermore, the 'Greedy Grep' logic in GrepTool and RipGrepTool lacks file size validation, posing a significant risk of Out Of Memory (OOM) crashes when encountering large files. Additionally, there is code duplication in the diff snippet generation between edit.ts and write-file.ts, with one version containing a potential bug, which should be addressed by extracting it into a shared utility.
packages/core/src/tools/grep.ts
Outdated
| const content = await fsPromises.readFile( | ||
| fileMatches[0].absolutePath, | ||
| 'utf8', | ||
| ); | ||
| fileLines = content.split(/\r?\n/); |
There was a problem hiding this comment.
The 'Greedy Grep' logic reads the entire content of a matched file into memory using fsPromises.readFile without any size validation. This poses a high risk of an Out Of Memory (OOM) crash when encountering large files, which is a regression compared to the ReadFileTool's 20MB limit. Additionally, this logic is duplicated with packages/core/src/tools/ripGrep.ts; extracting it into a shared utility would improve maintainability and facilitate consistent application of necessary safeguards.
packages/core/src/tools/ripGrep.ts
Outdated
| const content = await fsPromises.readFile( | ||
| fileMatches[0].absolutePath, | ||
| 'utf8', | ||
| ); | ||
| fileLines = content.split(/\r?\n/); |
There was a problem hiding this comment.
The 'Greedy Grep' implementation in RipGrepTool reads the entire content of matched files into memory to provide context. There is no check on the file size before calling fsPromises.readFile. Reading large files into memory and splitting them into lines can cause the Node.js process to crash due to memory exhaustion (OOM), providing a straightforward vector for Denial of Service.
gemini-cli
bot
added
the
priority/p1
gundermanc
changed the title
gemini-cli
bot
added
the
🔒 maintainer only
|
|
||
| if (names_only) { | ||
| const filePaths = Object.keys(matchesByFile).sort(); | ||
| let llmContent = `Found ${filePaths.length} files with matches for pattern "${pattern}" ${searchLocationDescription}${include ? ` (filter: "${include}")` : ''}${wasTruncated ? ` (results limited to ${totalMaxMatches} matches for performance)` : ''}:\n`; |
There was a problem hiding this comment.
array of strings might be a bit clearer than doing the manual newlines, but this is also fine.
| @@ -351,6 +357,15 @@ class WriteFileToolInvocation extends BaseToolInvocation< | |||
| ); | |||
| } | |||
|
|
|||
| // Return a diff of the file before and after the write so that the agent | |||
| // can avoid the need to spend a turn doing a verification read. | |||
There was a problem hiding this comment.
hmmmmm, this one is an overwrite right? So the contents on disk are exactly as the model specified? Maybe I'm misunderstanding something, if the contents of the file after write are identical to what the model just passed to write_file this might not be necessary?
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
Summary
Utilize 'pipelining' of tool calls, where we selectively optimize common multi-turn tool call sequences to eliminate unnecessary turns.
This change specifically:
Modifies grep_search to return file content in the response if there are 3 or fewer results. This enables the agent to skip turns that would be spent grepping -> reading, particularly when doing unambiguous searches, like navigating to a class or function definition via grep "class Foo".
Modifies write_file to return a diff of the file content in the response, helping the agent to avoid the need to do verification reads after writing a file. This is particularly important given the fuzzy matching strategy we use when editing files. The little corrections the tool makes makes it so that what is written might not exactly match what the agent expected.
Analysis
Nets a stat-sig 10% drop in turn count in SWEBench.
Token consumption
Ranging between neutral and a mild improvement
Potential follow ups
Identify additional opportunities for pipelining of key workflows. Another that comes to mind is the write_file -> validate loop.
Details
Related Issues
How to Validate
Pre-Merge Checklist