feat(api): add chat handler and integrate mcp/openai clients #277
Conversation
Signed-off-by: kasanatte <[email protected]>
karmada-bot
added
the
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @kasanatte, 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 establishes the foundational API for an AI Assistant within the dashboard, exposing a new 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
|
karmada-bot
added
the
size/XL
![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 chat API with streaming and tool-calling capabilities by integrating with OpenAI and a new MCP client. It also adds a legacy, simpler assistant endpoint. The overall structure is good, but there are several issues with error handling in the streaming logic that could lead to silent failures and a confusing experience for the client. Additionally, there's a bug in how streamed tool calls are accumulated, and some opportunities for code deduplication. My review provides specific suggestions to improve robustness and maintainability.
| if err != nil { | ||
| if errors.Is(err, io.EOF) { | ||
| break | ||
| } | ||
| klog.Errorf("Error receiving stream response: %v", err) | ||
| break | ||
| } |
There was a problem hiding this comment.
When resp.Recv() returns an error that is not io.EOF, the error is logged and the loop is broken, but the error is not propagated. The calling function handleChatCompletion then proceeds to process tool calls and send a completion signal, which is incorrect and misleading for the client.
To fix this, streamResponse should return the error. The caller, handleChatCompletion, should then check for this error, send an appropriate SSE error event to the client, and terminate the request instead of proceeding.
| if err := json.Unmarshal([]byte(toolCall.Function.Arguments), &args); err != nil { | ||
| klog.Errorf("Failed to parse tool arguments: %v, args: %s", err, toolCall.Function.Arguments) | ||
| return openai.ChatCompletionMessage{} | ||
| } |
There was a problem hiding this comment.
If parsing the tool arguments fails, an empty openai.ChatCompletionMessage is returned. This causes the tool call to be silently ignored by processToolCalls, and the LLM will not receive a result for its requested action. This can lead to confused or incomplete responses from the AI.
The error should be reported back to the LLM as the result of the tool call.
if err := json.Unmarshal([]byte(toolCall.Function.Arguments), &args); err != nil {
klog.Errorf("Failed to parse tool arguments: %v, args: %s", err, toolCall.Function.Arguments)
result := fmt.Sprintf("Error parsing arguments for tool %s: %v", toolCall.Function.Name, err)
return openai.ChatCompletionMessage{
Role: openai.ChatMessageRoleTool,
Content: result,
Name: toolCall.Function.Name,
ToolCallID: toolCall.ID,
}
}| if err != nil { | ||
| klog.Errorf("Failed to create final chat completion stream: %v", err) | ||
| return |
There was a problem hiding this comment.
If creating the final chat completion stream fails, the error is logged and the function returns silently. The main handler then proceeds to send a completion signal, misleading the client into thinking the request was successful. The error should be propagated to the client via an SSE event.
if err != nil {
klog.Errorf("Failed to create final chat completion stream: %v", err)
// It's important to notify the client about the error.
msg := ChatResponse{Type: "error", Content: "Failed to get final response from LLM"}
if data, err := json.Marshal(msg); err == nil {
fmt.Fprintf(c.Writer, "data: %s\n\n", data)
c.Writer.Flush()
}
return
}| toolCallBuffer[index].Type = toolCall.Type | ||
| } | ||
| if toolCall.Function.Name != "" { | ||
| toolCallBuffer[index].Function.Name = toolCall.Function.Name |
There was a problem hiding this comment.
The function name from a streamed tool call is being assigned (=) rather than appended (+=). According to the OpenAI API, the function name can also be delivered in chunks, similar to arguments. This implementation will fail to correctly assemble the function name if it is chunked, leading to failed tool calls.
toolCallBuffer[index].Function.Name += toolCall.Function.Name| if err := session.run(); err != nil { | ||
| klog.Errorf("Answering session run failed: %v", err) | ||
| } |
There was a problem hiding this comment.
Errors that occur within session.run() during the SSE stream (e.g., failure to create the stream or receive data) are logged but not communicated to the client. Since the HTTP headers have already been sent, the client connection will simply close without any indication of what went wrong. It's better to send an error event over the SSE stream to inform the client.
if err := session.run(); err != nil {
klog.Errorf("Answering session run failed: %v", err)
session.sendStreamEvent("error", map[string]string{"message": err.Error()})
}| msg := ChatResponse{ | ||
| Type: "content", | ||
| Content: choice.Delta.Content, | ||
| } | ||
| if data, err := json.Marshal(msg); err == nil { | ||
| fmt.Fprintf(c.Writer, "data: %s\n\n", data) | ||
| c.Writer.Flush() | ||
| } |
There was a problem hiding this comment.
The logic for marshaling a ChatResponse and sending it over the SSE stream is repeated in multiple places (here, processToolCalls, executeTool, etc.). This code duplication makes the code harder to maintain.
Consider extracting this logic into a dedicated helper function, for example sendEvent(c *gin.Context, response ChatResponse), and using it throughout the file.
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces the main chat handler and exposes the /api/v1/chat API endpoint. This handler serves as the core backend logic for the AI Assistant, orchestrating the MCP and OpenAI clients to:
Which issue(s) this PR fixes:
Fixes #273
Special notes for your reviewer:
It depends on the changes from PR #275 & PR #276 。
Does this PR introduce a user-facing change?: