From 654e5c6e633976a9158633f93f6e078c797c1a7d Mon Sep 17 00:00:00 2001 From: Yingjie He Date: Thu, 30 Jan 2025 09:24:52 -0800 Subject: [PATCH] feat: update ui for ollama host (#912) --- .../src/routes/{secrets.rs => configs.rs} | 71 +++++++++++-------- crates/goose-server/src/routes/mod.rs | 4 +- .../src/routes/providers_and_keys.json | 2 +- crates/goose/src/config/base.rs | 61 +++++++++++++--- ui/desktop/src/ChatWindow.tsx | 2 +- .../src/components/settings/api_keys/types.ts | 4 +- .../components/settings/api_keys/utils.tsx | 29 ++------ .../ConfigureBuiltInExtensionModal.tsx | 2 +- .../extensions/ConfigureExtensionModal.tsx | 2 +- .../extensions/ManualExtensionModal.tsx | 2 +- .../settings/providers/BaseProviderGrid.tsx | 66 +---------------- .../providers/ConfigureProvidersGrid.tsx | 21 +++--- .../components/settings/providers/utils.tsx | 14 ---- .../welcome_screen/ProviderGrid.tsx | 4 +- 14 files changed, 125 insertions(+), 159 deletions(-) rename crates/goose-server/src/routes/{secrets.rs => configs.rs} (71%) delete mode 100644 ui/desktop/src/components/settings/providers/utils.tsx diff --git a/crates/goose-server/src/routes/secrets.rs b/crates/goose-server/src/routes/configs.rs similarity index 71% rename from crates/goose-server/src/routes/secrets.rs rename to crates/goose-server/src/routes/configs.rs index a4a29be4..05e599c4 100644 --- a/crates/goose-server/src/routes/secrets.rs +++ b/crates/goose-server/src/routes/configs.rs @@ -8,23 +8,23 @@ use serde_json::Value; use std::collections::HashMap; #[derive(Serialize)] -struct SecretResponse { +struct ConfigResponse { error: bool, } #[derive(Deserialize)] #[serde(rename_all = "camelCase")] -struct SecretRequest { +struct ConfigRequest { key: String, value: String, is_secret: bool, } -async fn store_secret( +async fn store_config( State(state): State, headers: HeaderMap, - Json(request): Json, -) -> Result, StatusCode> { + Json(request): Json, +) -> Result, StatusCode> { // Verify secret key let secret_key = headers .get("X-Secret-Key") @@ -42,18 +42,18 @@ async fn store_secret( config.set(&request.key, Value::String(request.value)) }; match result { - Ok(_) => Ok(Json(SecretResponse { error: false })), - Err(_) => Ok(Json(SecretResponse { error: true })), + Ok(_) => Ok(Json(ConfigResponse { error: false })), + Err(_) => Ok(Json(ConfigResponse { error: true })), } } #[derive(Debug, Serialize, Deserialize)] -pub struct ProviderSecretRequest { +pub struct ProviderConfigRequest { pub providers: Vec, } #[derive(Debug, Serialize, Deserialize)] -pub struct SecretStatus { +pub struct ConfigStatus { pub is_set: bool, pub location: Option, } @@ -64,7 +64,7 @@ pub struct ProviderResponse { pub name: Option, pub description: Option, pub models: Option>, - pub secret_status: HashMap, + pub config_status: HashMap, } #[derive(Debug, Serialize, Deserialize)] @@ -80,30 +80,33 @@ static PROVIDER_ENV_REQUIREMENTS: Lazy> = Lazy:: serde_json::from_str(contents).expect("Failed to parse providers_and_keys.json") }); -fn check_key_status(key: &str) -> (bool, Option) { +fn check_key_status(config: &Config, key: &str) -> (bool, Option) { if let Ok(_value) = std::env::var(key) { (true, Some("env".to_string())) - } else if Config::global().get_secret::(key).is_ok() { + } else if config.get::(key).is_ok() { + (true, Some("yaml".to_string())) + } else if config.get_secret::(key).is_ok() { (true, Some("keyring".to_string())) } else { (false, None) } } -async fn check_provider_secrets( - Json(request): Json, +async fn check_provider_configs( + Json(request): Json, ) -> Result>, StatusCode> { let mut response = HashMap::new(); + let config = Config::global(); for provider_name in request.providers { if let Some(provider_config) = PROVIDER_ENV_REQUIREMENTS.get(&provider_name) { - let mut secret_status = HashMap::new(); + let mut config_status = HashMap::new(); for key in &provider_config.required_keys { - let (key_set, key_location) = check_key_status(key); - secret_status.insert( + let (key_set, key_location) = check_key_status(config, key); + config_status.insert( key.to_string(), - SecretStatus { + ConfigStatus { is_set: key_set, location: key_location, }, @@ -117,7 +120,7 @@ async fn check_provider_secrets( name: Some(provider_config.name.clone()), description: Some(provider_config.description.clone()), models: Some(provider_config.models.clone()), - secret_status, + config_status, }, ); } else { @@ -128,7 +131,7 @@ async fn check_provider_secrets( name: None, description: None, models: None, - secret_status: HashMap::new(), + config_status: HashMap::new(), }, ); } @@ -138,14 +141,16 @@ async fn check_provider_secrets( } #[derive(Deserialize)] -struct DeleteSecretRequest { +#[serde(rename_all = "camelCase")] +struct DeleteConfigRequest { key: String, + is_secret: bool, } -async fn delete_secret( +async fn delete_config( State(state): State, headers: HeaderMap, - Json(request): Json, + Json(request): Json, ) -> Result { // Verify secret key let secret_key = headers @@ -158,7 +163,13 @@ async fn delete_secret( } // Attempt to delete the key - match Config::global().delete_secret(&request.key) { + let config = Config::global(); + let result = if request.is_secret { + config.delete_secret(&request.key) + } else { + config.delete(&request.key) + }; + match result { Ok(_) => Ok(StatusCode::NO_CONTENT), Err(_) => Err(StatusCode::NOT_FOUND), } @@ -166,9 +177,9 @@ async fn delete_secret( pub fn routes(state: AppState) -> Router { Router::new() - .route("/secrets/providers", post(check_provider_secrets)) - .route("/secrets/store", post(store_secret)) - .route("/secrets/delete", delete(delete_secret)) + .route("/configs/providers", post(check_provider_configs)) + .route("/configs/store", post(store_config)) + .route("/configs/delete", delete(delete_config)) .with_state(state) } @@ -179,12 +190,12 @@ mod tests { #[tokio::test] async fn test_unsupported_provider() { // Setup - let request = ProviderSecretRequest { + let request = ProviderConfigRequest { providers: vec!["unsupported_provider".to_string()], }; // Execute - let result = check_provider_secrets(Json(request)).await; + let result = check_provider_configs(Json(request)).await; // Assert assert!(result.is_ok()); @@ -194,6 +205,6 @@ mod tests { .get("unsupported_provider") .expect("Provider should exist"); assert!(!provider_status.supported); - assert!(provider_status.secret_status.is_empty()); + assert!(provider_status.config_status.is_empty()); } } diff --git a/crates/goose-server/src/routes/mod.rs b/crates/goose-server/src/routes/mod.rs index 231c0242..b95286e4 100644 --- a/crates/goose-server/src/routes/mod.rs +++ b/crates/goose-server/src/routes/mod.rs @@ -1,9 +1,9 @@ // Export route modules pub mod agent; +pub mod configs; pub mod extension; pub mod health; pub mod reply; -pub mod secrets; use axum::Router; @@ -14,5 +14,5 @@ pub fn configure(state: crate::state::AppState) -> Router { .merge(reply::routes(state.clone())) .merge(agent::routes(state.clone())) .merge(extension::routes(state.clone())) - .merge(secrets::routes(state)) + .merge(configs::routes(state)) } diff --git a/crates/goose-server/src/routes/providers_and_keys.json b/crates/goose-server/src/routes/providers_and_keys.json index 54148a54..ae21d035 100644 --- a/crates/goose-server/src/routes/providers_and_keys.json +++ b/crates/goose-server/src/routes/providers_and_keys.json @@ -33,7 +33,7 @@ "name": "Ollama", "description": "Lorem ipsum", "models": ["qwen2.5"], - "required_keys": [] + "required_keys": ["OLLAMA_HOST"] }, "openrouter": { "name": "OpenRouter", diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 15150bfa..6e305b64 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -165,6 +165,21 @@ impl Config { } } + // Save current values to the config file + fn save_values(&self, values: HashMap) -> Result<(), ConfigError> { + // Convert to YAML for storage + let yaml_value = serde_yaml::to_string(&values)?; + + // Ensure the directory exists + if let Some(parent) = self.config_path.parent() { + std::fs::create_dir_all(parent) + .map_err(|e| ConfigError::DirectoryError(e.to_string()))?; + } + + std::fs::write(&self.config_path, yaml_value)?; + Ok(()) + } + // Load current secrets from the keyring fn load_secrets(&self) -> Result, ConfigError> { let entry = Entry::new(&self.keyring_service, KEYRING_USERNAME)?; @@ -231,17 +246,27 @@ impl Config { let mut values = self.load_values()?; values.insert(key.to_string(), value); - // Convert to YAML for storage - let yaml_value = serde_yaml::to_string(&values)?; + self.save_values(values) + } - // Ensure the directory exists - if let Some(parent) = self.config_path.parent() { - std::fs::create_dir_all(parent) - .map_err(|e| ConfigError::DirectoryError(e.to_string()))?; - } + /// Delete a configuration value in the config file. + /// + /// This will immediately write the value to the config file. The value + /// can be any type that can be serialized to JSON/YAML. + /// + /// Note that this does not affect environment variables - those can only + /// be set through the system environment. + /// + /// # Errors + /// + /// Returns a ConfigError if: + /// - There is an error reading or writing the config file + /// - There is an error serializing the value + pub fn delete(&self, key: &str) -> Result<(), ConfigError> { + let mut values = self.load_values()?; + values.remove(key); - std::fs::write(&self.config_path, yaml_value)?; - Ok(()) + self.save_values(values) } /// Get a secret value. @@ -408,6 +433,24 @@ mod tests { Ok(()) } + #[test] + fn test_value_management() -> Result<(), ConfigError> { + let temp_file = NamedTempFile::new().unwrap(); + let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + + config.set("key", Value::String("value".to_string()))?; + + let value: String = config.get("key")?; + assert_eq!(value, "value"); + + config.delete("key")?; + + let result: Result = config.get("key"); + assert!(matches!(result, Err(ConfigError::NotFound(_)))); + + Ok(()) + } + #[test] #[serial] fn test_secret_management() -> Result<(), ConfigError> { diff --git a/ui/desktop/src/ChatWindow.tsx b/ui/desktop/src/ChatWindow.tsx index d4ac0013..f50c5e78 100644 --- a/ui/desktop/src/ChatWindow.tsx +++ b/ui/desktop/src/ChatWindow.tsx @@ -385,7 +385,7 @@ export default function ChatWindow() { }, []); const storeSecret = async (key: string, value: string) => { - const response = await fetch(getApiUrl('/secrets/store'), { + const response = await fetch(getApiUrl('/configs/store'), { method: 'POST', headers: { 'Content-Type': 'application/json', diff --git a/ui/desktop/src/components/settings/api_keys/types.ts b/ui/desktop/src/components/settings/api_keys/types.ts index 7585b7fd..78dc57b3 100644 --- a/ui/desktop/src/components/settings/api_keys/types.ts +++ b/ui/desktop/src/components/settings/api_keys/types.ts @@ -3,10 +3,10 @@ export interface ProviderResponse { name?: string; description?: string; models?: string[]; - secret_status: Record; + config_status: Record; } -export interface SecretDetails { +export interface ConfigDetails { key: string; is_set: boolean; location?: string; diff --git a/ui/desktop/src/components/settings/api_keys/utils.tsx b/ui/desktop/src/components/settings/api_keys/utils.tsx index 3c906ac6..c8291812 100644 --- a/ui/desktop/src/components/settings/api_keys/utils.tsx +++ b/ui/desktop/src/components/settings/api_keys/utils.tsx @@ -1,6 +1,5 @@ import { Provider, ProviderResponse } from './types'; import { getApiUrl, getSecretKey } from '../../../config'; -import { special_provider_cases } from '../providers/utils'; export function isSecretKey(keyName: string): boolean { // Ollama and Databricks use host name right now and it should not be stored as secret. @@ -10,21 +9,12 @@ export function isSecretKey(keyName: string): boolean { export async function getActiveProviders(): Promise { try { // Fetch the secrets settings - const secretsSettings = await getSecretsSettings(); - - // Check for special provider cases (e.g. ollama needs to be installed in Applications folder) - const specialCasesResults = await Promise.all( - Object.entries(special_provider_cases).map(async ([providerName, checkFunction]) => { - const isActive = await checkFunction(); // Dynamically re-check status - console.log(`Special case result for ${providerName}:`, isActive); - return isActive ? providerName : null; - }) - ); + const configSettings = await getConfigSettings(); // Extract active providers based on `is_set` in `secret_status` or providers with no keys - const activeProviders = Object.values(secretsSettings) // Convert object to array + const activeProviders = Object.values(configSettings) // Convert object to array .filter((provider) => { - const apiKeyStatus = Object.values(provider.secret_status || {}); // Get all key statuses + const apiKeyStatus = Object.values(provider.config_status || {}); // Get all key statuses // Include providers if: // - They have at least one key set (`is_set: true`) @@ -32,25 +22,20 @@ export async function getActiveProviders(): Promise { }) .map((provider) => provider.name || 'Unknown Provider'); // Extract provider name - // Combine active providers from secrets settings and special cases - const allActiveProviders = [ - ...activeProviders, - ...specialCasesResults.filter((provider) => provider !== null), // Filter out null results - ]; - return allActiveProviders; + return activeProviders; } catch (error) { console.error('Failed to get active providers:', error); return []; } } -export async function getSecretsSettings(): Promise> { +export async function getConfigSettings(): Promise> { const providerList = await getProvidersList(); // Extract the list of IDs const providerIds = providerList.map((provider) => provider.id); - // Fetch secrets state (set/unset) using the provider IDs - const response = await fetch(getApiUrl('/secrets/providers'), { + // Fetch configs state (set/unset) using the provider IDs + const response = await fetch(getApiUrl('/configs/providers'), { method: 'POST', headers: { 'Content-Type': 'application/json', diff --git a/ui/desktop/src/components/settings/extensions/ConfigureBuiltInExtensionModal.tsx b/ui/desktop/src/components/settings/extensions/ConfigureBuiltInExtensionModal.tsx index 05340f70..01a42dd6 100644 --- a/ui/desktop/src/components/settings/extensions/ConfigureBuiltInExtensionModal.tsx +++ b/ui/desktop/src/components/settings/extensions/ConfigureBuiltInExtensionModal.tsx @@ -43,7 +43,7 @@ export function ConfigureBuiltInExtensionModal({ const value = envValues[envKey]; if (!value) continue; - const storeResponse = await fetch(getApiUrl('/secrets/store'), { + const storeResponse = await fetch(getApiUrl('/configs/store'), { method: 'POST', headers: { 'Content-Type': 'application/json', diff --git a/ui/desktop/src/components/settings/extensions/ConfigureExtensionModal.tsx b/ui/desktop/src/components/settings/extensions/ConfigureExtensionModal.tsx index 4a81d70b..0ae8914d 100644 --- a/ui/desktop/src/components/settings/extensions/ConfigureExtensionModal.tsx +++ b/ui/desktop/src/components/settings/extensions/ConfigureExtensionModal.tsx @@ -45,7 +45,7 @@ export function ConfigureExtensionModal({ const value = envValues[envKey]; if (!value) continue; - const storeResponse = await fetch(getApiUrl('/secrets/store'), { + const storeResponse = await fetch(getApiUrl('/configs/store'), { method: 'POST', headers: { 'Content-Type': 'application/json', diff --git a/ui/desktop/src/components/settings/extensions/ManualExtensionModal.tsx b/ui/desktop/src/components/settings/extensions/ManualExtensionModal.tsx index 92adda3b..75336450 100644 --- a/ui/desktop/src/components/settings/extensions/ManualExtensionModal.tsx +++ b/ui/desktop/src/components/settings/extensions/ManualExtensionModal.tsx @@ -59,7 +59,7 @@ export function ManualExtensionModal({ isOpen, onClose, onSubmit }: ManualExtens try { // Store environment variables as secrets for (const envVar of envVars) { - const storeResponse = await fetch(getApiUrl('/secrets/store'), { + const storeResponse = await fetch(getApiUrl('/configs/store'), { method: 'POST', headers: { 'Content-Type': 'application/json', diff --git a/ui/desktop/src/components/settings/providers/BaseProviderGrid.tsx b/ui/desktop/src/components/settings/providers/BaseProviderGrid.tsx index acb50b92..0af1d6e9 100644 --- a/ui/desktop/src/components/settings/providers/BaseProviderGrid.tsx +++ b/ui/desktop/src/components/settings/providers/BaseProviderGrid.tsx @@ -112,35 +112,6 @@ function BaseProviderCard({ )} - - {/* Not Configured state: Red exclamation mark for Ollama */} - {!isConfigured && name === 'Ollama' && ( - - - -
- ! -
- - - -

- To use, the{' '} - - Ollama app - {' '} - must be installed on your machine and open. -

- - - - - )}

{description} @@ -149,41 +120,6 @@ function BaseProviderCard({

- {!isConfigured && name === 'Ollama' && ( - - - - - - - -

Re-check for active Ollama app running in the background.

- - - - - )} - {/* Default "Add Keys" Button for other providers */} {!isConfigured && onAddKeys && hasRequiredKeys && ( @@ -251,7 +187,7 @@ function BaseProviderCard({ -

Remove {name} API Key

+

Remove {name} API Key or Host

 diff --git a/ui/desktop/src/components/settings/providers/ConfigureProvidersGrid.tsx b/ui/desktop/src/components/settings/providers/ConfigureProvidersGrid.tsx index f34e5aca..ab1b5b9d 100644 --- a/ui/desktop/src/components/settings/providers/ConfigureProvidersGrid.tsx +++ b/ui/desktop/src/components/settings/providers/ConfigureProvidersGrid.tsx @@ -90,7 +90,7 @@ export function ConfigureProvidersGrid() { // Delete existing key if provider is already configured const isUpdate = providers.find((p) => p.id === selectedForSetup)?.isConfigured; if (isUpdate) { - const deleteResponse = await fetch(getApiUrl('/secrets/delete'), { + const deleteResponse = await fetch(getApiUrl('/configs/delete'), { method: 'DELETE', headers: { 'Content-Type': 'application/json', @@ -108,7 +108,7 @@ export function ConfigureProvidersGrid() { // Store new key const isSecret = isSecretKey(keyName); - const storeResponse = await fetch(getApiUrl('/secrets/store'), { + const storeResponse = await fetch(getApiUrl('/configs/store'), { method: 'POST', headers: { 'Content-Type': 'application/json', @@ -162,23 +162,28 @@ export function ConfigureProvidersGrid() { return; } + const isSecret = isSecretKey(keyName); + const toastInfo = isSecret ? 'API key' : 'host'; try { // Check if the selected provider is currently active if (currentModel?.provider === providerToDelete.name) { toast.error( - `Cannot delete the API key for ${providerToDelete.name} because it's the provider of the current model (${currentModel.name}). Please switch to a different model first.` + `Cannot delete the ${toastInfo} for ${providerToDelete.name} because it's the provider of the current model (${currentModel.name}). Please switch to a different model first.` ); setIsConfirmationOpen(false); return; } - const deleteResponse = await fetch(getApiUrl('/secrets/delete'), { + const deleteResponse = await fetch(getApiUrl('/configs/delete'), { method: 'DELETE', headers: { 'Content-Type': 'application/json', 'X-Secret-Key': getSecretKey(), }, - body: JSON.stringify({ key: keyName }), + body: JSON.stringify({ + key: keyName, + isSecret, + }), }); if (!deleteResponse.ok) { @@ -188,13 +193,13 @@ export function ConfigureProvidersGrid() { } console.log('Key deleted successfully.'); - toast.success(`Successfully deleted API key for ${providerToDelete.name}`); + toast.success(`Successfully deleted ${toastInfo} for ${providerToDelete.name}`); const updatedKeys = await getActiveProviders(); setActiveKeys(updatedKeys); } catch (error) { console.error('Error deleting key:', error); - toast.error(`Unable to delete API key for ${providerToDelete.name}`); + toast.error(`Unable to delete ${toastInfo} for ${providerToDelete.name}`); } setIsConfirmationOpen(false); }; @@ -233,7 +238,7 @@ export function ConfigureProvidersGrid() { {isConfirmationOpen && providerToDelete && ( setIsConfirmationOpen(false)} /> diff --git a/ui/desktop/src/components/settings/providers/utils.tsx b/ui/desktop/src/components/settings/providers/utils.tsx deleted file mode 100644 index 821b7865..00000000 --- a/ui/desktop/src/components/settings/providers/utils.tsx +++ /dev/null @@ -1,14 +0,0 @@ -export const special_provider_cases = { - Ollama: async () => await checkForOllama(), // Dynamically re-check -}; - -export async function checkForOllama() { - console.log('Invoking check-ollama IPC handler...'); - try { - const ollamaInstalled = await window.electron.checkForOllama(); - return ollamaInstalled; - } catch (error) { - console.error('Error invoking check-ollama:', error); - return false; - } -} diff --git a/ui/desktop/src/components/welcome_screen/ProviderGrid.tsx b/ui/desktop/src/components/welcome_screen/ProviderGrid.tsx index 48a9d640..268c4d02 100644 --- a/ui/desktop/src/components/welcome_screen/ProviderGrid.tsx +++ b/ui/desktop/src/components/welcome_screen/ProviderGrid.tsx @@ -83,7 +83,7 @@ export function ProviderGrid({ onSubmit }: ProviderGridProps) { try { if (selectedId && providers.find((p) => p.id === selectedId)?.isConfigured) { - const deleteResponse = await fetch(getApiUrl('/secrets/delete'), { + const deleteResponse = await fetch(getApiUrl('/configs/delete'), { method: 'DELETE', headers: { 'Content-Type': 'application/json', @@ -100,7 +100,7 @@ export function ProviderGrid({ onSubmit }: ProviderGridProps) { } const isSecret = isSecretKey(keyName); - const storeResponse = await fetch(getApiUrl('/secrets/store'), { + const storeResponse = await fetch(getApiUrl('/configs/store'), { method: 'POST', headers: { 'Content-Type': 'application/json',