Skip to content

Fix: deduplicate custom providers in UI and include config keys for custom providers #4514

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.

Sign up for GitHub

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

Closed
cbruyndoncx wants to merge 27 commits into from
Closed

Fix: deduplicate custom providers in UI and include config keys for custom providers #4514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
71d137c
no more duplicate custom providers in ui
cbruyndoncx Sep 4, 2025
3b967bc
ui(desktop): reload provider config values when provider changes
cbruyndoncx Sep 4, 2025
08c20e8
Merge branch 'main' into fix-duplicate-provider-in-ui
cbruyndoncx Sep 5, 2025
964e33d
custom providers: use per-provider base_url config key to avoid keych…
cbruyndoncx Sep 5, 2025
743aee3
providers: log response URL and body on non-success to help debug LLM…
cbruyndoncx Sep 5, 2025
fd8dfd1
providers: log LLM request payload and outgoing URL for better debug …
cbruyndoncx Sep 5, 2025
9df1534
providers: include request payload and provider hint in response logg…
cbruyndoncx Sep 5, 2025
a54665c
custom providers: warn on legacy shared CUSTOM_PROVIDER_BASE_URL in s…
cbruyndoncx Sep 5, 2025
1432632
custom providers: auto-remove legacy CUSTOM_PROVIDER_BASE_URL secret …
cbruyndoncx Sep 5, 2025
1f6989b
server: add UpdateCustomProviderRequest and prep for provider update …
cbruyndoncx Sep 5, 2025
ba329db
server: add update_custom_provider endpoint to update JSON and secret…
cbruyndoncx Sep 5, 2025
c399e24
ui: POST custom provider edits to server JSON (typed payload)
cbruyndoncx Sep 5, 2025
75c43aa
ui: ensure custom provider submit builds payload and validates non-em…
cbruyndoncx Sep 5, 2025
c268b0c
ui: call server PUT using electron-provided host/port and ensure URL …
cbruyndoncx Sep 5, 2025
ea20067
ui: custom provider PUT uses electron config for server base URL; imp…
cbruyndoncx Sep 5, 2025
309f53d
server: log update_custom_provider payload for debugging
cbruyndoncx Sep 5, 2025
bc58e0d
desktop(ui): show expandable success toast after updating custom prov…
cbruyndoncx Sep 6, 2025
a9a33ca
desktop(ui): include server response in provider update flow; show ex…
cbruyndoncx Sep 6, 2025
dff64b3
desktop(ui): await refreshProviders from modal onSubmit to ensure det…
cbruyndoncx Sep 6, 2025
32b38e0
modal: keep provider-update toast from auto-closing while details are…
cbruyndoncx Sep 6, 2025
3a97924
formats/openai: support delta.name and delta.refusal; accept SSE and …
cbruyndoncx Sep 6, 2025
64b3168
formats/openai: log raw stream lines for debugging
cbruyndoncx Sep 6, 2025
ff18ec5
secrets: UI sentinel and server guard; wire sentinel into provider fo…
cbruyndoncx Sep 6, 2025
72c3c2c
show all provider config params and sentinel behavior
cbruyndoncx Sep 6, 2025
8402be0
ui: show all provider config keys for custom providers; pass field-ch…
cbruyndoncx Sep 6, 2025
53bc1ba
providers(openai): yield structured streaming error message to UI ins…
cbruyndoncx Sep 6, 2025
5f72bdc
server(reply): emit an Error SSE event when we yield an OpenAI stream…
cbruyndoncx Sep 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 101 additions & 60 deletions crates/goose-server/src/routes/config_management.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::routes::utils::check_provider_configured;
use crate::state::AppState;
use axum::{
extract::{Path, State},
routing::{delete, get, post},
routing::{delete, get, post, put},
Json, Router,
};
use etcetera::{choose_app_strategy, AppStrategy};
Expand Down Expand Up @@ -87,6 +87,15 @@ pub struct CreateCustomProviderRequest {
pub supports_streaming: Option<bool>,
}

#[derive(Deserialize, Serialize, ToSchema)]
pub struct UpdateCustomProviderRequest {
pub display_name: Option<String>,
pub api_url: Option<String>,
pub api_key: Option<String>,
pub models: Option<Vec<String>>,
pub supports_streaming: Option<bool>,
}

#[utoipa::path(
post,
path = "/config/upsert",
Expand All @@ -104,6 +113,18 @@ pub async fn upsert_config(
verify_secret_key(&headers, &state)?;

let config = Config::global();

// Defensive guard: if this is a secret check and the client sent a boolean 'true' to
// indicate presence, do not write that boolean into the secret storage. Treat as a no-op.
if query.is_secret {
if let Value::Bool(b) = &query.value {
if *b {
tracing::info!(key = %query.key, "Skipping upsert for secret key with boolean true (presence-only)");
return Ok(Json(Value::String(format!("Skipped secret upsert for {} (presence-only)", query.key))));
}
}
}

let result = config.set(&query.key, query.value, query.is_secret);

match result {
Expand Down Expand Up @@ -316,64 +337,7 @@ pub async fn providers(
) -> Result<Json<Vec<ProviderDetails>>, StatusCode> {
verify_secret_key(&headers, &state)?;

let mut providers_metadata = get_providers();

let custom_providers_dir = goose::config::custom_providers::custom_providers_dir();

if custom_providers_dir.exists() {
if let Ok(entries) = std::fs::read_dir(&custom_providers_dir) {
for entry in entries.flatten() {
if let Some(extension) = entry.path().extension() {
if extension == "json" {
if let Ok(content) = std::fs::read_to_string(entry.path()) {
if let Ok(custom_provider) = serde_json::from_str::<
goose::config::custom_providers::CustomProviderConfig,
>(&content)
{
// CustomProviderConfig => ProviderMetadata
let default_model = custom_provider
.models
.first()
.map(|m| m.name.clone())
.unwrap_or_default();

let metadata = goose::providers::base::ProviderMetadata {
name: custom_provider.name.clone(),
display_name: custom_provider.display_name.clone(),
description: custom_provider
.description
.clone()
.unwrap_or_else(|| {
format!("{} (custom)", custom_provider.display_name)
}),
default_model,
known_models: custom_provider.models.clone(),
model_doc_link: "Custom provider".to_string(),
config_keys: vec![
goose::providers::base::ConfigKey::new(
&custom_provider.api_key_env,
true,
true,
None,
),
goose::providers::base::ConfigKey::new(
"CUSTOM_PROVIDER_BASE_URL",
true,
false,
Some(&custom_provider.base_url),
),
],
};
providers_metadata.push(metadata);
}
}
}
}
}
}
}

let providers_response: Vec<ProviderDetails> = providers_metadata
let providers_response: Vec<ProviderDetails> = get_providers()
.into_iter()
.map(|metadata| {
let is_configured = check_provider_configured(&metadata);
Expand Down Expand Up @@ -766,6 +730,8 @@ pub async fn get_current_model(

#[utoipa::path(
post,


path = "/config/custom-providers",
request_body = CreateCustomProviderRequest,
responses(
Expand Down Expand Up @@ -798,6 +764,81 @@ pub async fn create_custom_provider(
Ok(Json(format!("Custom provider added - ID: {}", config.id())))
}

#[utoipa::path(
put,
path = "/config/custom-providers/{id}",
request_body = UpdateCustomProviderRequest,
responses(
(status = 200, description = "Custom provider updated successfully", body = String),
(status = 400, description = "Invalid request"),
(status = 404, description = "Provider not found"),
(status = 500, description = "Internal server error")
)
)]
pub async fn update_custom_provider(
State(state): State<Arc<AppState>>,
headers: HeaderMap,
axum::extract::Path(id): axum::extract::Path<String>,
Json(request): Json<UpdateCustomProviderRequest>,
) -> Result<Json<String>, StatusCode> {
verify_secret_key(&headers, &state)?;

// Log incoming update for debugging
let payload_value = serde_json::to_value(&request).unwrap_or(serde_json::Value::Null);
tracing::info!(id = %id, payload = ?payload_value, "update_custom_provider called");

let custom_providers_dir = goose::config::custom_providers::custom_providers_dir();
let file_path = custom_providers_dir.join(format!("{}.json", id));

if !file_path.exists() {
return Err(StatusCode::NOT_FOUND);
}

// Read existing config
let content = std::fs::read_to_string(&file_path).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
let mut config: goose::config::custom_providers::CustomProviderConfig =
serde_json::from_str(&content).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;

// Update fields if provided
if let Some(display_name) = request.display_name {
config.display_name = display_name;
}
if let Some(api_url) = request.api_url {
config.base_url = api_url;
}
if let Some(models) = request.models {
config.models = models
.into_iter()
.map(|m| goose::providers::base::ModelInfo::new(m, 128000))
.collect();
}
if let Some(s) = request.supports_streaming {
config.supports_streaming = Some(s);
}

// Persist API key into secrets if provided
if let Some(api_key) = request.api_key {
let cfg = goose::config::Config::global();
if let Err(e) = cfg.set_secret(&config.api_key_env, serde_json::Value::String(api_key)) {
tracing::error!("Failed to set secret for {}: {}", config.api_key_env, e);
return Err(StatusCode::INTERNAL_SERVER_ERROR);
}
}

// Save updated JSON atomically
let tmp = file_path.with_extension("json.tmp");
let json_content = serde_json::to_string_pretty(&config).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
std::fs::write(&tmp, &json_content).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
std::fs::rename(&tmp, &file_path).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;

// Refresh in-memory providers
if let Err(e) = goose::providers::refresh_custom_providers() {
tracing::warn!("Failed to refresh custom providers after update: {}", e);
}

Ok(Json(format!("Updated custom provider: {}", id)))
}

#[utoipa::path(
delete,
path = "/config/custom-providers/{id}",
Expand Down Expand Up @@ -845,7 +886,7 @@ pub fn routes(state: Arc<AppState>) -> Router {
.route("/config/custom-providers", post(create_custom_provider))
.route(
"/config/custom-providers/{id}",
delete(remove_custom_provider),
delete(remove_custom_provider).put(update_custom_provider),
)
.with_state(state)
}
Expand Down
37 changes: 32 additions & 5 deletions crates/goose-server/src/routes/reply.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,14 +265,17 @@ async fn reply_handler(
Ok(stream) => stream,
Err(e) => {
tracing::error!("Failed to start reply stream: {:?}", e);
stream_event(
MessageEvent::Error {
error: e.to_string(),
},
let err_text = e.to_string();
// send Error event (for telemetry / UI error handling)
let _ = stream_event(
MessageEvent::Error { error: err_text.clone() },
&task_tx,
&cancel_token,
)
.await;
// also send a visible assistant message so the UI shows it inline
let assistant_msg = Message::assistant().with_text(format!("Provider error: {}", err_text));
let _ = stream_event(MessageEvent::Message { message: assistant_msg }, &task_tx, &cancel_token).await;
return;
}
};
Expand Down Expand Up @@ -312,8 +315,32 @@ async fn reply_handler(
track_tool_telemetry(content, all_messages.messages());
}

// Push and send the message event
all_messages.push(message.clone());
stream_event(MessageEvent::Message { message }, &tx, &cancel_token).await;
stream_event(MessageEvent::Message { message: message.clone() }, &tx, &cancel_token).await;

// If this message appears to be a provider streaming error produced by
// the OpenAI-format streaming handler, surface an Error event as
// well so the frontend's error handling path runs and the UI
// visibly shows the failure. We detect this by looking for the
// distinctive prefix we emit when streaming errors are encountered.
if let Some(first_text) = message
.content
.iter()
.find_map(|c| match c {
goose::conversation::message::MessageContent::Text(t) => Some(t.text.clone()),
_ => None,
})
.filter(|s| s.starts_with("LLM streaming error encountered"))
{
// Send a short error string (avoid flooding the SSE with huge payloads)
let short = if first_text.len() > 1024 {
format!("{}...", &first_text[..1024])
} else {
first_text
};
let _ = stream_event(MessageEvent::Error { error: short }, &tx, &cancel_token).await;
}
}
Ok(Some(Ok(AgentEvent::HistoryReplaced(new_messages)))) => {
// Replace the message history with the compacted messages
Expand Down
59 changes: 59 additions & 0 deletions crates/goose/src/config/custom_providers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,37 @@ pub fn register_custom_providers(
) -> Result<()> {
let configs = load_custom_providers(dir)?;

// Detect legacy shared key usage in keyring/config that could override
// per-provider base URLs. Historically a single key name
// "CUSTOM_PROVIDER_BASE_URL" was used for all providers which could
// result in cross-provider collisions if stored in the keyring. If that
// legacy key exists, log a warning so operators can remove/migrate it.
let global_config = crate::config::Config::global();
if let Ok(val) = global_config.get_secret::<String>("CUSTOM_PROVIDER_BASE_URL") {
tracing::warn!("Detected legacy shared key 'CUSTOM_PROVIDER_BASE_URL' in secret storage. This can cause custom providers to use the wrong base_url. Value: {}.", val);
// Attempt to remove the legacy shared key from secret storage. This is
// a best-effort cleanup to prevent custom providers from picking up a
// wrong global base_url. If deletion fails, log the error and continue
// without panicking.
match global_config.delete_secret("CUSTOM_PROVIDER_BASE_URL") {
Ok(_) => tracing::info!(
"Removed legacy secret key 'CUSTOM_PROVIDER_BASE_URL' from secret storage."
),
Err(e) => tracing::error!(
"Failed to remove legacy secret key 'CUSTOM_PROVIDER_BASE_URL': {}",
e
),
}
}

for config in configs {
let config_clone = config.clone();
// Use a unique base URL key per custom provider to avoid collisions in the
// global config/keyring. Previously this used the constant
// "CUSTOM_PROVIDER_BASE_URL" for every provider which caused different
// providers to read/write the same key and mix up values stored in the
// keyring or config file.
let base_url_key = format!("{}_BASE_URL", config.name.to_uppercase());
let description = config
.description
.clone()
Expand All @@ -174,36 +203,66 @@ pub fn register_custom_providers(

match config.engine {
ProviderEngine::OpenAI => {
let config_keys = vec![
crate::providers::base::ConfigKey::new(&config.api_key_env, true, true, None),
crate::providers::base::ConfigKey::new(
&base_url_key,
true,
false,
Some(&config.base_url),
),
];
registry.register_with_name::<OpenAiProvider, _>(
config.name.clone(),
config.display_name.clone(),
description,
default_model,
known_models,
config_keys,
move |model: ModelConfig| {
OpenAiProvider::from_custom_config(model, config_clone.clone())
},
);
}
ProviderEngine::Ollama => {
let config_keys = vec![
crate::providers::base::ConfigKey::new(&config.api_key_env, true, true, None),
crate::providers::base::ConfigKey::new(
&base_url_key,
true,
false,
Some(&config.base_url),
),
];
registry.register_with_name::<OllamaProvider, _>(
config.name.clone(),
config.display_name.clone(),
description,
default_model,
known_models,
config_keys,
move |model: ModelConfig| {
OllamaProvider::from_custom_config(model, config_clone.clone())
},
);
}
ProviderEngine::Anthropic => {
let config_keys = vec![
crate::providers::base::ConfigKey::new(&config.api_key_env, true, true, None),
crate::providers::base::ConfigKey::new(
&base_url_key,
true,
false,
Some(&config.base_url),
),
];
registry.register_with_name::<AnthropicProvider, _>(
config.name.clone(),
config.display_name.clone(),
description,
default_model,
known_models,
config_keys,
move |model: ModelConfig| {
AnthropicProvider::from_custom_config(model, config_clone.clone())
},
Expand Down
Loading