From b447daf960c91a561ccec66bc8ba8515b10a6716 Mon Sep 17 00:00:00 2001 From: Eugene Pyvovarov Date: Tue, 26 Aug 2025 12:20:04 +0200 Subject: [PATCH 1/3] fixes model selection for Ollama provider Signed-off-by: Eugene Pyvovarov --- .../src/routes/config_management.rs | 21 +++++++++ crates/goose/src/providers/ollama.rs | 45 +++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/crates/goose-server/src/routes/config_management.rs b/crates/goose-server/src/routes/config_management.rs index 0224cc79565d..e3d5511a82df 100644 --- a/crates/goose-server/src/routes/config_management.rs +++ b/crates/goose-server/src/routes/config_management.rs @@ -938,4 +938,25 @@ mod tests { std::env::remove_var("OPENAI_API_KEY"); } + + #[tokio::test] + async fn test_get_provider_models_ollama_not_running() { + // Test when Ollama is not running (typical scenario in CI/testing) + let test_state = create_test_state().await; + let mut headers = HeaderMap::new(); + headers.insert("X-Secret-Key", "test".parse().unwrap()); + + let result = + get_provider_models(State(test_state), headers, Path("ollama".to_string())).await; + + // Should succeed but return empty array when Ollama is not accessible + // The implementation gracefully handles connection failures and returns Ok(None) + // which translates to an empty Vec in the API response + assert!(result.is_ok(), "Expected successful response from Ollama provider even when not running"); + let models = result.unwrap().0; + + // Should return empty list when Ollama is not accessible + // (implementation returns Ok(None) which becomes empty Vec in response) + assert_eq!(models.len(), 0, "Expected empty models list when Ollama is not running"); + } } diff --git a/crates/goose/src/providers/ollama.rs b/crates/goose/src/providers/ollama.rs index 1479a847fe73..442dfe0aa6fe 100644 --- a/crates/goose/src/providers/ollama.rs +++ b/crates/goose/src/providers/ollama.rs @@ -228,6 +228,51 @@ impl Provider for OllamaProvider { fn supports_streaming(&self) -> bool { self.supports_streaming } + + async fn fetch_supported_models(&self) -> Result>, ProviderError> { + // Ollama uses /api/tags endpoint to list installed models + let response = match self.api_client.response_get("api/tags").await { + Ok(resp) => resp, + Err(e) => { + tracing::warn!("Failed to fetch models from Ollama: {}", e); + return Ok(None); + } + }; + + // Parse JSON response + let json: serde_json::Value = match response.json().await { + Ok(json) => json, + Err(e) => { + tracing::warn!("Failed to parse Ollama models response: {}", e); + return Ok(None); + } + }; + + // Extract model names from the response + // Ollama returns: { "models": [{"name": "model1", "size": ..., "digest": ..., "modified_at": ...}, ...] } + let models = json + .get("models") + .and_then(|v| v.as_array()) + .map(|arr| { + let mut model_names: Vec = arr.iter() + .filter_map(|m| m.get("name").and_then(|v| v.as_str())) + .map(|s| s.to_string()) + .collect(); + model_names.sort(); + model_names + }); + + match models { + Some(model_list) if !model_list.is_empty() => { + tracing::info!("Found {} models in Ollama", model_list.len()); + Ok(Some(model_list)) + } + _ => { + tracing::info!("No models found in Ollama or unable to parse response"); + Ok(None) + } + } + } } impl OllamaProvider { From 665bbf9dacf2a4698eb2045e7532669a35ffeb1e Mon Sep 17 00:00:00 2001 From: Eugene Pyvovarov Date: Tue, 26 Aug 2025 12:26:10 +0200 Subject: [PATCH 2/3] add dynamic ollama models to lead/worker model selection Signed-off-by: Eugene Pyvovarov --- .../subcomponents/LeadWorkerSettings.tsx | 51 ++++++++++++++----- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx b/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx index d2e255a76291..775c34f24786 100644 --- a/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx +++ b/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx @@ -13,7 +13,7 @@ interface LeadWorkerSettingsProps { } export function LeadWorkerSettings({ isOpen, onClose }: LeadWorkerSettingsProps) { - const { read, upsert, getProviders, remove } = useConfig(); + const { read, upsert, getProviders, getProviderModels, remove } = useConfig(); const { currentModel } = useModelAndProvider(); const [leadModel, setLeadModel] = useState(''); const [workerModel, setWorkerModel] = useState(''); @@ -96,21 +96,48 @@ export function LeadWorkerSettings({ isOpen, onClose }: LeadWorkerSettingsProps) }); }); } else { - // Fallback to provider-based models + // Fetch models with dynamic discovery and static fallback const providers = await getProviders(false); const activeProviders = providers.filter((p) => p.is_configured); - activeProviders.forEach(({ metadata, name }) => { - if (metadata.known_models) { - metadata.known_models.forEach((model) => { - options.push({ - value: model.name, - label: `${model.name} (${metadata.display_name})`, - provider: name, + // Fetch models for all active providers with dynamic discovery + for (const provider of activeProviders) { + try { + // Try dynamic discovery first + let models = await getProviderModels(provider.name); + + // Fallback to static known_models if dynamic returns empty + if ((!models || models.length === 0) && provider.metadata.known_models?.length) { + models = provider.metadata.known_models.map((m) => m.name); + } + + // Add models to options + if (models && models.length > 0) { + models.forEach((modelName) => { + options.push({ + value: modelName, + label: `${modelName} (${provider.metadata.display_name})`, + provider: provider.name, + }); }); - }); + } + } catch (error) { + // If dynamic fetch fails, use static fallback + console.warn( + `Failed to fetch models for ${provider.name}, using static fallback:`, + error + ); + if (provider.metadata.known_models) { + provider.metadata.known_models.forEach((model) => { + options.push({ + value: model.name, + label: `${model.name} (${provider.metadata.display_name})`, + provider: provider.name, + }); + }); + } } - }); + } } setModelOptions(options); @@ -122,7 +149,7 @@ export function LeadWorkerSettings({ isOpen, onClose }: LeadWorkerSettingsProps) }; loadConfig(); - }, [read, getProviders, currentModel, isOpen]); + }, [read, getProviders, getProviderModels, currentModel, isOpen]); const handleSave = async () => { try { From c2a1ceacae874ea9fb32dc0c8b9a7efa35b65952 Mon Sep 17 00:00:00 2001 From: Eugene Pyvovarov Date: Tue, 26 Aug 2025 15:57:10 +0200 Subject: [PATCH 3/3] removed unneeded test, moved duplicate logic to modelinterface.ts Signed-off-by: Eugene Pyvovarov --- .../src/routes/config_management.rs | 21 -------- .../settings/models/modelInterface.ts | 22 ++++++++ .../models/subcomponents/AddModelModal.tsx | 51 +++++-------------- .../subcomponents/LeadWorkerSettings.tsx | 44 ++++------------ 4 files changed, 43 insertions(+), 95 deletions(-) diff --git a/crates/goose-server/src/routes/config_management.rs b/crates/goose-server/src/routes/config_management.rs index e3d5511a82df..0224cc79565d 100644 --- a/crates/goose-server/src/routes/config_management.rs +++ b/crates/goose-server/src/routes/config_management.rs @@ -938,25 +938,4 @@ mod tests { std::env::remove_var("OPENAI_API_KEY"); } - - #[tokio::test] - async fn test_get_provider_models_ollama_not_running() { - // Test when Ollama is not running (typical scenario in CI/testing) - let test_state = create_test_state().await; - let mut headers = HeaderMap::new(); - headers.insert("X-Secret-Key", "test".parse().unwrap()); - - let result = - get_provider_models(State(test_state), headers, Path("ollama".to_string())).await; - - // Should succeed but return empty array when Ollama is not accessible - // The implementation gracefully handles connection failures and returns Ok(None) - // which translates to an empty Vec in the API response - assert!(result.is_ok(), "Expected successful response from Ollama provider even when not running"); - let models = result.unwrap().0; - - // Should return empty list when Ollama is not accessible - // (implementation returns Ok(None) which becomes empty Vec in response) - assert_eq!(models.len(), 0, "Expected empty models list when Ollama is not running"); - } } diff --git a/ui/desktop/src/components/settings/models/modelInterface.ts b/ui/desktop/src/components/settings/models/modelInterface.ts index 4a397647dde8..c8253384d3c9 100644 --- a/ui/desktop/src/components/settings/models/modelInterface.ts +++ b/ui/desktop/src/components/settings/models/modelInterface.ts @@ -39,3 +39,25 @@ export async function getProviderMetadata( } return matches.metadata; } + +export async function getModelOptionsForProvider( + provider: ProviderDetails, + getProviderModels: (providerName: string) => Promise +): Promise<{ value: string; provider: string }[]> { + let models: string[] = []; + + try { + models = await getProviderModels(provider.name); + } catch (error) { + console.warn(`Failed to fetch models for ${provider.name}:`, error); + } + + if ((!models || models.length === 0) && provider.metadata.known_models?.length) { + models = provider.metadata.known_models.map((m) => m.name); + } + + return models.map((modelName) => ({ + value: modelName, + provider: provider.name, + })); +} diff --git a/ui/desktop/src/components/settings/models/subcomponents/AddModelModal.tsx b/ui/desktop/src/components/settings/models/subcomponents/AddModelModal.tsx index 88ef4e695249..83542b8d8bc8 100644 --- a/ui/desktop/src/components/settings/models/subcomponents/AddModelModal.tsx +++ b/ui/desktop/src/components/settings/models/subcomponents/AddModelModal.tsx @@ -16,7 +16,7 @@ import { Select } from '../../../ui/Select'; import { useConfig } from '../../../ConfigContext'; import { useModelAndProvider } from '../../../ModelAndProviderContext'; import type { View } from '../../../../utils/navigationUtils'; -import Model, { getProviderMetadata } from '../modelInterface'; +import Model, { getProviderMetadata, getModelOptionsForProvider } from '../modelInterface'; import { getPredefinedModelsFromEnv, shouldShowPredefinedModels } from '../predefinedModelsUtils'; type AddModelModalProps = { @@ -145,54 +145,27 @@ export const AddModelModal = ({ onClose, setView }: AddModelModalProps) => { // Fetching models for all providers const modelPromises = activeProviders.map(async (p) => { - const providerName = p.name; - try { - let models = await getProviderModels(providerName); - // Fallback to known_models if server returned none - if ((!models || models.length === 0) && p.metadata.known_models?.length) { - models = p.metadata.known_models.map((m) => m.name); - } - return { provider: p, models, error: null }; - } catch (e: unknown) { - return { - provider: p, - models: null, - error: `Failed to fetch models for ${providerName}${e instanceof Error ? `: ${e.message}` : ''}`, - }; - } + const modelOptions = await getModelOptionsForProvider(p, getProviderModels); + return { provider: p, modelOptions }; }); const results = await Promise.all(modelPromises); - // Process results and build grouped options + // Build grouped options const groupedOptions: { options: { value: string; label: string; provider: string }[] }[] = []; - const errors: string[] = []; - - results.forEach(({ provider: p, models, error }) => { - if (error) { - errors.push(error); - // Fallback to metadata known_models on error - if (p.metadata.known_models && p.metadata.known_models.length > 0) { - groupedOptions.push({ - options: p.metadata.known_models.map(({ name }) => ({ - value: name, - label: name, - provider: p.name, - })), - }); - } - } else if (models && models.length > 0) { + + results.forEach(({ modelOptions }) => { + if (modelOptions.length > 0) { groupedOptions.push({ - options: models.map((m) => ({ value: m, label: m, provider: p.name })), + options: modelOptions.map((option) => ({ + value: option.value, + label: option.value, + provider: option.provider, + })), }); } }); - // Log errors if any providers failed (don't show to user) - if (errors.length > 0) { - console.error('Provider model fetch errors:', errors); - } - // Add the "Custom model" option to each provider group groupedOptions.forEach((group) => { const providerName = group.options[0]?.provider; diff --git a/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx b/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx index 775c34f24786..9655213105fc 100644 --- a/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx +++ b/ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx @@ -6,6 +6,7 @@ import { Select } from '../../../ui/Select'; import { Input } from '../../../ui/input'; import { getPredefinedModelsFromEnv, shouldShowPredefinedModels } from '../predefinedModelsUtils'; import { Dialog, DialogContent, DialogHeader, DialogTitle } from '../../../ui/dialog'; +import { getModelOptionsForProvider } from '../modelInterface'; interface LeadWorkerSettingsProps { isOpen: boolean; @@ -102,41 +103,14 @@ export function LeadWorkerSettings({ isOpen, onClose }: LeadWorkerSettingsProps) // Fetch models for all active providers with dynamic discovery for (const provider of activeProviders) { - try { - // Try dynamic discovery first - let models = await getProviderModels(provider.name); - - // Fallback to static known_models if dynamic returns empty - if ((!models || models.length === 0) && provider.metadata.known_models?.length) { - models = provider.metadata.known_models.map((m) => m.name); - } - - // Add models to options - if (models && models.length > 0) { - models.forEach((modelName) => { - options.push({ - value: modelName, - label: `${modelName} (${provider.metadata.display_name})`, - provider: provider.name, - }); - }); - } - } catch (error) { - // If dynamic fetch fails, use static fallback - console.warn( - `Failed to fetch models for ${provider.name}, using static fallback:`, - error - ); - if (provider.metadata.known_models) { - provider.metadata.known_models.forEach((model) => { - options.push({ - value: model.name, - label: `${model.name} (${provider.metadata.display_name})`, - provider: provider.name, - }); - }); - } - } + const providerOptions = await getModelOptionsForProvider(provider, getProviderModels); + providerOptions.forEach((option) => { + options.push({ + value: option.value, + label: `${option.value} (${provider.metadata.display_name})`, + provider: option.provider, + }); + }); } }