fix(core): guard formatTruncatedToolOutput against non-positive maxCh… - #29004
fix(core): guard formatTruncatedToolOutput against non-positive maxCh…#29004Eswar809 wants to merge 1 commit into
Conversation
|
📊 PR Size: size/S
|
Summary of ChangesHello, 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 addresses a bug in the Highlights
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 the 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 counterproductive. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the formatTruncatedToolOutput utility to return the content unchanged when maxChars is zero or negative, along with adding corresponding unit tests. The reviewer noted that returning the full, untruncated content when maxChars is non-positive defeats the purpose of the utility and could lead to memory or context window issues. They suggested instead performing a full truncation (omitting all characters) when maxChars is less than or equal to 0.
| maxChars: number, | ||
| ): string { | ||
| if (contentStr.length <= maxChars) return contentStr; | ||
| if (maxChars <= 0 || contentStr.length <= maxChars) return contentStr; |
There was a problem hiding this comment.
When maxChars is 0 or negative, returning the entire contentStr unchanged defeats the purpose of the truncation utility. If the tool output is extremely large (e.g., several megabytes), returning it untruncated can cause downstream failures such as context window overflow or out-of-memory errors.
Instead of returning the full content, we should treat any non-positive maxChars as a budget of 0 and perform a full truncation (showing 0 characters of head/tail and omitting the rest). This safely truncates the output while avoiding the slice(-0) bug.
Please also update the corresponding unit tests in packages/core/src/utils/fileUtils.test.ts to assert that the output is correctly truncated (showing 0 characters and omitting the rest) rather than returned unchanged.
| if (maxChars <= 0 || contentStr.length <= maxChars) return contentStr; | |
| if (contentStr.length <= maxChars) return contentStr; | |
| if (maxChars <= 0) { | |
| return `Output too large. Showing first 0 and last 0 characters. For full output see: ${outputFile}\n\n\n... [${contentStr.length.toLocaleString()} characters omitted] ...\n\n`; | |
| } |
References
- When implementing truncation logic, a simpler approach is preferred over a more complex one if the potential inaccuracy is trivial compared to the overall buffer size.
5d541cb to
fdbbd26
Compare
Summary
Guards
formatTruncatedToolOutputagainst non-positivemaxCharsvalues, such as0or negative budgets, to prevent truncated output from unexpectedly inflating to roughly twice the original size due to negative slice offsets.Problem
When
formatTruncatedToolOutputwas called withmaxChars <= 0, the truncation logic behaved incorrectly:headChars = Math.floor(maxChars * 0.2)could evaluate to a negative number.slice(0, headChars)to return almost the full string instead of a small prefix.tailChars = maxChars - headCharscould evaluate to0or a negative value.slice(-tailChars), such asslice(-0), to return the entire string.headandtailsegments were then concatenated inside the truncation wrapper.Fix
Added an early guard in
formatTruncatedToolOutputto handle invalid or unnecessary truncation cases:This ensures that when there is no valid truncation budget, or the content already fits within the allowed budget, the original string is returned unchanged.
Related Issues
Fixes #28620
Validation
Run the
fileUtilsunit tests:npm test -w @google/gemini-cli-core -- src/utils/fileUtils.test.tsVerify that all test cases pass, including the new cases for:
maxChars = 0maxChars = -1000Pre-Merge Checklist