Skip to content

fix(go): compat_oai plugin returns incomplete history when streaming - #5239

Merged
apascal07 merged 1 commit into
genkit-ai:mainfrom
MikeRez0:go-compat_oai-stream-history
Aug 1, 2026
Merged

fix(go): compat_oai plugin returns incomplete history when streaming#5239
apascal07 merged 1 commit into
genkit-ai:mainfrom
MikeRez0:go-compat_oai-stream-history

Conversation

@MikeRez0

@MikeRez0 MikeRez0 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Changes fix this issue
#4683

Checklist (if applicable):

@google-cla

google-cla Bot commented May 6, 2026

Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the go label May 6, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the generateStream and convertChatCompletionToModelResponse functions in go/plugins/compat_oai/generate.go to accept and utilize the ai.ModelRequest object, ensuring the request context is preserved in the model response. A review comment identifies that the signature change for convertChatCompletionToModelResponse breaks the generateComplete function, which was not updated in this PR. The feedback includes a code suggestion to fix the compilation error and improve error handling by wrapping errors with contextual information.

Comment thread go/plugins/compat_oai/generate.go Outdated
@MichaelDoyle
MichaelDoyle requested a review from apascal07 May 6, 2026 15:15
@MichaelDoyle

Copy link
Copy Markdown
Contributor

Thanks @MikeRez0! Can you get the CLA taken care of? In the mean time we'll get the review done.

@MikeRez0

MikeRez0 commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

Already signed

@MikeRez0

Copy link
Copy Markdown
Contributor Author

@apascal07 Hi, Alex! Can you review changes, please? I want to use stream mode in my project

@apascal07

Copy link
Copy Markdown
Collaborator

A few requested changes:

  1. Keep convertChatCompletionToModelResponse pure — instead of threading req through it, set resp.Request = req in generateStream after the call, mirroring what generateComplete already does. That avoids changing the converter's signature and keeps a single pattern for both paths.
  2. Delete the commented-out // Request: &ai.ModelRequest{}, line rather than leaving it in.
  3. With the above, the existing resp.Request = req in generateComplete stays correct. As written, it's now redundant with the assignment inside the converter.
  4. Add a regression test for the streaming path that asserts resp.Request.Messages (or resp.History()) preserves the input messages — TestGenerator_Stream currently discards the response with _, so this bug could silently return.

@apascal07 apascal07 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments above.

@apascal07
apascal07 requested a review from cabljac June 9, 2026 14:10
generateStream never set ModelResponse.Request, and
convertChatCompletionToModelResponse defaulted it to an empty
&ai.ModelRequest{}. Streaming responses therefore reported an empty
message list, so ModelResponse.History() returned only the model reply
and dropped the entire input conversation. The non-streaming path was
unaffected because generateComplete already assigned resp.Request.

Thread the originating request through generateStream and assign it
after conversion, mirroring generateComplete. Drop the misleading
default from the converter so it stays a pure conversion and callers
own the request.

Fixes genkit-ai#4683

Co-authored-by: Alex Pascal <apascal07@gmail.com>
@apascal07
apascal07 force-pushed the go-compat_oai-stream-history branch from 5a584be to 66cb2ac Compare August 1, 2026 01:56
@apascal07
apascal07 enabled auto-merge (squash) August 1, 2026 01:57
@apascal07
apascal07 merged commit 5dadf24 into genkit-ai:main Aug 1, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants