feat: goose2 add support for custom providers in ui & acp (#8924)

crates/goose/src/acp/server/custom_dispatch.rs

@@ -104,6 +104,54 @@ impl GooseAcpAgent {
         self.on_list_providers(req).await
     }
 
+    #[custom_method(ProviderCatalogListRequest)]
+    async fn dispatch_list_provider_catalog(
+        &self,
+        req: ProviderCatalogListRequest,
+    ) -> Result<ProviderCatalogListResponse, sacp::Error> {
+        self.on_list_provider_catalog(req).await
+    }
+
+    #[custom_method(ProviderCatalogTemplateRequest)]
+    async fn dispatch_get_provider_catalog_template(
+        &self,
+        req: ProviderCatalogTemplateRequest,
+    ) -> Result<ProviderCatalogTemplateResponse, sacp::Error> {
+        self.on_get_provider_catalog_template(req).await
+    }
+
+    #[custom_method(CustomProviderCreateRequest)]
+    async fn dispatch_create_custom_provider(
+        &self,
+        req: CustomProviderCreateRequest,
+    ) -> Result<CustomProviderCreateResponse, sacp::Error> {
+        self.on_create_custom_provider(req).await
+    }
+
+    #[custom_method(CustomProviderReadRequest)]
+    async fn dispatch_read_custom_provider(
+        &self,
+        req: CustomProviderReadRequest,
+    ) -> Result<CustomProviderReadResponse, sacp::Error> {
+        self.on_read_custom_provider(req).await
+    }
+
+    #[custom_method(CustomProviderUpdateRequest)]
+    async fn dispatch_update_custom_provider(
+        &self,
+        req: CustomProviderUpdateRequest,
+    ) -> Result<CustomProviderUpdateResponse, sacp::Error> {
+        self.on_update_custom_provider(req).await
+    }
+
+    #[custom_method(CustomProviderDeleteRequest)]
+    async fn dispatch_delete_custom_provider(
+        &self,
+        req: CustomProviderDeleteRequest,
+    ) -> Result<CustomProviderDeleteResponse, sacp::Error> {
+        self.on_delete_custom_provider(req).await
+    }
+
     #[custom_method(RefreshProviderInventoryRequest)]
     async fn dispatch_refresh_provider_inventory(
         &self,

crates/goose/src/acp/server/providers.rs

@@ -1,4 +1,6 @@
 use super::*;
+use crate::config::declarative_providers;
+use std::str::FromStr;
 
 fn inventory_entry_to_dto(entry: ProviderInventoryEntry) -> ProviderInventoryEntryDto {
     let stale = ProviderInventoryService::is_stale(&entry);
@@ -110,6 +112,197 @@ fn provider_config_field_value(
     }
 }
 
+fn provider_catalog_entry_to_dto(
+    entry: crate::providers::catalog::ProviderCatalogEntry,
+) -> ProviderCatalogEntryDto {
+    ProviderCatalogEntryDto {
+        provider_id: entry.id,
+        name: entry.name,
+        format: entry.format,
+        api_url: entry.api_url,
+        model_count: entry.model_count,
+        doc_url: entry.doc_url,
+        env_var: entry.env_var,
+    }
+}
+
+fn provider_template_to_dto(
+    template: crate::providers::catalog::ProviderTemplate,
+) -> ProviderTemplateDto {
+    ProviderTemplateDto {
+        provider_id: template.id,
+        name: template.name,
+        format: template.format,
+        api_url: template.api_url,
+        models: template
+            .models
+            .into_iter()
+            .map(|model| ProviderTemplateModelDto {
+                id: model.id,
+                name: model.name,
+                context_limit: model.context_limit,
+                capabilities: ProviderTemplateCapabilitiesDto {
+                    tool_call: model.capabilities.tool_call,
+                    reasoning: model.capabilities.reasoning,
+                    attachment: model.capabilities.attachment,
+                    temperature: model.capabilities.temperature,
+                },
+                deprecated: model.deprecated,
+            })
+            .collect(),
+        supports_streaming: template.supports_streaming,
+        env_var: template.env_var,
+        doc_url: template.doc_url,
+    }
+}
+
+fn custom_provider_engine_to_dto(engine: &declarative_providers::ProviderEngine) -> &'static str {
+    match engine {
+        declarative_providers::ProviderEngine::OpenAI => "openai_compatible",
+        declarative_providers::ProviderEngine::Anthropic => "anthropic_compatible",
+        declarative_providers::ProviderEngine::Ollama => "ollama_compatible",
+    }
+}
+
+fn normalize_custom_provider_engine(engine: &str) -> Result<String, sacp::Error> {
+    let engine = engine.trim().to_lowercase();
+    if declarative_providers::ProviderEngine::from_str(&engine).is_err() {
+        return Err(sacp::Error::invalid_params()
+            .data(format!("Unsupported custom provider engine: {engine}")));
+    }
+
+    match engine.as_str() {
+        "openai" | "openai_compatible" => Ok("openai_compatible".to_string()),
+        "anthropic" | "anthropic_compatible" => Ok("anthropic_compatible".to_string()),
+        "ollama" | "ollama_compatible" => Ok("ollama_compatible".to_string()),
+        _ => unreachable!("provider engine was validated above"),
+    }
+}
+
+fn non_empty_trimmed(value: String, field: &str) -> Result<String, sacp::Error> {
+    let value = value.trim().to_string();
+    if value.is_empty() {
+        return Err(sacp::Error::invalid_params().data(format!("{field} cannot be empty")));
+    }
+    Ok(value)
+}
+
+fn normalize_optional_string(value: Option<String>) -> Option<String> {
+    value.and_then(|value| {
+        let value = value.trim().to_string();
+        (!value.is_empty()).then_some(value)
+    })
+}
+
+fn normalize_custom_provider_upsert(
+    mut provider: CustomProviderUpsertDto,
+    require_api_key: bool,
+) -> Result<CustomProviderUpsertDto, sacp::Error> {
+    provider.engine = normalize_custom_provider_engine(&provider.engine)?;
+    provider.display_name = non_empty_trimmed(provider.display_name, "displayName")?;
+    provider.api_url = non_empty_trimmed(provider.api_url, "apiUrl")?;
+    let url = url::Url::parse(&provider.api_url)
+        .map_err(|_| sacp::Error::invalid_params().data("apiUrl must be a valid URL"))?;
+    if !matches!(url.scheme(), "http" | "https") {
+        return Err(sacp::Error::invalid_params().data("apiUrl must use HTTP or HTTPS"));
+    }
+
+    provider.api_key = provider.api_key.and_then(|api_key| {
+        let api_key = api_key.trim().to_string();
+        (!api_key.is_empty()).then_some(api_key)
+    });
+    if require_api_key && provider.requires_auth && provider.api_key.is_none() {
+        return Err(sacp::Error::invalid_params().data("apiKey cannot be empty"));
+    }
+    provider.models = provider
+        .models
+        .into_iter()
+        .filter_map(|model| {
+            let model = model.trim().to_string();
+            (!model.is_empty()).then_some(model)
+        })
+        .collect();
+    if provider.models.is_empty() {
+        return Err(sacp::Error::invalid_params().data("models cannot be empty"));
+    }
+
+    provider.headers = provider
+        .headers
+        .into_iter()
+        .map(|(key, value)| {
+            let key = key.trim().to_string();
+            let value = value.trim().to_string();
+            if key.is_empty() {
+                return Ok(None);
+            }
+            reqwest::header::HeaderName::from_bytes(key.as_bytes()).map_err(|_| {
+                sacp::Error::invalid_params().data(format!("Invalid header name: {key}"))
+            })?;
+            reqwest::header::HeaderValue::from_str(&value).map_err(|_| {
+                sacp::Error::invalid_params().data(format!("Invalid header value for: {key}"))
+            })?;
+            Ok(Some((key, value)))
+        })
+        .collect::<Result<Vec<_>, sacp::Error>>()?
+        .into_iter()
+        .flatten()
+        .collect();
+    provider.catalog_provider_id = normalize_optional_string(provider.catalog_provider_id);
+    provider.base_path = normalize_optional_string(provider.base_path);
+    Ok(provider)
+}
+
+fn custom_provider_headers(headers: HashMap<String, String>) -> Option<HashMap<String, String>> {
+    (!headers.is_empty()).then_some(headers)
+}
+
+fn load_declarative_provider_for_client(
+    provider_id: &str,
+) -> Result<declarative_providers::LoadedProvider, sacp::Error> {
+    declarative_providers::load_provider(provider_id).map_err(|error| {
+        if error.to_string().contains("Provider not found") {
+            sacp::Error::invalid_params().data(format!("Unknown provider: {provider_id}"))
+        } else if error.to_string().contains("Invalid provider id") {
+            sacp::Error::invalid_params().data(error.to_string())
+        } else {
+            sacp::Error::internal_error().data(error.to_string())
+        }
+    })
+}
+
+fn custom_provider_config_to_dto(
+    config: &declarative_providers::DeclarativeProviderConfig,
+) -> CustomProviderConfigDto {
+    let api_key_env = normalize_optional_string(Some(config.api_key_env.clone()));
+    let api_key_set = api_key_env
+        .as_ref()
+        .map(|key| {
+            Config::global()
+                .get_secret::<serde_json::Value>(key)
+                .is_ok()
+        })
+        .unwrap_or(false);
+
+    CustomProviderConfigDto {
+        provider_id: config.name.clone(),
+        engine: custom_provider_engine_to_dto(&config.engine).to_string(),
+        display_name: config.display_name.clone(),
+        api_url: config.base_url.clone(),
+        models: config
+            .models
+            .iter()
+            .map(|model| model.name.clone())
+            .collect(),
+        supports_streaming: config.supports_streaming,
+        headers: config.headers.clone().unwrap_or_default(),
+        requires_auth: config.requires_auth,
+        catalog_provider_id: config.catalog_provider_id.clone(),
+        base_path: config.base_path.clone(),
+        api_key_env,
+        api_key_set,
+    }
+}
+
 fn refresh_skip_reason_to_dto(reason: RefreshSkipReason) -> RefreshProviderInventorySkipReasonDto {
     match reason {
         RefreshSkipReason::UnknownProvider => {
@@ -154,6 +347,196 @@ impl GooseAcpAgent {
         })
     }
 
+    pub(super) async fn on_list_provider_catalog(
+        &self,
+        req: ProviderCatalogListRequest,
+    ) -> Result<ProviderCatalogListResponse, sacp::Error> {
+        let formats = match req.format {
+            Some(format) => vec![format
+                .parse::<crate::providers::catalog::ProviderFormat>()
+                .map_err(|error| sacp::Error::invalid_params().data(error))?],
+            None => vec![
+                crate::providers::catalog::ProviderFormat::OpenAI,
+                crate::providers::catalog::ProviderFormat::Anthropic,
+                crate::providers::catalog::ProviderFormat::Ollama,
+            ],
+        };
+
+        let mut providers = Vec::new();
+        for format in formats {
+            providers.extend(
+                crate::providers::catalog::get_providers_by_format(format)
+                    .await
+                    .into_iter()
+                    .map(provider_catalog_entry_to_dto),
+            );
+        }
+        providers.sort_by(|a, b| {
+            a.name
+                .cmp(&b.name)
+                .then_with(|| a.provider_id.cmp(&b.provider_id))
+        });
+
+        Ok(ProviderCatalogListResponse { providers })
+    }
+
+    pub(super) async fn on_get_provider_catalog_template(
+        &self,
+        req: ProviderCatalogTemplateRequest,
+    ) -> Result<ProviderCatalogTemplateResponse, sacp::Error> {
+        let template = crate::providers::catalog::get_provider_template(&req.provider_id)
+            .ok_or_else(|| {
+                sacp::Error::invalid_params()
+                    .data(format!("Unknown catalog provider: {}", req.provider_id))
+            })?;
+        Ok(ProviderCatalogTemplateResponse {
+            template: provider_template_to_dto(template),
+        })
+    }
+
+    pub(super) async fn on_create_custom_provider(
+        &self,
+        req: CustomProviderCreateRequest,
+    ) -> Result<CustomProviderCreateResponse, sacp::Error> {
+        let provider = normalize_custom_provider_upsert(req.provider, true)?;
+        let config = declarative_providers::create_custom_provider(
+            declarative_providers::CreateCustomProviderParams {
+                engine: provider.engine,
+                display_name: provider.display_name,
+                api_url: provider.api_url,
+                api_key: provider.api_key,
+                models: provider.models,
+                supports_streaming: provider.supports_streaming,
+                headers: custom_provider_headers(provider.headers),
+                requires_auth: provider.requires_auth,
+                catalog_provider_id: provider.catalog_provider_id,
+                base_path: provider.base_path,
+            },
+        )
+        .internal_err_ctx("Failed to create custom provider")?;
+
+        Config::global().invalidate_secrets_cache();
+        crate::providers::refresh_custom_providers()
+            .await
+            .internal_err_ctx("Failed to refresh custom providers")?;
+
+        let provider_id = config.name;
+        let provider_ids = [provider_id.clone()];
+        let status = Self::provider_config_status(provider_id.clone()).await;
+        let refresh = self.start_provider_inventory_refresh(&provider_ids).await?;
+        Ok(CustomProviderCreateResponse {
+            provider_id,
+            status,
+            refresh,
+        })
+    }
+
+    pub(super) async fn on_read_custom_provider(
+        &self,
+        req: CustomProviderReadRequest,
+    ) -> Result<CustomProviderReadResponse, sacp::Error> {
+        let loaded = load_declarative_provider_for_client(&req.provider_id)?;
+        let status = Self::provider_config_status(req.provider_id).await;
+        Ok(CustomProviderReadResponse {
+            provider: custom_provider_config_to_dto(&loaded.config),
+            editable: loaded.is_editable,
+            status,
+        })
+    }
+
+    pub(super) async fn on_update_custom_provider(
+        &self,
+        req: CustomProviderUpdateRequest,
+    ) -> Result<CustomProviderUpdateResponse, sacp::Error> {
+        let loaded = load_declarative_provider_for_client(&req.provider_id)?;
+        if !loaded.is_editable {
+            return Err(sacp::Error::invalid_params()
+                .data(format!("Provider is not editable: {}", req.provider_id)));
+        }
+
+        let provider = normalize_custom_provider_upsert(req.provider, false)?;
+        if provider.requires_auth && provider.api_key.is_none() {
+            let api_key_env = if loaded.config.api_key_env.is_empty() {
+                declarative_providers::generate_api_key_name(&req.provider_id)
+            } else {
+                loaded.config.api_key_env.clone()
+            };
+            if Config::global().get_secret::<String>(&api_key_env).is_err() {
+                return Err(sacp::Error::invalid_params()
+                    .data("apiKey is required when auth is enabled and no secret is stored"));
+            }
+        }
+        declarative_providers::update_custom_provider(
+            declarative_providers::UpdateCustomProviderParams {
+                id: req.provider_id.clone(),
+                engine: provider.engine,
+                display_name: provider.display_name,
+                api_url: provider.api_url,
+                api_key: provider.api_key,
+                models: provider.models,
+                supports_streaming: provider.supports_streaming,
+                headers: Some(provider.headers),
+                requires_auth: provider.requires_auth,
+                catalog_provider_id: provider.catalog_provider_id,
+                base_path: provider.base_path,
+            },
+        )
+        .internal_err_ctx("Failed to update custom provider")?;
+
+        Config::global().invalidate_secrets_cache();
+        crate::providers::refresh_custom_providers()
+            .await
+            .internal_err_ctx("Failed to refresh custom providers")?;
+
+        let provider_ids = [req.provider_id.clone()];
+        let status = Self::provider_config_status(req.provider_id.clone()).await;
+        let refresh = self.start_provider_inventory_refresh(&provider_ids).await?;
+        Ok(CustomProviderUpdateResponse {
+            provider_id: req.provider_id,
+            status,
+            refresh,
+        })
+    }
+
+    pub(super) async fn on_delete_custom_provider(
+        &self,
+        req: CustomProviderDeleteRequest,
+    ) -> Result<CustomProviderDeleteResponse, sacp::Error> {
+        let loaded = load_declarative_provider_for_client(&req.provider_id)?;
+        if !loaded.is_editable {
+            return Err(sacp::Error::invalid_params()
+                .data(format!("Provider is not editable: {}", req.provider_id)));
+        }
+
+        if Config::global()
+            .get_param::<String>("GOOSE_PROVIDER")
+            .ok()
+            .as_deref()
+            == Some(req.provider_id.as_str())
+        {
+            return Err(sacp::Error::invalid_params().data(format!(
+                "Cannot delete active provider: {}",
+                req.provider_id
+            )));
+        }
+
+        declarative_providers::remove_custom_provider(&req.provider_id)
+            .internal_err_ctx("Failed to delete custom provider")?;
+
+        Config::global().invalidate_secrets_cache();
+        crate::providers::refresh_custom_providers()
+            .await
+            .internal_err_ctx("Failed to refresh custom providers")?;
+
+        Ok(CustomProviderDeleteResponse {
+            provider_id: req.provider_id,
+            refresh: RefreshProviderInventoryResponse {
+                started: Vec::new(),
+                skipped: Vec::new(),
+            },
+        })
+    }
+
     pub(super) async fn provider_config_status(provider_id: String) -> ProviderConfigStatusDto {
         let is_configured = match crate::providers::get_from_registry(&provider_id).await {
             Ok(entry) => {

crates/goose/src/config/declarative_providers.rs

@@ -9,6 +9,7 @@ use anyhow::Result;
 use include_dir::{include_dir, Dir};
 use once_cell::sync::Lazy;
 use serde::{Deserialize, Deserializer, Serialize};
+use std::str::FromStr;
 
 /// Deserialize an optional string, treating empty/whitespace-only values as None.
 fn deserialize_non_empty_string<'de, D>(deserializer: D) -> Result<Option<String>, D::Error>
@@ -19,7 +20,7 @@ where
     Ok(opt.filter(|s| !s.trim().is_empty()))
 }
 use std::collections::HashMap;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::sync::Mutex;
 use utoipa::ToSchema;
 
@@ -37,6 +38,19 @@ pub enum ProviderEngine {
     Anthropic,
 }
 
+impl FromStr for ProviderEngine {
+    type Err = anyhow::Error;
+
+    fn from_str(engine: &str) -> Result<Self> {
+        match engine.trim().to_lowercase().as_str() {
+            "openai" | "openai_compatible" => Ok(Self::OpenAI),
+            "anthropic" | "anthropic_compatible" => Ok(Self::Anthropic),
+            "ollama" | "ollama_compatible" => Ok(Self::Ollama),
+            _ => Err(anyhow::anyhow!("Invalid provider type: {}", engine)),
+        }
+    }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)]
 pub struct EnvVarConfig {
     pub name: String,
@@ -147,7 +161,19 @@ static ID_GENERATION_LOCK: Lazy<Mutex<()>> = Lazy::new(|| Mutex::new(()));
 pub fn generate_id(display_name: &str) -> String {
     let _guard = ID_GENERATION_LOCK.lock().unwrap();
 
-    let normalized = display_name.to_lowercase().replace(' ', "_");
+    let normalized = display_name
+        .to_lowercase()
+        .chars()
+        .map(|ch| {
+            if ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_' || ch == '-' {
+                ch
+            } else {
+                '_'
+            }
+        })
+        .collect::<String>()
+        .trim_matches('_')
+        .to_string();
     let base_id = format!("custom_{}", normalized);
 
     let custom_dir = custom_providers_dir();
@@ -162,6 +188,40 @@ pub fn generate_id(display_name: &str) -> String {
     candidate_id
 }
 
+pub fn validate_provider_id(id: &str) -> Result<()> {
+    let mut chars = id.chars();
+    let Some(first) = chars.next() else {
+        return Err(anyhow::anyhow!(
+            "Invalid provider id: provider id cannot be empty"
+        ));
+    };
+
+    if !(first.is_ascii_lowercase() || first.is_ascii_digit() || first == '_') {
+        return Err(anyhow::anyhow!("Invalid provider id: {}", id));
+    }
+
+    if chars.all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_' || ch == '-') {
+        Ok(())
+    } else {
+        Err(anyhow::anyhow!("Invalid provider id: {}", id))
+    }
+}
+
+fn custom_provider_file_path(id: &str) -> Result<PathBuf> {
+    if id.is_empty()
+        || id
+            .chars()
+            .any(|ch| ch == '/' || ch == '\\' || ch.is_control())
+    {
+        return Err(anyhow::anyhow!(
+            "Invalid provider id: {}",
+            if id.is_empty() { "<empty>" } else { id }
+        ));
+    }
+
+    Ok(custom_providers_dir().join(format!("{}.json", id)))
+}
+
 pub fn generate_api_key_name(id: &str) -> String {
     format!("{}_API_KEY", id.to_uppercase())
 }
@@ -171,7 +231,7 @@ pub struct CreateCustomProviderParams {
     pub engine: String,
     pub display_name: String,
     pub api_url: String,
-    pub api_key: String,
+    pub api_key: Option<String>,
     pub models: Vec<String>,
     pub supports_streaming: Option<bool>,
     pub headers: Option<HashMap<String, String>>,
@@ -186,7 +246,7 @@ pub struct UpdateCustomProviderParams {
     pub engine: String,
     pub display_name: String,
     pub api_url: String,
-    pub api_key: String,
+    pub api_key: Option<String>,
     pub models: Vec<String>,
     pub supports_streaming: Option<bool>,
     pub headers: Option<HashMap<String, String>>,
@@ -199,11 +259,17 @@ pub fn create_custom_provider(
     params: CreateCustomProviderParams,
 ) -> Result<DeclarativeProviderConfig> {
     let id = generate_id(&params.display_name);
+    validate_provider_id(&id)?;
 
     let api_key_env = if params.requires_auth {
+        let api_key = params
+            .api_key
+            .as_deref()
+            .filter(|api_key| !api_key.trim().is_empty())
+            .ok_or_else(|| anyhow::anyhow!("apiKey cannot be empty"))?;
         let api_key_name = generate_api_key_name(&id);
         let config = Config::global();
-        config.set_secret(&api_key_name, &params.api_key)?;
+        config.set_secret(&api_key_name, &api_key)?;
         api_key_name
     } else {
         String::new()
@@ -217,12 +283,7 @@ pub fn create_custom_provider(
 
     let provider_config = DeclarativeProviderConfig {
         name: id.clone(),
-        engine: match params.engine.as_str() {
-            "openai_compatible" => ProviderEngine::OpenAI,
-            "anthropic_compatible" => ProviderEngine::Anthropic,
-            "ollama_compatible" => ProviderEngine::Ollama,
-            _ => return Err(anyhow::anyhow!("Invalid provider type: {}", params.engine)),
-        },
+        engine: ProviderEngine::from_str(&params.engine)?,
         display_name: params.display_name.clone(),
         description: Some(format!("Custom {} provider", params.display_name)),
         api_key_env,
@@ -258,18 +319,24 @@ pub fn update_custom_provider(params: UpdateCustomProviderParams) -> Result<()>
     let editable = loaded_provider.is_editable;
 
     let config = Config::global();
-
     let api_key_env = if params.requires_auth {
         let api_key_name = if existing_config.api_key_env.is_empty() {
             generate_api_key_name(&params.id)
         } else {
             existing_config.api_key_env.clone()
         };
-        if !params.api_key.is_empty() {
-            config.set_secret(&api_key_name, &params.api_key)?;
+        if let Some(api_key) = params.api_key.as_deref() {
+            config.set_secret(&api_key_name, &api_key)?;
+        } else if config.get_secret::<String>(&api_key_name).is_err() {
+            return Err(anyhow::anyhow!(
+                "apiKey is required when auth is enabled and no secret is stored"
+            ));
         }
         api_key_name
     } else {
+        if existing_config.api_key_env == generate_api_key_name(&params.id) {
+            config.delete_secret(&existing_config.api_key_env)?;
+        }
         String::new()
     };
 
@@ -282,12 +349,7 @@ pub fn update_custom_provider(params: UpdateCustomProviderParams) -> Result<()>
 
         let updated_config = DeclarativeProviderConfig {
             name: params.id.clone(),
-            engine: match params.engine.as_str() {
-                "openai_compatible" => ProviderEngine::OpenAI,
-                "anthropic_compatible" => ProviderEngine::Anthropic,
-                "ollama_compatible" => ProviderEngine::Ollama,
-                _ => return Err(anyhow::anyhow!("Invalid provider type: {}", params.engine)),
-            },
+            engine: ProviderEngine::from_str(&params.engine)?,
             display_name: params.display_name,
             description: existing_config.description,
             api_key_env,
@@ -311,7 +373,7 @@ pub fn update_custom_provider(params: UpdateCustomProviderParams) -> Result<()>
             fast_model: existing_config.fast_model.clone(),
         };
 
-        let file_path = custom_providers_dir().join(format!("{}.json", updated_config.name));
+        let file_path = custom_provider_file_path(&updated_config.name)?;
         let json_content = serde_json::to_string_pretty(&updated_config)?;
         std::fs::write(file_path, json_content)?;
     }
@@ -320,11 +382,13 @@ pub fn update_custom_provider(params: UpdateCustomProviderParams) -> Result<()>
 
 pub fn remove_custom_provider(id: &str) -> Result<()> {
     let config = Config::global();
-    let api_key_name = generate_api_key_name(id);
-    let _ = config.delete_secret(&api_key_name);
+    let loaded_provider = load_provider(id)?;
+    let api_key_env = loaded_provider.config.api_key_env;
+    if api_key_env == generate_api_key_name(id) {
+        let _ = config.delete_secret(&api_key_env);
+    }
 
-    let custom_providers_dir = custom_providers_dir();
-    let file_path = custom_providers_dir.join(format!("{}.json", id));
+    let file_path = custom_provider_file_path(id)?;
 
     if file_path.exists() {
         std::fs::remove_file(file_path)?;
@@ -334,7 +398,7 @@ pub fn remove_custom_provider(id: &str) -> Result<()> {
 }
 
 pub fn load_provider(id: &str) -> Result<LoadedProvider> {
-    let custom_file_path = custom_providers_dir().join(format!("{}.json", id));
+    let custom_file_path = custom_provider_file_path(id)?;
 
     if custom_file_path.exists() {
         let content = std::fs::read_to_string(&custom_file_path)?;
@@ -624,6 +688,79 @@ mod tests {
         assert_eq!(config.models[0].context_limit, 131072);
     }
 
+    #[test]
+    fn test_validate_provider_id_rejects_legacy_punctuation_for_new_ids() {
+        assert!(validate_provider_id("custom_z.ai").is_err());
+    }
+
+    fn write_legacy_provider_config(id: &str, display_name: &str) {
+        let custom_dir = custom_providers_dir();
+        std::fs::create_dir_all(&custom_dir).unwrap();
+        let content = format!(
+            r#"{{
+  "name": "{id}",
+  "engine": "openai",
+  "display_name": "{display_name}",
+  "description": "legacy provider",
+  "api_key_env": "",
+  "base_url": "https://example.invalid/v1/chat/completions",
+  "models": [],
+  "requires_auth": false
+}}"#
+        );
+        std::fs::write(custom_dir.join(format!("{id}.json")), content).unwrap();
+    }
+
+    #[test]
+    fn test_load_provider_allows_legacy_custom_id_with_punctuation() {
+        let temp_dir = tempfile::tempdir().unwrap();
+        let temp_root = temp_dir.path().display().to_string();
+        let _guard = env_lock::lock_env([("GOOSE_PATH_ROOT", Some(temp_root.as_str()))]);
+
+        write_legacy_provider_config("custom_z.ai", "Z.AI");
+
+        let loaded = load_provider("custom_z.ai").unwrap();
+        assert!(loaded.is_editable);
+        assert_eq!(loaded.config.name, "custom_z.ai");
+    }
+
+    #[test]
+    fn test_update_and_remove_provider_allow_legacy_custom_id_with_punctuation() {
+        let temp_dir = tempfile::tempdir().unwrap();
+        let temp_root = temp_dir.path().display().to_string();
+        let _guard = env_lock::lock_env([("GOOSE_PATH_ROOT", Some(temp_root.as_str()))]);
+
+        write_legacy_provider_config("custom_z.ai", "Z.AI");
+
+        update_custom_provider(UpdateCustomProviderParams {
+            id: "custom_z.ai".to_string(),
+            engine: "openai".to_string(),
+            display_name: "Z.AI Updated".to_string(),
+            api_url: "https://updated.example.invalid/v1/chat/completions".to_string(),
+            api_key: None,
+            models: vec!["z-model".to_string()],
+            supports_streaming: Some(true),
+            headers: None,
+            requires_auth: false,
+            catalog_provider_id: None,
+            base_path: None,
+        })
+        .unwrap();
+
+        let updated = load_provider("custom_z.ai").unwrap();
+        assert_eq!(updated.config.display_name, "Z.AI Updated");
+        assert_eq!(updated.config.models[0].name, "z-model");
+
+        remove_custom_provider("custom_z.ai").unwrap();
+        assert!(!custom_providers_dir().join("custom_z.ai.json").exists());
+    }
+
+    #[test]
+    fn test_load_provider_rejects_path_segments() {
+        assert!(load_provider("custom_../secret").is_err());
+        assert!(load_provider("custom_..\\secret").is_err());
+    }
+
     #[test]
     fn test_expand_env_vars_replaces_placeholder() {
         let _guard = env_lock::lock_env([("TEST_EXPAND_HOST", Some("https://example.com/api"))]);