fix(js): do not append prompt when resuming generation (#4652)#4657
fix(js): do not append prompt when resuming generation (#4652)#4657BenInvertase wants to merge 4 commits intofirebase:mainfrom
Conversation
Summary of ChangesHello @BenInvertase, 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 critical bug in the generation resumption logic. Previously, when attempting to resume an AI generation, the system would erroneously re-append the initial prompt, disrupting the message sequence and causing the resume operation to fail. The changes ensure that prompts are only added when appropriate, maintaining the integrity of the conversation history during resumed generations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where the prompt was being appended during a resumed generation, which caused precondition failures. The fix is applied consistently in both generate.ts and prompt.ts to handle raw generate calls and definePrompt cases. The addition of a unit test for toGenerateRequest effectively validates the change.
I've added one suggestion in generate.ts to improve the consistency of message processing, which I believe will make the implementation more robust.
| @@ -343,7 +343,7 @@ function messagesFromOptions(options: GenerateOptions): MessageData[] { | |||
| if (options.messages) { | |||
| messages.push(...options.messages); | |||
There was a problem hiding this comment.
There's an inconsistency in how messages are processed here compared to toGenerateRequest. This function pushes messages from options.messages directly, while toGenerateRequest uses Message.parseData(m) to normalize each message. The type definition for options.messages allows for lenient message formats (e.g., content as a string), which Message.parseData handles. Without parsing, this could lead to incorrectly formatted messages being sent to the model. To ensure consistency and robustness, it would be better to parse the messages here as well.
| messages.push(...options.messages); | |
| messages.push(...options.messages.map((m) => Message.parseData(m))); |
There was a problem hiding this comment.
this seems preexisting? not something introduced here.
| messages.push(...options.messages.map((m) => Message.parseData(m))); | ||
| } | ||
| if (options.prompt) { | ||
| if (options.prompt && !options.resume) { |
There was a problem hiding this comment.
(same as other comment in this file i think)
| messages.push(...options.messages); | ||
| } | ||
| if (options.prompt) { | ||
| if (options.prompt && !options.resume) { |
There was a problem hiding this comment.
only thing i'm thinking is if a user explicitly does e.g
ai.generate({
prompt: 'count to 10',
messages: [...],
resume: { respond: [...] },
})then the prompt is silently ignored, which might be confusing -
might be better to be defensive here and do:
if (options.prompt && options.resume) {
throw new GenkitError({
status: 'INVALID_ARGUMENT',
message: 'prompt is not supported when resume is set. The message history in messages is used instead.',
});
}b1a3415 to
79eff1f
Compare
79eff1f to
799bc7c
Compare
|
@cabljac Thanks for the review. I've updated based on your suggestion. |
Fixes #4652
When resuming an interrupted generation with
messagesandresume, the SDK was still appending the prompt as a new user message, so the last message was no longer the model message with a tool request and the resume precondition failed. This change skips appending the prompt whenresumeis set in bothgenerate.ts(for rawgeneratecalls) andprompt.ts(fordefinePrompt).