feat(go): add OpenAI plugin #4071
Conversation
Summary of ChangesHello @hugoaguirre, 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 introduces a new plugin for integrating OpenAI services into the Go Genkit framework. It establishes the necessary infrastructure to communicate with OpenAI's API, allowing Genkit applications to leverage OpenAI's large language models for chat and embedding functionalities. The plugin handles the complex mapping between Genkit's abstract AI interfaces and OpenAI's specific API request and response formats, including support for various message roles and tool interactions. 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. 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 a new OpenAI plugin for Go. The initial scaffolding is in place, including the plugin structure, dependency updates, and type conversion functions for requests. However, the core functionality for model generation is largely incomplete. Several key functions like DefineModel, generateStream, and generateComplete are stubbed out and return errors or nil. I've left specific comments on these areas that need to be implemented for the plugin to be functional. I've also pointed out a few other areas for improvement regarding configuration handling, logging, and security.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new plugin for OpenAI, leveraging their newer Responses API. The implementation is comprehensive, covering streaming, tool usage, and various content types. The code is well-structured with clear separation of concerns between generation logic, translation, and the main plugin definition. The addition of both live and unit tests is excellent and provides good coverage.
I've identified a few areas for improvement:
- A potential issue in stream handling where an empty response is returned instead of an error.
- A minor inconsistency in the configuration type used in
ResolveAction. - Some robustness improvements in the live tests to prevent panics and use of valid model names.
Overall, this is a solid contribution that significantly expands the capabilities of the platform.
| // prevent returning an error if stream does not provide [responses.ResponseCompletedEvent] | ||
| // user might already have received the chunks throughout the loop | ||
| return &ai.ModelResponse{ | ||
| Request: input, | ||
| Message: &ai.Message{Role: ai.RoleModel}, | ||
| }, nil |
There was a problem hiding this comment.
The function returns a default ai.ModelResponse if the stream ends without a ResponseCompletedEvent. This can hide potential issues, as a stream should ideally end with either a completion event or an error. Returning a default response might lead to silent failures or confusing behavior for the caller, who would receive an empty response without usage statistics.
Consider returning an error if finalResp is nil and stream.Err() is also nil to make failures more explicit.
| // prevent returning an error if stream does not provide [responses.ResponseCompletedEvent] | |
| // user might already have received the chunks throughout the loop | |
| return &ai.ModelResponse{ | |
| Request: input, | |
| Message: &ai.Message{Role: ai.RoleModel}, | |
| }, nil | |
| return nil, fmt.Errorf("stream finished without a completion event") |
There was a problem hiding this comment.
Note for the future: This is an unlikely scenario but, since we are talking about streaming. The user might already have received all the chunks, failing at this point might cause the request to fail. This edge-case is to cover up a possible scenario when responses.ResponseCompletedEvent is not sent
| // TODO: add image and video models | ||
| default: | ||
| supports = &internal.Multimodal | ||
| config = &openai.ChatCompletionNewParams{} |
There was a problem hiding this comment.
The config variable is being initialized with &openai.ChatCompletionNewParams{}, which corresponds to the legacy Chat Completions API. However, the rest of the plugin, including the newModel and generate functions, uses &responses.ResponseNewParams{} from the newer Responses API. This inconsistency will lead to an incorrect schema being exposed for the model configuration. To ensure consistency and correctness, this should be updated to use &responses.ResponseNewParams{}.
| config = &openai.ChatCompletionNewParams{} | |
| config = &responses.ResponseNewParams{} |
| switch config.(type) { | ||
| // TODO: add support for imagen and video | ||
| case *responses.ResponseNewParams: | ||
| return generate(ctx, client, name, input, cb) | ||
| default: | ||
| return generate(ctx, client, name, input, cb) | ||
| } |
There was a problem hiding this comment.
The switch statement here is redundant because both the case and default branches execute the same code. The // TODO suggests this is for future expansion, but in its current state, the switch can be removed to simplify the code. The generate function can be called directly.
// TODO: add support for imagen and video
return generate(ctx, client, name, input, cb)| ai.WithStreaming(func(ctx context.Context, c *ai.ModelResponseChunk) error { | ||
| out += c.Content[0].Text | ||
| return nil | ||
| }), |
There was a problem hiding this comment.
Accessing c.Content[0] directly can cause a panic if an empty chunk is received from the stream. It's safer to iterate over the Content slice and check the part type to handle this case gracefully, as done in other tests in this file.
ai.WithStreaming(func(ctx context.Context, c *ai.ModelResponseChunk) error {
for _, p := range c.Content {
if p.IsText() {
out += p.Text
}
}
return nil
}),| t.Fatal(err) | ||
| } | ||
| out := "" | ||
| m := oai.Model(g, "gpt-5") |
There was a problem hiding this comment.
The model gpt-5 is used here, but it is not a publicly available model. This will cause the test to fail when run against the public OpenAI API. Please use a valid, available model that supports the features being tested, such as gpt-4o.
m := oai.Model(g, "gpt-4o")| }) | ||
|
|
||
| t.Run("tools", func(t *testing.T) { | ||
| m := oai.Model(g, "gpt-5") |
There was a problem hiding this comment.
The model gpt-5 is used here, but it is not a publicly available model. This will cause the test to fail. Please use a valid, available model that supports tools, such as gpt-4o.
m := oai.Model(g, "gpt-4o")| ai.WithStreaming(func(ctx context.Context, c *ai.ModelResponseChunk) error { | ||
| out += c.Content[0].Text | ||
| return nil | ||
| }), |
There was a problem hiding this comment.
Accessing c.Content[0] directly can cause a panic if an empty chunk is received from the stream. It's safer to iterate over the Content slice to handle this case gracefully.
ai.WithStreaming(func(ctx context.Context, c *ai.ModelResponseChunk) error {
for _, p := range c.Content {
if p.IsText() {
out += p.Text
}
}
return nil
}),| }) | ||
|
|
||
| t.Run("streaming with thinking", func(t *testing.T) { | ||
| m := oai.Model(g, "gpt-5.2") |
There was a problem hiding this comment.
The model gpt-5.2 is used here, but it is not a publicly available model. This will cause the test to fail. Please use a valid, available model that supports reasoning, such as gpt-4o.
m := oai.Model(g, "gpt-4o")| ai.WithStreaming(func(ctx context.Context, c *ai.ModelResponseChunk) error { | ||
| out += c.Content[0].Text | ||
| return nil | ||
| }), |
There was a problem hiding this comment.
Accessing c.Content[0] directly can cause a panic if an empty chunk is received from the stream. It's safer to iterate over the Content slice to handle this case gracefully.
ai.WithStreaming(func(ctx context.Context, c *ai.ModelResponseChunk) error {
for _, p := range c.Content {
if p.IsText() {
out += p.Text
}
}
return nil
}),1 participant
Checklist (if applicable):