feat: Implement script output summarization and refine image routing#14097
feat: Implement script output summarization and refine image routing#14097mgajda wants to merge 5 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @mgajda, 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 model routing capabilities by introducing two new strategies: one for summarizing lengthy script outputs and another for intelligently handling image-related requests. The script output summarization leverages the "flash-lite" model for efficiency, only summarizing outputs above a certain length and preserving critical excerpts. The image routing dynamically selects between "flash-lite" and "pro" image models based on configuration. These changes aim to optimize model usage, improve performance, and expand the CLI's utility, alongside minor code refinements and new feature documentation. 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 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
|
![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 significant new features for script output summarization and image model routing. I've identified several critical issues that need to be addressed. There's a duplicated strategy in the model router, a critical bug in the shell tool that discards command output, and the new summarization strategy is currently ineffective as it doesn't pass the summarized content to the next stage. Additionally, there are bugs in the new ImageStrategy and its tests. My review includes detailed suggestions to fix these issues.
| new FallbackStrategy(), | ||
| new OverrideStrategy(), | ||
| new ScriptOutputSummarizationStrategy(), | ||
| new ScriptOutputSummarizationStrategy(), |
There was a problem hiding this comment.
The ScriptOutputSummarizationStrategy is instantiated twice. This will cause the strategy to be executed twice for each routing decision, which is inefficient and may lead to unintended side effects. Please remove the duplicate instance.
| } as RoutingContext; | ||
|
|
||
| // Mock config to disable preview features | ||
| vi.spy10('getPreviewFeatures').mockReturnValue(false); |
There was a problem hiding this comment.
There is a typo vi.spy10 which should be vi.spyOn. This will cause the test to fail. Additionally, spyOn requires the object to be spied on as the first argument.
| vi.spy10('getPreviewFeatures').mockReturnValue(false); | |
| vi.spyOn(mockConfig, 'getPreviewFeatures').mockReturnValue(false); |
| const summaryPrompt = `Summarize the following script output concisely:\n\n\n${actualOutput}\n\n\n\ | ||
| ${actualOutput} | ||
| `; |
There was a problem hiding this comment.
The actualOutput is duplicated in the summaryPrompt. This makes the prompt unnecessarily long and may confuse the model, leading to poor summaries. Please remove the duplicate.
const summaryPrompt = `Summarize the following script output concisely:\n\n\n${actualOutput}\n`;| const summarizedOutput = summaryResult.text; | ||
|
|
||
| // Return a decision to use the flash-lite model for the summarized output. | ||
| // This implies the next stage of routing or processing will use this summarized output. | ||
| // A more advanced mechanism might be needed to *replace* the context for subsequent strategies. | ||
| return { | ||
| model: DEFAULT_GEMINI_FLASH_MODEL, // Using the default flash-lite alias | ||
| metadata: { | ||
| source: this.name, | ||
| latencyMs: 0, // Placeholder, actual measurement needed | ||
| reasoning: `Summarized script output using ${DEFAULT_GEMINI_FLASH_MODEL}.`, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This strategy is currently ineffective. It generates a summary of the script output but does not replace the original long output in the context. As a result, subsequent strategies and the main model will still process the original, unsummarized content, defeating the purpose of this strategy.
The strategy should modify the shellOutputPart.text with the summarizedOutput and then return null to allow other routing strategies to determine the model for the next turn based on the new, summarized content.
const summarizedOutput = summaryResult.text;
// Replace the original long output with the summary for subsequent processing.
shellOutputPart.text = summarizedOutput;
// Return null to allow other strategies to route based on the summarized content.
return null;|
|
||
| return { | ||
| llmContent, | ||
| llmContent: `[SHELL_OUTPUT]\nCommand: ${this.params.command}\nDirectory: ${this.params.dir_path || '\'(root)\''}`, // Corrected llmContent with prefix |
There was a problem hiding this comment.
The llmContent is being overwritten with a string that only contains the command and directory, completely omitting the actual output, errors, and exit code of the shell command. This is a critical bug, as the model will not receive the result of the execution. The [SHELL_OUTPUT] prefix should be prepended to the existing llmContent variable, which already contains the full execution details.
llmContent: `[SHELL_OUTPUT]\n${llmContent}`,| const preferredGeneralModel = config.getModel(); | ||
| // Resolve the general model preference to its concrete name | ||
| const resolvedGeneralModel = resolveModel(preferredGeneralModel, config.getPreviewFeatures()); | ||
|
|
||
| let modelToUse: string; | ||
| // Check if the preferred general model is 'flash-lite' based on its name | ||
| if (resolvedGeneralModel.includes('flash-lite')) { | ||
| // Route to the flash-lite image model | ||
| modelToUse = GEMINI_MODEL_ALIAS_FLASH_LITE_IMAGE; | ||
| } else { | ||
| // Fallback to pro image model or default pro model based on preview flag | ||
| modelToUse = config.getPreviewFeatures() ? 'gemini-2.5-pro-image-preview' : DEFAULT_GEMINI_MODEL; | ||
| } |
There was a problem hiding this comment.
The logic for selecting an image model is inconsistent and has a bug. When the preferred model is 'flash-lite', it routes to a preview model regardless of whether preview features are enabled. This could lead to using a preview model when it's not intended. The logic is also inconsistent as it returns a concrete model name for the 'pro' preference but an alias for 'flash-lite'.
I suggest refactoring to consistently handle the preview flag and return concrete model names for both cases to improve clarity and correctness.
| const preferredGeneralModel = config.getModel(); | |
| // Resolve the general model preference to its concrete name | |
| const resolvedGeneralModel = resolveModel(preferredGeneralModel, config.getPreviewFeatures()); | |
| let modelToUse: string; | |
| // Check if the preferred general model is 'flash-lite' based on its name | |
| if (resolvedGeneralModel.includes('flash-lite')) { | |
| // Route to the flash-lite image model | |
| modelToUse = GEMINI_MODEL_ALIAS_FLASH_LITE_IMAGE; | |
| } else { | |
| // Fallback to pro image model or default pro model based on preview flag | |
| modelToUse = config.getPreviewFeatures() ? 'gemini-2.5-pro-image-preview' : DEFAULT_GEMINI_MODEL; | |
| } | |
| const previewEnabled = config.getPreviewFeatures(); | |
| let modelToUse: string; | |
| if (!previewEnabled) { | |
| modelToUse = DEFAULT_GEMINI_MODEL; | |
| } else { | |
| const preferredGeneralModel = config.getModel(); | |
| const resolvedGeneralModel = resolveModel(preferredGeneralModel, previewEnabled); | |
| if (resolvedGeneralModel.includes('flash-lite')) { | |
| modelToUse = 'gemini-2.5-flash-lite-image-preview'; | |
| } else { | |
| modelToUse = 'gemini-2.5-pro-image-preview'; | |
| } | |
| } |
|
The linked issues are not right, can you please use the pull request template and stop overriding it. It is also very unclear why this pull request is needed maybe you linked wrong issues and you have other issues you meant? |
|
Pull requests in general should solve one single contained problem or feature. They should be as small as possible to limit separation of concerns, allow for easy reviews, and be able to be cleanly reverted is something introduces a bug. This PR does two things by the title, "script output summarization and refine image routing". These are unrelated features or bugs from what I can quickly see. Thus they should be two separate PRs and not one. I would also prefer discussion on a GitHub issue prior to PRs. As there is no such thing as a flash lite image model and I am not sure what ImageStrategy is trying to do, our models like Gemini 3 and 2.5 Pro etc are multi-modal, they can take in image data as is... are we trying to create images with the change? Again this is why we require GitHub issues prior to creating PRs. |
|
Closing this for now, please have proper GitHub issues linked and separation of concerns for clean pull requests that we can efficiently review. Appreciate it kindly. |
|
Thanks for feedback. Will correct my mistake |
2 participants
This PR addresses the following:
Addresses issues: #1, #2 (assuming these are the issues I just created).