mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix(ollama): address review issues - double API calls, silent failure, dead code, unsafe casts
1. Remove double API calls in requestOllamaModels and refreshOllamaModels
- Changed flushModels(options, true) to flushModels(options, false) so the
cache is only invalidated, not re-fetched. discoverOllamaModelsWithSorting
is the single authoritative API call path.
2. Fix silent failure when no models found in requestOllamaModels
- Always send ollamaModels message to the webview regardless of totalCount,
so the UI can reflect an empty state instead of silently stalling.
- Applied same fix to refreshOllamaModels for consistency.
3. Remove dead getOllamaModels from modelCache.ts
- Replaced getOllamaModels with discoverOllamaModelsWithSorting in
fetchModelsFromProvider, eliminating the duplicate discovery code path.
- getOllamaModels is still exported for native-ollama.ts direct usage.
4. Replace unsafe as-any casts in ollama.ts
- Added OllamaInternalAxiosConfig interface extending InternalAxiosRequestConfig
with __retryCount and metadata fields.
- Replaced all (config as any) / (error.config as any) casts with the typed
interface in retry and logging interceptors.
- Changed raw API response cast from as-any to Partial<OllamaModelInfoResponse>.
- Changed Record<string, any> to ModelRecord in handler code.
This commit is contained in:
parent
609fbac98e
commit
9f4dc6e3de
4 changed files with 69 additions and 44 deletions
|
|
@ -21,7 +21,7 @@ import { getVercelAiGatewayModels } from "./vercel-ai-gateway"
|
|||
import { getRequestyModels } from "./requesty"
|
||||
import { getLiteLLMModels } from "./litellm"
|
||||
import { GetModelsOptions } from "../../../shared/api"
|
||||
import { getOllamaModels } from "./ollama"
|
||||
import { discoverOllamaModelsWithSorting } from "./ollama"
|
||||
import { getLMStudioModels } from "./lmstudio"
|
||||
import { getRooModels } from "./roo"
|
||||
|
||||
|
|
@ -72,14 +72,21 @@ async function fetchModelsFromProvider(options: GetModelsOptions): Promise<Model
|
|||
// Type safety ensures apiKey and baseUrl are always provided for LiteLLM.
|
||||
models = await getLiteLLMModels(options.apiKey, options.baseUrl)
|
||||
break
|
||||
case "ollama":
|
||||
models = await getOllamaModels(options.baseUrl, options.apiKey, {
|
||||
case "ollama": {
|
||||
// Use discoverOllamaModelsWithSorting as the single discovery path,
|
||||
// extracting only the tools-supporting models for the model cache.
|
||||
const discoveryResult = await discoverOllamaModelsWithSorting(options.baseUrl, options.apiKey, {
|
||||
modelDiscoveryTimeout: options.ollamaModelDiscoveryTimeout,
|
||||
maxRetries: options.ollamaMaxRetries,
|
||||
retryDelay: options.ollamaRetryDelay,
|
||||
enableLogging: options.ollamaEnableLogging,
|
||||
})
|
||||
models = {}
|
||||
for (const m of discoveryResult.modelsWithTools) {
|
||||
models[m.name] = m.modelInfo
|
||||
}
|
||||
break
|
||||
}
|
||||
case "lmstudio":
|
||||
models = await getLMStudioModels(options.baseUrl)
|
||||
break
|
||||
|
|
|
|||
|
|
@ -25,6 +25,15 @@ interface OllamaAxiosConfig {
|
|||
enableLogging?: boolean
|
||||
}
|
||||
|
||||
/**
|
||||
* Extended Axios request config with retry count and timing metadata.
|
||||
* Used by retry and logging interceptors to track state across requests.
|
||||
*/
|
||||
interface OllamaInternalAxiosConfig extends InternalAxiosRequestConfig {
|
||||
__retryCount?: number
|
||||
metadata?: { startTime: number }
|
||||
}
|
||||
|
||||
export function createOllamaAxiosInstance(config: OllamaAxiosConfig = {}): AxiosInstance {
|
||||
const {
|
||||
baseUrl = "http://localhost:11434",
|
||||
|
|
@ -64,7 +73,10 @@ function setupRetryInterceptor(instance: AxiosInstance, config: { retries: numbe
|
|||
instance.interceptors.response.use(
|
||||
(response) => response,
|
||||
async (error: AxiosError) => {
|
||||
const axiosConfig = error.config as any
|
||||
const axiosConfig = error.config as OllamaInternalAxiosConfig | undefined
|
||||
if (!axiosConfig) {
|
||||
return Promise.reject(error)
|
||||
}
|
||||
|
||||
axiosConfig.__retryCount = axiosConfig.__retryCount || 0
|
||||
if (axiosConfig.__retryCount >= config.retries) {
|
||||
|
|
@ -94,7 +106,7 @@ function setupRetryInterceptor(instance: AxiosInstance, config: { retries: numbe
|
|||
|
||||
function setupLoggingInterceptor(instance: AxiosInstance) {
|
||||
instance.interceptors.request.use((config: InternalAxiosRequestConfig) => {
|
||||
;(config as any).metadata = { startTime: Date.now() }
|
||||
;(config as OllamaInternalAxiosConfig).metadata = { startTime: Date.now() }
|
||||
console.debug("[Ollama] Request:", {
|
||||
method: config.method?.toUpperCase(),
|
||||
url: `${config.baseURL}${config.url}`,
|
||||
|
|
@ -106,7 +118,7 @@ function setupLoggingInterceptor(instance: AxiosInstance) {
|
|||
|
||||
instance.interceptors.response.use(
|
||||
(response: AxiosResponse) => {
|
||||
const startTime = (response.config as any).metadata?.startTime
|
||||
const startTime = (response.config as OllamaInternalAxiosConfig).metadata?.startTime
|
||||
const duration = startTime ? Date.now() - startTime : undefined
|
||||
console.debug("[Ollama] Response:", {
|
||||
status: response.status,
|
||||
|
|
@ -118,7 +130,7 @@ function setupLoggingInterceptor(instance: AxiosInstance) {
|
|||
return response
|
||||
},
|
||||
(error: AxiosError) => {
|
||||
const startTime = (error.config as any)?.metadata?.startTime
|
||||
const startTime = (error.config as OllamaInternalAxiosConfig | undefined)?.metadata?.startTime
|
||||
const duration = startTime ? Date.now() - startTime : undefined
|
||||
console.error("[Ollama] Error:", {
|
||||
code: error.code,
|
||||
|
|
@ -361,7 +373,7 @@ export async function discoverOllamaModelsWithSorting(
|
|||
const parsedDetail = OllamaModelInfoResponseSchema.safeParse(detailResponse.data)
|
||||
if (!parsedDetail.success) {
|
||||
// If validation fails, check if required fields exist in raw data
|
||||
const rawData = detailResponse.data as any
|
||||
const rawData = detailResponse.data as Partial<OllamaModelInfoResponse>
|
||||
if (!rawData?.details?.family || !rawData?.details?.parameter_size) {
|
||||
if (config?.enableLogging) {
|
||||
console.warn(`Invalid response for model ${ollamaModel.name}: missing required fields`)
|
||||
|
|
|
|||
|
|
@ -252,7 +252,10 @@ describe("webviewMessageHandler - requestOllamaModels", () => {
|
|||
type: "requestOllamaModels",
|
||||
})
|
||||
|
||||
expect(mockFlushModels).toHaveBeenCalledWith({ provider: "ollama", baseUrl: "http://localhost:1234" }, true)
|
||||
expect(mockFlushModels).toHaveBeenCalledWith(
|
||||
{ provider: "ollama", baseUrl: "http://localhost:1234", apiKey: undefined },
|
||||
false,
|
||||
)
|
||||
expect(mockDiscoverOllamaModels).toHaveBeenCalledWith(
|
||||
"http://localhost:1234",
|
||||
undefined,
|
||||
|
|
|
|||
|
|
@ -985,12 +985,16 @@ export const webviewMessageHandler = async (
|
|||
case "requestOllamaModels": {
|
||||
const { apiConfiguration: ollamaApiConfig } = await provider.getState()
|
||||
try {
|
||||
const ollamaOptions = {
|
||||
provider: "ollama" as const,
|
||||
baseUrl: ollamaApiConfig.ollamaBaseUrl,
|
||||
apiKey: ollamaApiConfig.ollamaApiKey,
|
||||
}
|
||||
await flushModels(ollamaOptions, true)
|
||||
// Invalidate stale cache without refetching (discoverOllamaModelsWithSorting
|
||||
// will perform the single authoritative API call below)
|
||||
await flushModels(
|
||||
{
|
||||
provider: "ollama" as const,
|
||||
baseUrl: ollamaApiConfig.ollamaBaseUrl,
|
||||
apiKey: ollamaApiConfig.ollamaApiKey,
|
||||
},
|
||||
false,
|
||||
)
|
||||
|
||||
const result = await discoverOllamaModelsWithSorting(
|
||||
ollamaApiConfig.ollamaBaseUrl,
|
||||
|
|
@ -1004,20 +1008,19 @@ export const webviewMessageHandler = async (
|
|||
)
|
||||
|
||||
// Convert modelsWithTools array to Record for compatibility
|
||||
const modelsWithToolsRecord: Record<string, any> = {}
|
||||
const modelsWithToolsRecord: ModelRecord = {}
|
||||
for (const model of result.modelsWithTools) {
|
||||
modelsWithToolsRecord[model.name] = model.modelInfo
|
||||
}
|
||||
|
||||
// Always send the models message if we have any results
|
||||
if (result.totalCount > 0) {
|
||||
provider.postMessageToWebview({
|
||||
type: "ollamaModels",
|
||||
ollamaModels: modelsWithToolsRecord,
|
||||
ollamaModelsWithTools: result.modelsWithTools,
|
||||
modelsWithoutTools: result.modelsWithoutTools,
|
||||
})
|
||||
}
|
||||
// Always send models to the webview, even when empty, so the UI
|
||||
// can reflect that no models were found instead of silently stalling.
|
||||
provider.postMessageToWebview({
|
||||
type: "ollamaModels",
|
||||
ollamaModels: modelsWithToolsRecord,
|
||||
ollamaModelsWithTools: result.modelsWithTools,
|
||||
modelsWithoutTools: result.modelsWithoutTools,
|
||||
})
|
||||
} catch (error) {
|
||||
console.debug("Ollama models fetch failed:", error)
|
||||
}
|
||||
|
|
@ -1065,17 +1068,16 @@ export const webviewMessageHandler = async (
|
|||
const apiKey = message.ollamaApiKey ?? ollamaApiConfig.ollamaApiKey
|
||||
|
||||
try {
|
||||
const ollamaOptions = {
|
||||
provider: "ollama" as const,
|
||||
baseUrl: baseUrl,
|
||||
apiKey: apiKey,
|
||||
ollamaModelDiscoveryTimeout: ollamaApiConfig.ollamaModelDiscoveryTimeout,
|
||||
ollamaMaxRetries: ollamaApiConfig.ollamaMaxRetries,
|
||||
ollamaRetryDelay: ollamaApiConfig.ollamaRetryDelay,
|
||||
ollamaEnableLogging: ollamaApiConfig.ollamaEnableLogging,
|
||||
}
|
||||
|
||||
await flushModels(ollamaOptions, true)
|
||||
// Invalidate stale cache without refetching (discoverOllamaModelsWithSorting
|
||||
// will perform the single authoritative API call below)
|
||||
await flushModels(
|
||||
{
|
||||
provider: "ollama" as const,
|
||||
baseUrl: baseUrl,
|
||||
apiKey: apiKey,
|
||||
},
|
||||
false,
|
||||
)
|
||||
|
||||
const result = await discoverOllamaModelsWithSorting(baseUrl, apiKey, {
|
||||
modelDiscoveryTimeout: ollamaApiConfig.ollamaModelDiscoveryTimeout,
|
||||
|
|
@ -1087,7 +1089,7 @@ export const webviewMessageHandler = async (
|
|||
const durationMs = Date.now() - startTime
|
||||
|
||||
// Convert modelsWithTools array to Record for compatibility
|
||||
const modelsWithToolsRecord: Record<string, any> = {}
|
||||
const modelsWithToolsRecord: ModelRecord = {}
|
||||
for (const model of result.modelsWithTools) {
|
||||
modelsWithToolsRecord[model.name] = model.modelInfo
|
||||
}
|
||||
|
|
@ -1104,14 +1106,15 @@ export const webviewMessageHandler = async (
|
|||
})
|
||||
}
|
||||
|
||||
// Always send the models message if we have any results
|
||||
// Always send models to the webview so the UI stays in sync
|
||||
provider.postMessageToWebview({
|
||||
type: "ollamaModels",
|
||||
ollamaModels: modelsWithToolsRecord,
|
||||
ollamaModelsWithTools: result.modelsWithTools,
|
||||
modelsWithoutTools: result.modelsWithoutTools,
|
||||
})
|
||||
|
||||
if (result.totalCount > 0) {
|
||||
provider.postMessageToWebview({
|
||||
type: "ollamaModels",
|
||||
ollamaModels: modelsWithToolsRecord,
|
||||
ollamaModelsWithTools: result.modelsWithTools,
|
||||
modelsWithoutTools: result.modelsWithoutTools,
|
||||
})
|
||||
provider.postMessageToWebview({
|
||||
type: "ollamaModelsRefreshResult",
|
||||
success: true,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue