Mirror: Use Mistral SDK in MistralHandler.streamFim (#5660)#14
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @jeremylongshore, 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 significantly improves the integration with Mistral AI's FIM completion API by switching from a custom HTTP request implementation to using the official Mistral SDK. This change streamlines the codebase, enhances reliability, and standardizes error handling and data parsing for FIM streaming, ultimately leading to a more maintainable and robust provider. Highlights
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
|
Review Summary by QodoUse Mistral SDK in MistralHandler.streamFim
WalkthroughsDescription• Replace manual HTTP fetch with Mistral SDK client • Simplify FIM request construction and error handling • Update response parsing for SDK response format • Convert snake_case to camelCase for usage tokens Diagramflowchart LR
A["Manual fetch with headers"] -->|"Replace with"| B["Mistral SDK client"]
C["Custom error handling"] -->|"Simplify to"| D["SDK error handling"]
E["SSE stream parsing"] -->|"Update to"| F["SDK response iteration"]
G["snake_case tokens"] -->|"Convert to"| H["camelCase tokens"]
File Changes1. src/api/providers/mistral.ts
|
![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 refactors the streamFim method in MistralHandler to use the Mistral SDK, which is a significant improvement over the previous manual fetch implementation. The changes make the code cleaner, more maintainable, and add better error handling with telemetry. I've identified one critical issue that could lead to a runtime error and have provided a suggestion to fix it.
| for await (const ev of response) { | ||
| const data = ev.data | ||
|
|
||
| const content = data.choices[0]?.delta.content |
There was a problem hiding this comment.
There's a potential TypeError here. If data.choices is an empty array, data.choices[0] will be undefined. Then data.choices[0]?.delta will also be undefined, and trying to access .content on undefined will throw an error. You should add optional chaining for the delta property as well to prevent this. This pattern is already used in the createMessage method in this same file (line 120).
| const content = data.choices[0]?.delta.content | |
| const content = data.choices[0]?.delta?.content |
Code Review by Qodo
1. data.choices[0].delta unsafe access
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| for await (const ev of response) { | ||
| const data = ev.data | ||
|
|
||
| const content = data.choices[0]?.delta.content |
There was a problem hiding this comment.
1. data.choices[0].delta unsafe access 📘 Rule violation ⛯ Reliability
data.choices[0]?.delta.content can throw at runtime when choices is missing/empty or when delta is undefined, causing the FIM stream to crash mid-generation. This violates the requirement to handle null/empty edge cases at potential failure points.
Agent Prompt
## Issue description
`streamFim()` can throw when parsing streamed events because it assumes `data.choices` exists and that `delta` is always defined.
## Issue Context
The current code uses `data.choices[0]?.delta.content`, which can still throw if `data.choices` is undefined/null or if `delta` is undefined.
## Fix Focus Areas
- src/api/providers/mistral.ts[281-293]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const request = { | ||
| model, | ||
| temperature, | ||
| maxTokens: Math.min(requestMaxTokens, maxTokens ?? requestMaxTokens), | ||
| stream: true, | ||
| prompt: prefix, | ||
| suffix, | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text() | ||
| throw new Error(`FIM streaming failed: ${response.status} ${response.statusText} - ${errorText}`) | ||
| let response | ||
| try { | ||
| response = await this.client.fim.stream(request) | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error) | ||
| const apiError = new ApiProviderError(errorMessage, this.providerName, model, "streamFim") | ||
| TelemetryService.instance.captureException(apiError) | ||
| throw new Error(`Mistral FIM completion error: ${errorMessage}`) | ||
| } | ||
|
|
||
| for await (const data of streamSse(response)) { | ||
| const content = data.choices?.[0]?.delta?.content | ||
| if (content) { | ||
| for await (const ev of response) { | ||
| const data = ev.data | ||
|
|
||
| const content = data.choices[0]?.delta.content | ||
| if (typeof content === "string") { | ||
| yield content | ||
| } else if (content !== null && content !== undefined) { | ||
| for (const chunk of content) { | ||
| if (chunk.type === "text") { | ||
| yield chunk.text | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Call usage callback when available | ||
| // Note: Mistral FIM API returns usage in the final chunk with prompt_tokens and completion_tokens | ||
| if (data.usage && onUsage) { | ||
| onUsage({ | ||
| prompt_tokens: data.usage.prompt_tokens, | ||
| completion_tokens: data.usage.completion_tokens, | ||
| total_tokens: data.usage.total_tokens, | ||
| prompt_tokens: data.usage.promptTokens, | ||
| completion_tokens: data.usage.completionTokens, | ||
| total_tokens: data.usage.totalTokens, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Mistral fim tests outdated 📘 Rule violation ⛯ Reliability
streamFim() switched from fetch/streamSse to this.client.fim.stream(), but existing tests still mock global.fetch and streamSse and assert the old error message. This is a behavior change without corresponding test updates, risking failing CI and regressions.
Agent Prompt
## Issue description
The Mistral FIM tests mock `fetch` and `streamSse`, but `streamFim()` now uses the Mistral SDK client (`this.client.fim.stream`). The tests likely fail and no longer validate the new behavior.
## Issue Context
`streamFim()` now iterates `for await (const ev of response)` where each event has `ev.data`.
## Fix Focus Areas
- src/api/providers/mistral.ts[262-304]
- src/api/providers/__tests__/mistral-fim.spec.ts[7-191]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review Summary
Checklist
Findings1. Existing tests broken by refactor (blocking)The test file 4 failures:
Fix needed: Update 2. Missing changeset (blocking)This changes the FIM streaming implementation for a provider. Needs a patch changeset for 3. Upstream CI has test-extension failures (non-blocking, observation)Upstream CI shows 2 failures ( Local VerificationWe merged this PR on our fork and ran the full test suite. Regression (existing tests)
Why tests fail: The PR replaces Pre-existing failures (not from this PR)
Code AnalysisWhat the refactor doesReplaces a manual HTTP implementation with the Mistral SDK: Before (manual): const endpoint = new URL("v1/fim/completions", baseUrl)
const headers = { ...DEFAULT_HEADERS, Authorization: `Bearer ${apiKey}` }
const response = await fetch(endpoint, { method: "POST", body: JSON.stringify({...}), headers })
for await (const data of streamSse(response)) { ... }After (SDK): const response = await this.client.fim.stream({ model, prompt: prefix, suffix, ... })
for await (const ev of response) { ... }The SDK handles:
Removed importsThree imports removed, all replaced by SDK functionality:
Content type handlingThe PR adds proper handling for content that could be either a string or an array of content chunks: if (typeof content === "string") {
yield content
} else if (content !== null && content !== undefined) {
for (const chunk of content) {
if (chunk.type === "text") { yield chunk.text }
}
}This is more robust than the original CI Status
VerdictREQUEST_CHANGES — The refactor itself is clean and correct. Using the SDK instead of manual HTTP is the right architectural direction — every other method in |
Review Journal: kilocode Kilo-Org#5660
SummaryReplaces the manual HTTP fetch implementation in What ChangedOne file, 30 additions, 39 deletions. The diff is a clean swap: Removed: Manual endpoint URL construction, custom headers with Added: Three imports removed ( AnalysisThe PR originated from debugging a The content type handling is more robust than the original: // Original: assumes content is always a string
if (content) { yield content }
// New: handles string and content chunk array
if (typeof content === "string") { yield content }
else if (content !== null) { for (const chunk of content) { ... } }The usage token fields changed from snake_case ( VerificationRegression (existing tests)
Test failure root causeThe test file This is a test maintenance gap, not a code quality issue. The fix is to mock Bot Review Synthesis
Lessons Learned
Review #21 of 75 | Multi-AI analysis | Methodology |
2 participants
Mirror of Kilo-Org#5660
This PR mirrors the upstream change for multi-AI review analysis.
Bot Review Tracker