-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix: bracket stripping in gemini responses #8280
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
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.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="core/llm/llms/Gemini.ts">
<violation number="1" location="core/llm/llms/Gemini.ts:418">
streamChatGemini now yields raw SSE payloads instead of ChatMessage objects, so downstream rendering breaks when accessing message.role/content.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
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.
screen recording seems promising, nice!
- Wondering how this is working when it removes the translation of gemini chunks into chat chunks?
- Let's keep processGeminiResponse/implement the fix for processGeminiResponse since it is used by VertexAI
- Let's also implement for openai adapters!
Ah, missed it. It works for normal chat messages but not for tool calls. Fixed that!
implemented! |
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.
Something still feels off here, I think streamSse already does the buffer splitting which makes e.g. this line redundant:
const parts = buffer.split("\n,");
Thinking you should be able to remove streamResponse entirely from openai adapter and just use streamSse?
I think Vertex LLM class usage of processGeminiResponse would also require using streamSse. Alternatively, you could change processGeminiResponse to take response directly and use streamSse on it
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.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="core/llm/llms/Gemini.ts">
<violation number="1" location="core/llm/llms/Gemini.ts:409">
`streamSse(response)` yields parsed SSE objects, not raw string chunks, so feeding it into `processGeminiResponse` (which expects strings and re-parses JSON) will immediately corrupt the buffer (`[object Object]`) and break Gemini streaming. Please pass the original response stream or update `processGeminiResponse` to accept objects.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
core/llm/llms/Gemini.ts
Outdated
| )) { | ||
| yield message; | ||
|
|
||
| for await (const chunk of this.processGeminiResponse(streamSse(response))) { |
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.
streamSse(response) yields parsed SSE objects, not raw string chunks, so feeding it into processGeminiResponse (which expects strings and re-parses JSON) will immediately corrupt the buffer ([object Object]) and break Gemini streaming. Please pass the original response stream or update processGeminiResponse to accept objects.
Prompt for AI agents
Address the following comment on core/llm/llms/Gemini.ts at line 409:
<comment>`streamSse(response)` yields parsed SSE objects, not raw string chunks, so feeding it into `processGeminiResponse` (which expects strings and re-parses JSON) will immediately corrupt the buffer (`[object Object]`) and break Gemini streaming. Please pass the original response stream or update `processGeminiResponse` to accept objects.</comment>
<file context>
@@ -415,7 +406,7 @@ class Gemini extends BaseLLM {
});
- for await (const chunk of streamSse(response)) {
+ for await (const chunk of this.processGeminiResponse(streamSse(response))) {
yield chunk;
}
</file context>
it is needed for parsing the gemini chat response
implemented! |
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.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="core/llm/llms/VertexAI.ts">
<violation number="1" location="core/llm/llms/VertexAI.ts:290">
processGeminiResponse expects raw string chunks, but streamSse yields parsed objects. Passing streamSse here causes the handler to concatenate "[object Object]" and fail parsing, breaking Gemini streaming.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
core/llm/llms/VertexAI.ts
Outdated
| signal, | ||
| }); | ||
| yield* this.geminiInstance.processGeminiResponse(streamResponse(response)); | ||
| yield* this.geminiInstance.processGeminiResponse(streamSse(response)); |
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.
processGeminiResponse expects raw string chunks, but streamSse yields parsed objects. Passing streamSse here causes the handler to concatenate "[object Object]" and fail parsing, breaking Gemini streaming.
Prompt for AI agents
Address the following comment on core/llm/llms/VertexAI.ts at line 290:
<comment>processGeminiResponse expects raw string chunks, but streamSse yields parsed objects. Passing streamSse here causes the handler to concatenate "[object Object]" and fail parsing, breaking Gemini streaming.</comment>
<file context>
@@ -287,7 +287,7 @@ class VertexAI extends BaseLLM {
signal,
});
- yield* this.geminiInstance.processGeminiResponse(streamResponse(response));
+ yield* this.geminiInstance.processGeminiResponse(streamSse(response));
}
</file context>
What I meant is I think streamSse already splits chunks by new lines so the split at "\n," is extraneous continue/packages/fetch/src/stream.ts Lines 124 to 150 in 579bcbf
|
core/llm/llms/Gemini.ts
Outdated
| stream: AsyncIterable<string>, | ||
| ): AsyncGenerator<ChatMessage> { | ||
| let buffer = ""; | ||
| for await (const chunk of stream) { |
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.
why don't we just streamSse(stream) directly in processGeminiReponse?
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.
implemented!
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.
Nice!
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.27.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.30.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Use streamSse instead of processGeminiResponse when receiving responses from Gemini streamChat .
resolves CON-4266
closes #2651
AI Code Review
@continue-reviewChecklist
Screen recording or screenshot
before.mp4
after.mp4
Tests
[ What tests were added or updated to ensure the changes work as expected? ]
Summary by cubic
Switched Gemini streamChat to use streamSse for proper SSE handling and direct chunk streaming, fixing broken/laggy responses and simplifying the streaming path.