-
Notifications
You must be signed in to change notification settings - Fork 0
Mirror: Use Mistral SDK in MistralHandler.streamFim (#5660) #14
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,12 +15,9 @@ import { ApiHandlerOptions } from "../../shared/api" | |
|
|
||
| import { convertToMistralMessages } from "../transform/mistral-format" | ||
| import { ApiStream } from "../transform/stream" | ||
| import { handleProviderError } from "./utils/error-handler" | ||
|
|
||
| import { BaseProvider } from "./base-provider" | ||
| import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index" | ||
| import { DEFAULT_HEADERS } from "./constants" // kilocode_change | ||
| import { streamSse } from "../../services/continuedev/core/fetch/stream" // kilocode_change | ||
| import type { CompletionUsage } from "./openrouter" // kilocode_change | ||
| import type { FimHandler } from "./kilocode/FimHandler" // kilocode_change | ||
|
|
||
|
|
@@ -258,56 +255,50 @@ export class MistralHandler extends BaseProvider implements SingleCompletionHand | |
| ): AsyncGenerator<string> { | ||
| const { id: model, maxTokens } = this.getModel() | ||
|
|
||
| // Get the base URL for the model | ||
| // copy pasted from constructor, be sure to keep in sync | ||
| const baseUrl = model.startsWith("codestral-") | ||
| ? this.options.mistralCodestralUrl || "https://codestral.mistral.ai" | ||
| : "https://api.mistral.ai" | ||
|
|
||
| const endpoint = new URL("v1/fim/completions", baseUrl) | ||
|
|
||
| const headers: Record<string, string> = { | ||
| ...DEFAULT_HEADERS, | ||
| "Content-Type": "application/json", | ||
| Accept: "application/json", | ||
| Authorization: `Bearer ${this.options.mistralApiKey}`, | ||
| } | ||
|
|
||
| // temperature: 0.2 is mentioned as a sane example in mistral's docs | ||
| const temperature = 0.2 | ||
| const requestMaxTokens = 256 | ||
|
|
||
| const response = await fetch(endpoint, { | ||
| method: "POST", | ||
| body: JSON.stringify({ | ||
| model, | ||
| prompt: prefix, | ||
| suffix, | ||
| max_tokens: Math.min(requestMaxTokens, maxTokens ?? requestMaxTokens), | ||
| temperature, | ||
| stream: true, | ||
| }), | ||
| headers, | ||
| }) | ||
| 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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. data.choices[0].delta unsafe access 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
|
||
| 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, | ||
| }) | ||
| } | ||
| } | ||
|
Comment on lines
+262
to
304
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Mistral fim tests outdated 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
|
||
|
|
||
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.
There's a potential
TypeErrorhere. Ifdata.choicesis an empty array,data.choices[0]will beundefined. Thendata.choices[0]?.deltawill also beundefined, and trying to access.contentonundefinedwill throw an error. You should add optional chaining for thedeltaproperty as well to prevent this. This pattern is already used in thecreateMessagemethod in this same file (line 120).