diff --git a/crates/ironclaw_gateway/static/i18n/en.js b/crates/ironclaw_gateway/static/i18n/en.js index 3f48d48d01..f99e4e1f74 100644 --- a/crates/ironclaw_gateway/static/i18n/en.js +++ b/crates/ironclaw_gateway/static/i18n/en.js @@ -28,6 +28,7 @@ I18n.register('en', { 'restart.progressTitle': 'Restarting IronClaw', 'restart.progressSubtitle': 'Please wait for the process to restart...', 'restart.checkLogs': 'Check the Logs tab for details after restart completes.', + 'restart.closeTooltip': 'Close', // Theme 'theme.tooltipDark': 'Theme: Dark (click for Light)', @@ -101,6 +102,7 @@ I18n.register('en', { 'status.reconnecting': 'Reconnecting...', 'status.teeVerified': 'TEE Verified', 'status.restart': 'Restart', + 'status.restartTooltip': 'Gracefully restart the process', 'status.active': 'Active', 'status.installed': 'Installed', 'status.awaitingPairing': 'Awaiting Pairing', @@ -457,6 +459,10 @@ I18n.register('en', { 'config.builtin': 'built-in', 'config.useProvider': 'Use', 'config.configureProvider': 'Configure', + 'config.notConfigured': 'Not Configured', + 'config.configureToUse': 'Configure the API key before using this provider.', + 'config.baseUrlRequired': 'Base URL is required. Please configure the provider first.', + 'config.modelRequired': 'A model must be configured before using this provider.', 'config.providerConfigured': 'Provider "{name}" configured (restart to apply)', 'config.currentModel': 'Model: {model}', 'config.providerName': 'Display Name', diff --git a/crates/ironclaw_gateway/static/i18n/ko.js b/crates/ironclaw_gateway/static/i18n/ko.js index eaf69a275c..6aaaa1b80e 100644 --- a/crates/ironclaw_gateway/static/i18n/ko.js +++ b/crates/ironclaw_gateway/static/i18n/ko.js @@ -28,6 +28,7 @@ I18n.register('ko', { 'restart.progressTitle': 'IronClaw 재시작 중', 'restart.progressSubtitle': '프로세스가 재시작될 때까지 기다려 주세요...', 'restart.checkLogs': '재시작이 완료된 후 자세한 내용은 로그 탭을 확인하세요.', + 'restart.closeTooltip': '닫기', // 테마 'theme.tooltipDark': '테마: 다크 (클릭하여 라이트로 변경)', @@ -101,6 +102,7 @@ I18n.register('ko', { 'status.reconnecting': '재연결 중...', 'status.teeVerified': 'TEE 검증됨', 'status.restart': '재시작', + 'status.restartTooltip': '프로세스를 정상적으로 재시작합니다', 'status.active': '활성', 'status.installed': '설치됨', 'status.awaitingPairing': '페어링 대기 중', @@ -456,6 +458,10 @@ I18n.register('ko', { 'config.builtin': '내장', 'config.useProvider': '사용', 'config.configureProvider': '구성', + 'config.notConfigured': '미구성', + 'config.configureToUse': '이 공급자를 사용하기 전에 API 키를 구성하세요.', + 'config.baseUrlRequired': '베이스 URL이 필요합니다. 먼저 공급자를 구성하세요.', + 'config.modelRequired': '이 공급자를 사용하기 전에 모델을 구성해야 합니다.', 'config.providerConfigured': '공급자 "{name}"이(가) 구성되었습니다 (재시작 필요)', 'config.currentModel': '모델: {model}', 'config.providerName': '표시 이름', diff --git a/crates/ironclaw_gateway/static/i18n/zh-CN.js b/crates/ironclaw_gateway/static/i18n/zh-CN.js index 2b26595177..f70c06bb79 100644 --- a/crates/ironclaw_gateway/static/i18n/zh-CN.js +++ b/crates/ironclaw_gateway/static/i18n/zh-CN.js @@ -28,6 +28,7 @@ I18n.register('zh-CN', { 'restart.progressTitle': '正在重启 IronClaw', 'restart.progressSubtitle': '请等待进程重启...', 'restart.checkLogs': '重启完成后,请查看日志标签页了解详情。', + 'restart.closeTooltip': '关闭', // 主题 'theme.tooltipDark': '主题:深色(点击切换浅色)', @@ -101,6 +102,7 @@ I18n.register('zh-CN', { 'status.reconnecting': '重新连接中...', 'status.teeVerified': 'TEE 已验证', 'status.restart': '重启', + 'status.restartTooltip': '优雅地重启进程', 'status.active': '已激活', 'status.installed': '已安装', 'status.awaitingPairing': '等待配对', @@ -456,6 +458,10 @@ I18n.register('zh-CN', { 'config.builtin': '内置', 'config.useProvider': '使用', 'config.configureProvider': '配置', + 'config.notConfigured': '未配置', + 'config.configureToUse': '请先配置 API 密钥后再使用此提供商。', + 'config.baseUrlRequired': '需要配置基础 URL,请先配置此提供商。', + 'config.modelRequired': '使用此提供商前需要配置模型。', 'config.providerConfigured': '提供商 "{name}" 已配置(重启后生效)', 'config.currentModel': '模型:{model}', 'config.providerName': '显示名称', diff --git a/crates/ironclaw_gateway/static/js/surfaces/config.js b/crates/ironclaw_gateway/static/js/surfaces/config.js index 4b20b47d0e..a3dea15f76 100644 --- a/crates/ironclaw_gateway/static/js/surfaces/config.js +++ b/crates/ironclaw_gateway/static/js/surfaces/config.js @@ -60,6 +60,98 @@ function scrollToProviders() { if (section) section.scrollIntoView({ behavior: 'smooth', block: 'start' }); } +/** Check whether a provider has all required credentials (API key + base URL if required). */ +function isProviderConfigured(provider) { + // ── API key check ────────────────────────────────────────────────────── + // Built-in providers carry `api_key_required` from the backend registry. + // Custom providers don't — derive the requirement from the adapter instead: + // ollama runs locally and needs no key; other adapters do. + const needsKey = provider.builtin + ? provider.api_key_required !== false + : provider.adapter !== 'ollama'; + const hasEnvKey = provider.has_api_key === true; + const overrideKey = provider.builtin && _builtinOverrides[provider.id] + ? _builtinOverrides[provider.id].api_key + : undefined; + // For custom providers, `api_key` is either the sentinel (vaulted on the + // server) OR a freshly-entered plaintext string that hasn't been swapped + // for the sentinel yet. Both mean the provider is configured. + const customKey = !provider.builtin ? provider.api_key : undefined; + const hasDbKey = provider.builtin + ? (overrideKey === API_KEY_UNCHANGED || (typeof overrideKey === 'string' && overrideKey.length > 0)) + : (customKey === API_KEY_UNCHANGED || (typeof customKey === 'string' && customKey.length > 0)); + const keyOk = !needsKey || hasEnvKey || hasDbKey; + if (!keyOk) return false; + + // ── Base URL check ───────────────────────────────────────────────────── + // Built-ins with `base_url_required` (e.g. openai_compatible) have no + // hardcoded fallback in the client layer, so activation must be gated on + // having a URL from SOME source. Custom providers always need a URL + // because they have no default at all. + const needsBaseUrl = provider.builtin + ? provider.base_url_required === true + : true; + if (!needsBaseUrl) return true; + + const overrideBaseUrl = provider.builtin && _builtinOverrides[provider.id] + ? _builtinOverrides[provider.id].base_url + : undefined; + const hasOverrideBaseUrl = typeof overrideBaseUrl === 'string' && overrideBaseUrl.trim().length > 0; + const hasEnvBaseUrl = typeof provider.env_base_url === 'string' && provider.env_base_url.trim().length > 0; + // `provider.base_url` is the registry default for built-ins (may be empty + // when base_url_required=true and there's no default) OR the user-set URL + // for custom providers. + const hasProviderBaseUrl = typeof provider.base_url === 'string' && provider.base_url.trim().length > 0; + return hasOverrideBaseUrl || hasEnvBaseUrl || hasProviderBaseUrl; +} + +/** + * Determine what's missing on an unconfigured provider for a precise toast. + * Returns 'base_url' if the base URL is missing, 'api_key' if the key is + * missing, or 'ok' if nothing is missing. Mirrors the checks in + * isProviderConfigured — keep the two in sync. + */ +function providerMissingReason(provider) { + // API key check — matches isProviderConfigured above. + const needsKey = provider.builtin + ? provider.api_key_required !== false + : provider.adapter !== 'ollama'; + if (needsKey) { + const hasEnvKey = provider.has_api_key === true; + const overrideKey = provider.builtin && _builtinOverrides[provider.id] + ? _builtinOverrides[provider.id].api_key + : undefined; + const customKey = !provider.builtin ? provider.api_key : undefined; + const hasDbKey = provider.builtin + ? (overrideKey === API_KEY_UNCHANGED || (typeof overrideKey === 'string' && overrideKey.length > 0)) + : (customKey === API_KEY_UNCHANGED || (typeof customKey === 'string' && customKey.length > 0)); + if (!hasEnvKey && !hasDbKey) return 'api_key'; + } + // Base URL check — matches isProviderConfigured above. + const needsBaseUrl = provider.builtin + ? provider.base_url_required === true + : true; + if (needsBaseUrl) { + const overrideBaseUrl = provider.builtin && _builtinOverrides[provider.id] + ? _builtinOverrides[provider.id].base_url + : undefined; + const hasOverrideBaseUrl = typeof overrideBaseUrl === 'string' && overrideBaseUrl.trim().length > 0; + const hasEnvBaseUrl = typeof provider.env_base_url === 'string' && provider.env_base_url.trim().length > 0; + const hasProviderBaseUrl = typeof provider.base_url === 'string' && provider.base_url.trim().length > 0; + if (!hasOverrideBaseUrl && !hasEnvBaseUrl && !hasProviderBaseUrl) return 'base_url'; + } + return 'ok'; +} + +/** Open the appropriate configuration dialog for a provider. */ +function openProviderConfigDialog(provider) { + if (provider.builtin && provider.id !== 'bedrock') { + configureBuiltinProvider(provider.id); + } else if (!provider.builtin) { + editCustomProvider(provider.id); + } +} + function renderProviders() { const list = document.getElementById('providers-list'); const allProviders = [..._builtinProviders, ..._customProviders].sort((a, b) => { @@ -76,12 +168,16 @@ function renderProviders() { list.innerHTML = allProviders.map((p) => { const isActive = p.id === _activeLlmBackend; const adapterLabel = ADAPTER_LABELS[p.adapter] || p.adapter; + const isConfigured = isProviderConfigured(p); const activeBadge = isActive ? '' + I18n.t('status.active') + '' : ''; const builtinBadge = p.builtin ? '' + I18n.t('config.builtin') + '' : ''; + const unconfiguredBadge = !isActive && !isConfigured + ? '' + I18n.t('config.notConfigured') + '' + : ''; const deleteBtn = !p.builtin && !isActive ? '' : ''; @@ -92,7 +188,8 @@ function renderProviders() { const configureBtn = p.builtin && p.id !== 'bedrock' ? '' : ''; - const useBtn = !isActive + // Only show "Use" if provider is configured; unconfigured providers must be configured first + const useBtn = !isActive && isConfigured ? '' : ''; const overrideBaseUrl = p.builtin && _builtinOverrides[p.id] ? (_builtinOverrides[p.id].base_url || '') : ''; @@ -113,7 +210,7 @@ function renderProviders() { + '
' + '' + escapeHtml(p.name || p.id) + '' + '' + escapeHtml(p.id) + '' - + activeBadge + builtinBadge + + activeBadge + builtinBadge + unconfiguredBadge + '
' + '
' + '' + escapeHtml(adapterLabel) + '' @@ -129,12 +226,28 @@ function renderProviders() { function setActiveProvider(id) { const provider = [..._builtinProviders, ..._customProviders].find((p) => p.id === id); + if (provider && !isProviderConfigured(provider)) { + // Pick a specific message so the user knows WHAT is missing, not just + // "configure the provider". Check base URL first because a provider + // that needs both a key and a URL typically surfaces URL entry first + // in the dialog layout. + const reason = providerMissingReason(provider); + const toastKey = reason === 'base_url' ? 'config.baseUrlRequired' : 'config.configureToUse'; + showToast(I18n.t(toastKey), 'error'); + openProviderConfigDialog(provider); + return; + } // Restore the last-configured model for this provider, falling back to the provider's default - const restoredModel = - (_builtinOverrides[id] && _builtinOverrides[id].model) || - (provider && provider.default_model) || - null; + const overrideModel = _builtinOverrides[id] && _builtinOverrides[id].model; + const envModel = provider && provider.env_model; + const restoredModel = overrideModel || envModel || (provider && provider.default_model) || null; const defaultModel = restoredModel; + // Guard: a model must be available + if (!defaultModel) { + showToast(I18n.t('config.modelRequired') || 'Model is required', 'error'); + if (provider) openProviderConfigDialog(provider); + return; + } const modelUpdate = () => defaultModel ? apiFetchVoid('/api/settings/selected_model', { method: 'PUT', body: { value: defaultModel } }) : apiFetchVoid('/api/settings/selected_model', { method: 'DELETE' }); diff --git a/crates/ironclaw_gateway/static/styles/surfaces/config.css b/crates/ironclaw_gateway/static/styles/surfaces/config.css index 74811a1f68..43d9b0f74d 100644 --- a/crates/ironclaw_gateway/static/styles/surfaces/config.css +++ b/crates/ironclaw_gateway/static/styles/surfaces/config.css @@ -105,6 +105,11 @@ color: var(--text-secondary); } +.provider-badge-unconfigured { + background: rgba(251, 191, 36, 0.15); + color: #b45309; +} + .provider-card-meta { display: flex; align-items: center; diff --git a/src/channels/web/features/settings/mod.rs b/src/channels/web/features/settings/mod.rs index 1bbb91ac4d..3191868e7c 100644 --- a/src/channels/web/features/settings/mod.rs +++ b/src/channels/web/features/settings/mod.rs @@ -421,8 +421,11 @@ fn is_valid_provider_id(id: &str) -> bool { .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'-' || b == b'_') } -/// Returns `Err(422)` if any provider has an invalid ID or unrecognised adapter. +/// Returns `Err(422)` if any provider has an invalid ID, unrecognised adapter, +/// or a base URL that fails SSRF validation. fn validate_custom_providers(value: &serde_json::Value) -> Result<(), StatusCode> { + use crate::config::helpers::validate_operator_base_url; + let providers = match value.as_array() { Some(arr) => arr, None => return Ok(()), @@ -445,6 +448,14 @@ fn validate_custom_providers(value: &serde_json::Value) -> Result<(), StatusCode tracing::warn!(id = %id, adapter = %adapter, "Rejected unknown LLM adapter"); return Err(StatusCode::UNPROCESSABLE_ENTITY); } + // Validate base_url at save time to reject SSRF-unsafe URLs early. + if let Some(base_url) = p.get("base_url").and_then(|v| v.as_str()) + && !base_url.is_empty() + && let Err(e) = validate_operator_base_url(base_url, "base_url") + { + tracing::warn!(id = %id, base_url = %base_url, error = %e, "Rejected custom provider with invalid base URL"); + return Err(StatusCode::UNPROCESSABLE_ENTITY); + } } Ok(()) } @@ -1567,6 +1578,52 @@ mod tests { assert!(validate_custom_providers(&input).is_ok()); } + #[test] + fn test_validate_custom_providers_rejects_unsafe_base_url() { + // Cloud metadata endpoint — must be rejected at save time. + let input = serde_json::json!([{ + "id": "evil", + "adapter": "open_ai_completions", + "base_url": "https://169.254.169.254/latest/meta-data" + }]); + assert_eq!( + validate_custom_providers(&input).unwrap_err(), + StatusCode::UNPROCESSABLE_ENTITY, + ); + } + + #[test] + fn test_validate_custom_providers_accepts_valid_base_url() { + // Use a URL that passes operator policy without DNS resolution + // (localhost is always allowed, even in sandboxed CI environments). + let input = serde_json::json!([{ + "id": "my-llm", + "adapter": "open_ai_completions", + "base_url": "http://localhost:8080/v1" + }]); + assert!(validate_custom_providers(&input).is_ok()); + } + + #[test] + fn test_validate_custom_providers_allows_empty_base_url() { + // Empty base_url is accepted at save time so users can stage an + // incomplete config without losing it. It is NOT enforced during + // `LlmConfig::resolve_custom_provider` either (only a warning). + // What actually prevents such a config from being used at runtime: + // 1. Frontend activation guard (isProviderConfigured in + // static/js/surfaces/config.js blocks the "Use" button). + // 2. Startup fallback in `LlmConfig::resolve_with_fallback` + // (invoked from `Config::re_resolve_llm_with_secrets`) — + // demotes unusable custom providers to NearAI rather than + // crash-looping the instance (#2514). + let input = serde_json::json!([{ + "id": "my-llm", + "adapter": "open_ai_completions", + "base_url": "" + }]); + assert!(validate_custom_providers(&input).is_ok()); + } + #[test] fn test_admin_only_setting_keys_include_network_destinations() { assert!(is_admin_only_setting_key("llm_builtin_overrides")); diff --git a/src/channels/web/handlers/llm.rs b/src/channels/web/handlers/llm.rs index 3e000c014c..f8651ef7b8 100644 --- a/src/channels/web/handlers/llm.rs +++ b/src/channels/web/handlers/llm.rs @@ -143,50 +143,7 @@ fn interpret_chat_response( result: Result, ) -> TestConnectionResponse { match result { - Ok(r) => { - let status = r.status(); - if status.is_success() { - TestConnectionResponse { - ok: true, - message: format!("Connected ({})", status), - } - } else if status == reqwest::StatusCode::UNAUTHORIZED - || status == reqwest::StatusCode::FORBIDDEN - { - TestConnectionResponse { - ok: false, - message: format!("Authentication failed ({})", status), - } - } else if status == reqwest::StatusCode::BAD_REQUEST - || status == reqwest::StatusCode::UNPROCESSABLE_ENTITY - { - // 400/422 = server reachable, likely wrong endpoint variant — connectivity OK - TestConnectionResponse { - ok: true, - message: format!("Server reachable ({})", status), - } - } else if status == reqwest::StatusCode::NOT_FOUND { - // 404 = /models endpoint not found — server reachable but not OpenAI-compatible - TestConnectionResponse { - ok: false, - message: format!( - "Server reachable but /models endpoint not found ({}). \ - Check the base URL and adapter type.", - status - ), - } - } else if status.is_client_error() { - TestConnectionResponse { - ok: false, - message: format!("Client error ({})", status), - } - } else { - TestConnectionResponse { - ok: false, - message: format!("Server error ({})", status), - } - } - } + Ok(r) => interpret_chat_status(r.status()), Err(e) => TestConnectionResponse { ok: false, message: format!("Connection failed: {e}"), @@ -194,6 +151,57 @@ fn interpret_chat_response( } } +/// Pure status-code interpretation, extracted for testability. +fn interpret_chat_status(status: reqwest::StatusCode) -> TestConnectionResponse { + if status.is_success() { + TestConnectionResponse { + ok: true, + message: format!("Connected ({})", status), + } + } else if status == reqwest::StatusCode::UNAUTHORIZED + || status == reqwest::StatusCode::FORBIDDEN + { + TestConnectionResponse { + ok: false, + message: format!("Authentication failed ({})", status), + } + } else if status == reqwest::StatusCode::BAD_REQUEST + || status == reqwest::StatusCode::UNPROCESSABLE_ENTITY + { + // 400/422 = server reachable but the request was rejected, likely a + // wrong model name or endpoint variant. Report as not-ok so the UI + // doesn't mislead the user with a green badge. + TestConnectionResponse { + ok: false, + message: format!( + "Server reachable but returned an error ({}). \ + Check the model name and adapter type.", + status + ), + } + } else if status == reqwest::StatusCode::NOT_FOUND { + // 404 = /models endpoint not found — server reachable but not OpenAI-compatible + TestConnectionResponse { + ok: false, + message: format!( + "Server reachable but /models endpoint not found ({}). \ + Check the base URL and adapter type.", + status + ), + } + } else if status.is_client_error() { + TestConnectionResponse { + ok: false, + message: format!("Client error ({})", status), + } + } else { + TestConnectionResponse { + ok: false, + message: format!("Server error ({})", status), + } + } +} + // --------------------------------------------------------------------------- // List models // --------------------------------------------------------------------------- @@ -404,6 +412,7 @@ fn build_llm_providers() -> serde_json::Value { serde_json::Value::String(crate::llm::DEFAULT_MODEL.to_string()), ); entry.insert("api_key_required".into(), true.into()); + entry.insert("base_url_required".into(), false.into()); entry.insert("can_list_models".into(), true.into()); // Env defaults entry.insert( @@ -446,6 +455,7 @@ fn build_llm_providers() -> serde_json::Value { serde_json::Value::String(def.default_model.clone()), ); entry.insert("api_key_required".into(), def.api_key_required.into()); + entry.insert("base_url_required".into(), def.base_url_required.into()); let can_list = def.setup.as_ref().is_some_and(|s| s.can_list_models()); entry.insert("can_list_models".into(), can_list.into()); // Env defaults @@ -476,6 +486,7 @@ fn build_llm_providers() -> serde_json::Value { "anthropic.claude-3-sonnet-20240229-v1:0".into(), ); entry.insert("api_key_required".into(), false.into()); + entry.insert("base_url_required".into(), false.into()); entry.insert("can_list_models".into(), false.into()); providers.push(serde_json::Value::Object(entry)); } @@ -639,9 +650,34 @@ mod tests { p.get("default_model").is_some(), "{id} missing default_model" ); + // api_key_required and base_url_required gate frontend activation — + // both must be present so isProviderConfigured() can reason about them. + assert!( + p.get("api_key_required").is_some(), + "{id} missing api_key_required" + ); + assert!( + p.get("base_url_required").is_some(), + "{id} missing base_url_required" + ); } } + #[tokio::test] + async fn test_openai_compatible_exposes_base_url_required_true() { + // Regression: openai_compatible has base_url_required=true (no default). + // The frontend needs this flag to gate activation on a configured URL. + let result = build_llm_providers(); + let arr = result.as_array().expect("should be an array"); + let oc = + find_provider(arr, "openai_compatible").expect("openai_compatible should be present"); + assert_eq!( + oc.get("base_url_required").and_then(|v| v.as_bool()), + Some(true), + "openai_compatible must advertise base_url_required=true so the UI gates activation" + ); + } + // --- is_nearai_private_endpoint tests --- #[test] @@ -673,6 +709,48 @@ mod tests { assert!(!is_nearai_private_endpoint("https://private.evil.com/v1")); } + // --- interpret_chat_status tests --- + + #[test] + fn test_interpret_chat_status_400_reports_not_ok() { + // Regression: 400 was previously reported as ok:true ("Server reachable"), + // which misled the UI into showing a green "connected" badge when the + // model name or endpoint was actually wrong. + let result = interpret_chat_status(reqwest::StatusCode::BAD_REQUEST); + assert!(!result.ok, "400 must not be reported as ok"); + assert!( + result.message.contains("400"), + "message should include status code" + ); + assert!( + result.message.contains("model name") || result.message.contains("adapter"), + "message should hint at model/adapter mismatch, got: {}", + result.message + ); + } + + #[test] + fn test_interpret_chat_status_422_reports_not_ok() { + let result = interpret_chat_status(reqwest::StatusCode::UNPROCESSABLE_ENTITY); + assert!(!result.ok, "422 must not be reported as ok"); + assert!(result.message.contains("422")); + } + + #[test] + fn test_interpret_chat_status_200_reports_ok() { + let result = interpret_chat_status(reqwest::StatusCode::OK); + assert!(result.ok, "200 should be reported as ok"); + } + + #[test] + fn test_interpret_chat_status_401_reports_auth_failed() { + let result = interpret_chat_status(reqwest::StatusCode::UNAUTHORIZED); + assert!(!result.ok); + assert!(result.message.contains("Authentication")); + } + + // --- Admin role + private base URL tests (staging) --- + #[tokio::test] async fn test_llm_test_connection_allows_admin_private_base_url() { use axum::body::Body; diff --git a/src/config/llm.rs b/src/config/llm.rs index 85b2222f3f..0e0a88cddc 100644 --- a/src/config/llm.rs +++ b/src/config/llm.rs @@ -78,6 +78,141 @@ impl LlmConfig { } } + /// Resolve LLM configuration, with NearAI fallback for unusable configs. + /// + /// This entry point is for the **final** resolve after secrets have been + /// hydrated from the encrypted store. If the user-configured backend is + /// not usable (missing API key, missing base URL), we fall back to NearAI + /// rather than crashing — this prevents the #2514 crash-loop when a user + /// activates a provider via the UI without completing all required + /// fields. + /// + /// Do NOT call this during early startup (`Config::build()`) when + /// secrets are not yet hydrated — use [`resolve`] instead, otherwise + /// the fallback fires spuriously and gets overridden by the later + /// re-resolve, spamming operators with misleading error logs. + pub(crate) fn resolve_with_fallback(settings: &Settings) -> Result { + match Self::resolve(settings) { + Ok(cfg) => { + if let Some(reason) = Self::unusable_reason(&cfg, settings) { + tracing::error!( + backend = %cfg.backend, + reason = %reason, + "Configured LLM backend is not usable. Falling back to NearAI default. \ + Reconfigure in Settings → Inference → Model Providers." + ); + Self::resolve_nearai_fallback(settings, &cfg.backend) + } else { + Ok(cfg) + } + } + Err(e) if Self::is_fallback_recoverable(&e) => { + tracing::error!( + error = %e, + configured_backend = ?settings.llm_backend, + "Failed to resolve configured LLM backend. Falling back to NearAI default. \ + Reconfigure in Settings → Inference → Model Providers." + ); + let attempted = settings + .llm_backend + .clone() + .unwrap_or_else(|| "".to_string()); + Self::resolve_nearai_fallback(settings, &attempted) + } + Err(e) => Err(e), + } + } + + /// If the resolved config's LLM provider is unusable, return a short + /// reason string. `None` means the config is fine to use. Returns `None` + /// for special backends that don't use the `provider` slot + /// (nearai/bedrock/codex/gemini_oauth) — those have their own validation + /// inside `resolve_once`. + /// + /// This check is deliberately narrow: it only flags configurations that + /// will certainly fail at runtime. Built-in providers like Anthropic may + /// legitimately have an empty `base_url` at config time because the + /// downstream rig-core client hardcodes the canonical endpoint. + fn unusable_reason(cfg: &Self, settings: &Settings) -> Option<&'static str> { + let provider = cfg.provider.as_ref()?; + + let is_custom = settings + .llm_custom_providers + .iter() + .any(|c| c.id == provider.provider_id); + let is_ollama = matches!(provider.protocol, ProviderProtocol::Ollama); + + // Custom providers have no hardcoded base URL in the client layer — + // an empty `base_url` here means requests will be sent to a bare + // path with no host, which always fails. + if is_custom && provider.base_url.trim().is_empty() { + return Some("missing base URL"); + } + + // Ollama runs locally and has no API key concept. Every other + // provider needs at least one form of authentication. + if !is_ollama + && provider.api_key.is_none() + && provider.oauth_token.is_none() + && provider.refresh_token.is_none() + { + return Some("missing API key"); + } + + None + } + + /// Errors that indicate a fixable user-config problem (as opposed to a + /// programming bug or environmental failure). We fall back to NearAI on + /// these so the instance can still start. + fn is_fallback_recoverable(err: &ConfigError) -> bool { + matches!(err, ConfigError::MissingRequired { .. }) + } + + /// Re-resolve with `llm_backend` forced to `"nearai"`. + /// `attempted_backend` is only used for logging so operators can see which + /// backend we bailed out of. + fn resolve_nearai_fallback( + settings: &Settings, + attempted_backend: &str, + ) -> Result { + let mut fallback = settings.clone(); + fallback.llm_backend = Some("nearai".to_string()); + // The previously-selected model was tied to the unusable backend + // (e.g. "openai/gpt-4o" for OpenRouter, "kimi-k2-turbo-preview" for + // a custom kimi provider). Sending it to NearAI would 404. Clear it + // so resolve_model falls through to NearAI's default. The DB sync in + // Config::re_resolve_llm_with_secrets deletes the row persistently; + // this keeps the in-memory config consistent for the current process. + fallback.selected_model = None; + let cfg = Self::resolve(&fallback).map_err(|e| { + tracing::error!( + attempted = %attempted_backend, + fallback_error = %e, + "NearAI fallback also failed to resolve — surfacing original error" + ); + e + })?; + tracing::warn!( + attempted = %attempted_backend, + active = %cfg.backend, + active_model = %cfg.nearai.model, + "Active LLM backend fell back to NearAI default due to unusable user config" + ); + Ok(cfg) + } + + /// Resolve LLM configuration without any fallback behavior. + /// + /// Returns the config exactly as computed from env/DB/defaults, with no + /// safety net for missing credentials. Use this for: + /// - Early startup (before secrets are hydrated), so a spurious fallback + /// doesn't fire and get overridden by the later re-resolve. + /// - Tests that verify pure resolution mechanics (model/base_url priority + /// chains, alias normalization, etc.). + /// + /// The top-level `AppBuilder` path calls [`resolve_with_fallback`] after + /// hydrating secrets, which handles #2514-style crash-loop prevention. pub(crate) fn resolve(settings: &Settings) -> Result { let registry = ProviderRegistry::load(); @@ -274,7 +409,7 @@ impl LlmConfig { .clone() .or(optional_env("BEDROCK_REGION")?); if explicit_region.is_none() { - tracing::info!("BEDROCK_REGION not set, defaulting to us-east-1"); + tracing::debug!("BEDROCK_REGION not set, defaulting to us-east-1"); } let region = explicit_region.unwrap_or_else(|| "us-east-1".to_string()); let model = Self::selected_model_override(settings) @@ -461,7 +596,7 @@ impl LlmConfig { custom: &crate::settings::CustomLlmProviderSettings, settings: &Settings, ) -> Result { - tracing::info!( + tracing::debug!( id = %custom.id, adapter = %custom.adapter, base_url = ?custom.base_url, @@ -2454,4 +2589,279 @@ mod tests { .expect("resolve should succeed for non-NearAI backend without NearAI URL validation"); assert_eq!(cfg.backend, "openai_compatible"); } + + // ── Fallback-to-NearAI tests (issue #2514) ───────────────────────────── + // + // When the user-configured backend is unusable (missing API key or base + // URL), `resolve()` must NOT propagate the incomplete config. It must + // fall back to NearAI so the instance can still start and the user can + // reach the Web UI to fix their config. Previously, activating Anthropic + // without an API key put the instance into a crash loop. + + #[test] + fn resolve_falls_back_to_nearai_when_registry_backend_missing_api_key() { + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::remove_var("ANTHROPIC_API_KEY"); + std::env::remove_var("ANTHROPIC_OAUTH_TOKEN"); + } + + let settings = Settings { + llm_backend: Some("anthropic".to_string()), + ..Default::default() + }; + + // resolve() would hand back an unusable anthropic config (no key); + // resolve_with_fallback() must notice that and fall back to NearAI. + let cfg = LlmConfig::resolve_with_fallback(&settings) + .expect("resolve should succeed via fallback"); + assert_eq!( + cfg.backend, "nearai", + "unusable anthropic backend must fall back to NearAI" + ); + assert!( + cfg.provider.is_none(), + "NearAI fallback should not populate provider slot" + ); + } + + #[test] + fn resolve_fallback_clears_stale_selected_model() { + // Regression: when the user-configured backend (e.g. openrouter) was + // paired with a selected_model tied to that backend (e.g. + // "openai/gpt-4o"), fallback used to carry the stale model into the + // NearAI config. The runtime would then POST /v1/chat/completions + // with `"model": "openai/gpt-4o"` to NearAI, which 404s forever. + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::remove_var("LLM_API_KEY"); + std::env::remove_var("LLM_BASE_URL"); + std::env::remove_var("NEARAI_MODEL"); + } + + let settings = Settings { + llm_backend: Some("openai_compatible".to_string()), + // Model a user would pick for openai_compatible/openrouter, + // nonsensical to NearAI. + selected_model: Some("openai/gpt-4o".to_string()), + ..Default::default() + }; + + let cfg = LlmConfig::resolve_with_fallback(&settings) + .expect("resolve should succeed via fallback"); + assert_eq!(cfg.backend, "nearai"); + assert_ne!( + cfg.nearai.model, "openai/gpt-4o", + "fallback must not carry the pre-fallback selected_model into NearAI config — \ + got a stale model that would 404 on the first request" + ); + assert_eq!( + cfg.nearai.model, + crate::llm::DEFAULT_MODEL, + "NearAI fallback should use the built-in default model when the pre-fallback \ + selection is cleared" + ); + } + + #[test] + fn resolve_falls_back_when_openai_compatible_missing_base_url() { + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::remove_var("LLM_BASE_URL"); + std::env::remove_var("LLM_API_KEY"); + } + + let settings = Settings { + llm_backend: Some("openai_compatible".to_string()), + ..Default::default() + }; + + // openai_compatible has base_url_required=true and no default — this + // previously returned Err(MissingRequired), causing main.rs to bail + // and the container to crash-loop. resolve() now recovers. + let cfg = LlmConfig::resolve_with_fallback(&settings) + .expect("resolve should succeed via fallback"); + assert_eq!(cfg.backend, "nearai"); + } + + #[test] + fn resolve_does_not_fall_back_when_backend_is_properly_configured() { + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::set_var("GROQ_API_KEY", TEST_API_KEY); + } + + let settings = Settings { + llm_backend: Some("groq".to_string()), + ..Default::default() + }; + + let cfg = LlmConfig::resolve_with_fallback(&settings).expect("resolve should succeed"); + assert_eq!( + cfg.backend, "groq", + "a properly-configured backend must NOT trigger fallback" + ); + + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("GROQ_API_KEY"); + } + } + + #[test] + fn resolve_does_not_fall_back_for_anthropic_oauth_only() { + // Anthropic with only an OAuth token (no API key) must still resolve + // normally — the oauth_token counts as valid auth. + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::remove_var("ANTHROPIC_API_KEY"); + std::env::set_var("ANTHROPIC_OAUTH_TOKEN", TEST_ANTHROPIC_OAUTH_TOKEN); + } + + let settings = Settings { + llm_backend: Some("anthropic".to_string()), + ..Default::default() + }; + + let cfg = LlmConfig::resolve_with_fallback(&settings).expect("resolve should succeed"); + assert_eq!( + cfg.backend, "anthropic", + "anthropic with OAuth token must NOT fall back" + ); + + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("ANTHROPIC_OAUTH_TOKEN"); + } + } + + #[test] + fn resolve_falls_back_when_custom_provider_has_empty_base_url() { + // Custom providers have no hardcoded default base URL in the client + // layer, so an empty base_url means requests go to a bare path with + // no host. unusable_reason must catch this and fall back to NearAI. + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + } + + let settings = Settings { + llm_backend: Some("my-broken".to_string()), + llm_custom_providers: vec![crate::settings::CustomLlmProviderSettings { + id: "my-broken".to_string(), + name: "My Broken".to_string(), + adapter: "open_ai_completions".to_string(), + base_url: None, + default_model: Some("some-model".to_string()), + api_key: Some("sk-test".to_string()), + builtin: false, + }], + ..Default::default() + }; + + let cfg = LlmConfig::resolve_with_fallback(&settings) + .expect("resolve should succeed via fallback"); + assert_eq!( + cfg.backend, "nearai", + "custom provider with empty base_url must fall back to NearAI" + ); + } + + #[test] + fn resolve_does_not_fall_back_for_ollama_without_api_key() { + // Ollama runs locally and has no API key concept — missing api_key + // must NOT trigger fallback for built-in ollama. + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::remove_var("OLLAMA_BASE_URL"); + std::env::remove_var("OLLAMA_MODEL"); + } + + let settings = Settings { + llm_backend: Some("ollama".to_string()), + ..Default::default() + }; + + let cfg = LlmConfig::resolve_with_fallback(&settings).expect("resolve should succeed"); + assert_eq!( + cfg.backend, "ollama", + "ollama without api_key must NOT trigger fallback" + ); + } + + #[test] + fn resolve_does_not_fall_back_for_custom_ollama_with_base_url_no_key() { + // Custom ollama provider with a valid base_url and no api_key is + // fully usable — the !is_ollama guard in unusable_reason must skip + // the api_key check for any ollama-protocol provider. + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + } + + let settings = Settings { + llm_backend: Some("my-ollama".to_string()), + llm_custom_providers: vec![crate::settings::CustomLlmProviderSettings { + id: "my-ollama".to_string(), + name: "My Ollama".to_string(), + adapter: "ollama".to_string(), + base_url: Some("http://localhost:11434".to_string()), + default_model: Some("llama3".to_string()), + api_key: None, + builtin: false, + }], + ..Default::default() + }; + + let cfg = LlmConfig::resolve_with_fallback(&settings).expect("resolve should succeed"); + assert_eq!( + cfg.backend, "my-ollama", + "custom ollama provider without api_key must NOT fall back" + ); + } + + #[test] + fn resolve_pure_does_not_trigger_fallback_or_log() { + // Regression test for the spurious "Falling back to NearAI" log that + // fired at early-startup resolve (before secrets were hydrated) and + // then got overridden by the later re-resolve. The fix split the + // entry points: resolve() is pure, resolve_with_fallback() is the + // one that may swap in NearAI. build() calls resolve() so the log + // never fires spuriously. + let _guard = lock_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::remove_var("ANTHROPIC_API_KEY"); + std::env::remove_var("ANTHROPIC_OAUTH_TOKEN"); + } + + let settings = Settings { + llm_backend: Some("anthropic".to_string()), + ..Default::default() + }; + + // Pure resolve keeps the configured backend even when it would be + // considered unusable — the caller is responsible for hydrating + // secrets and calling resolve_with_fallback afterwards. + let cfg = LlmConfig::resolve(&settings).expect("resolve should succeed"); + assert_eq!( + cfg.backend, "anthropic", + "pure resolve must not auto-fall-back; that is resolve_with_fallback's job" + ); + } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 66187b09c5..080a45b0ac 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -477,13 +477,72 @@ impl Config { hydrate_llm_keys_from_secrets(&mut settings, secrets, user_id).await; } - LlmConfig::resolve(&settings) + // Startup path (non-strict): fall back to NearAI if the user-configured + // backend is unusable. This prevents the #2514 crash-loop and keeps the + // instance runnable while the user fixes their provider configuration. + // + // Hot-reload path (strict): use pure `resolve` so a bad save fails the + // whole call and lets the caller roll back the triggering settings + // write. Silently falling back here would be worse UX — the user + // saved "openrouter", runtime would switch to NearAI, the UI would + // show NearAI, and the user would wonder where their selection went. + if strict_db_reads { + return LlmConfig::resolve(&settings); + } + + let configured_backend = settings.llm_backend.clone(); + let cfg = LlmConfig::resolve_with_fallback(&settings)?; + + // If fallback demoted the backend, persist the effective backend to + // the DB so the UI, status endpoint, and any other consumers stay + // consistent with what is actually running. Without this, the user + // would see "Active: openrouter" in Settings while the runtime is + // quietly using NearAI. + if let Some(store) = store + && fallback_fired(configured_backend.as_deref(), &cfg.backend) + { + tracing::warn!( + configured = ?configured_backend, + active = %cfg.backend, + "Syncing llm_backend in DB to reflect post-fallback runtime state" + ); + if let Err(e) = store + .set_setting( + user_id, + "llm_backend", + &serde_json::Value::String(cfg.backend.clone()), + ) + .await + { + tracing::warn!( + error = %e, + "Failed to persist post-fallback llm_backend to DB — UI may \ + display the previously-selected backend until next save" + ); + } + // The previously-selected model is almost certainly wrong for + // the NearAI fallback (e.g. an OpenRouter model name). Clear + // it so resolve_model() picks NearAI's default on next load. + if settings.selected_model.is_some() + && let Err(e) = store.delete_setting(user_id, "selected_model").await + { + tracing::warn!( + error = %e, + "Failed to clear selected_model after fallback" + ); + } + } + + Ok(cfg) } /// Resolve only the LLM configuration from the current source stack. /// /// This is used by hot reload paths that need the exact owner/admin merge /// semantics from startup without rebuilding unrelated config sections. + /// Non-strict mode: applies `resolve_with_fallback`, so an unusable user + /// backend downgrades to NearAI at startup instead of crash-looping + /// (#2514). Use [`resolve_llm_with_secrets_strict`] for hot-reload paths. pub(crate) async fn resolve_llm_with_secrets( store: Option<&(dyn crate::db::SettingsStore + Sync)>, user_id: &str, @@ -497,6 +556,9 @@ impl Config { /// Resolve LLM configuration for hot reload paths that must fail closed on /// DB read errors so the caller can roll back the triggering settings write. + /// Strict mode also disables the NearAI fallback: a broken save produces + /// `Err` rather than a silent demotion, which is the signal the caller + /// needs to trigger rollback and preserve the user's explicit selection. pub(crate) async fn resolve_llm_with_secrets_strict( store: Option<&(dyn crate::db::SettingsStore + Sync)>, user_id: &str, @@ -553,6 +615,63 @@ impl Config { } } +/// Detect whether `resolve_with_fallback` demoted the user-configured backend +/// to NearAI. Returns true when the user explicitly asked for something +/// non-trivial (non-empty, not already nearai) and the resolver landed on a +/// different backend. Aliases like `open_ai` → `openai` are not counted as a +/// fallback — only cross-backend demotion is. +fn fallback_fired(configured: Option<&str>, active: &str) -> bool { + let configured = match configured.map(str::trim).filter(|s| !s.is_empty()) { + Some(c) => c, + None => return false, + }; + // Normalize both sides so alias-only drift (e.g. open_ai → openai, + // near / near_ai → nearai) doesn't spuriously look like a fallback. + normalize_backend(configured) != normalize_backend(active) +} + +/// Normalize a backend id to the canonical form that `LlmConfig::resolve` +/// lands on after alias resolution. Must produce the same canonical id the +/// resolver uses — otherwise `fallback_fired` will mis-fire on every restart +/// for any DB value that's a known alias (e.g. `claude` → `anthropic`, +/// `bigmodel` → `zai`, `github-copilot` → `github_copilot`) and trigger a +/// spurious DB rewrite. +/// +/// Two sources of aliases: +/// 1. Registry-defined aliases — delegated to `ProviderRegistry::find`, which +/// is the same lookup `resolve_registry_provider` uses. +/// 2. Hardcoded aliases for the four "virtual" backends that are not in the +/// registry (nearai / bedrock / gemini_oauth / openai_codex). These must +/// stay in sync with the matching branches in `LlmConfig::resolve`. +fn normalize_backend(raw: &str) -> String { + let lower = raw.to_lowercase(); + + // (1) Virtual backends (not in the registry) — hardcoded alias list + // mirroring LlmConfig::resolve. + match lower.as_str() { + "nearai" | "near" | "near_ai" => return "nearai".to_string(), + "bedrock" | "aws" | "aws_bedrock" => return "bedrock".to_string(), + "gemini_oauth" | "gemini-oauth" => return "gemini_oauth".to_string(), + "openai_codex" | "openai-codex" | "codex" => return "openai_codex".to_string(), + _ => {} + } + + // (2) Registry providers — any alias declared in `providers.json` is + // resolved by `ProviderRegistry::find` to its canonical `id`. This is the + // SAME canonicalization `LlmConfig::resolve_registry_provider` does, so + // DB values like `claude` / `bigmodel` / `github-copilot` / `open_ai` + // won't look like a fallback. + if let Some(def) = crate::llm::ProviderRegistry::load().find(&lower) { + return def.id.clone(); + } + + // Unknown backend — resolve() treats it as openai_compatible at runtime, + // but here we conservatively return the input as-is. A truly unknown id + // won't match the canonical `active` either way; the comparison in + // `fallback_fired` just has to be consistent between both sides. + lower +} + pub(crate) fn load_bootstrap_settings( toml_path: Option<&std::path::Path>, ) -> Result { @@ -1373,4 +1492,86 @@ mod tests { "existing key should not be overwritten" ); } + + // ── fallback_fired / normalize_backend tests ───────────────────────── + // + // These gate the post-fallback DB sync in re_resolve_llm_with_secrets, + // so wrong answers either (a) let stale user intent linger in the DB + // (UI shows openrouter, runtime uses NearAI) or (b) clobber the user's + // selection every startup even though nothing meaningfully changed. + + #[test] + fn fallback_fired_detects_cross_backend_demotion() { + // The #2514 scenario: user picked openrouter, config was unusable, + // resolver demoted to NearAI. DB must be synced. + assert!(fallback_fired(Some("openrouter"), "nearai")); + assert!(fallback_fired(Some("anthropic"), "nearai")); + assert!(fallback_fired(Some("openai_compatible"), "nearai")); + } + + #[test] + fn fallback_fired_ignores_alias_normalization() { + // `resolve` canonicalises backend aliases (near → nearai, open_ai → + // openai) but that is not a fallback and must not trigger a DB + // rewrite — doing so would churn the row on every startup. + // + // Virtual backends (not in the registry — alias set hardcoded in + // normalize_backend): + assert!(!fallback_fired(Some("near"), "nearai")); + assert!(!fallback_fired(Some("near_ai"), "nearai")); + assert!(!fallback_fired(Some("aws"), "bedrock")); + assert!(!fallback_fired(Some("aws_bedrock"), "bedrock")); + assert!(!fallback_fired(Some("codex"), "openai_codex")); + assert!(!fallback_fired(Some("openai-codex"), "openai_codex")); + assert!(!fallback_fired(Some("gemini-oauth"), "gemini_oauth")); + } + + #[test] + fn fallback_fired_ignores_registry_aliases() { + // Regression: `providers.json` declares aliases for many registry + // providers (e.g. `claude` → `anthropic`, `bigmodel` → `zai`, + // `github-copilot` → `github_copilot`, `open_ai` → `openai`). + // `resolve_registry_provider` canonicalises these to the registry's + // `id` field, so a DB value of `claude` produces `cfg.backend == + // "anthropic"`. normalize_backend must delegate to the registry so + // this is recognised as alias drift, not a fallback. Otherwise the + // DB gets rewritten on every startup for users who happen to have + // the alias form saved. + assert!(!fallback_fired(Some("claude"), "anthropic")); + assert!(!fallback_fired(Some("bigmodel"), "zai")); + assert!(!fallback_fired(Some("github-copilot"), "github_copilot")); + assert!(!fallback_fired(Some("githubcopilot"), "github_copilot")); + assert!(!fallback_fired(Some("open_ai"), "openai")); + assert!(!fallback_fired( + Some("openai-compatible"), + "openai_compatible" + )); + assert!(!fallback_fired(Some("compatible"), "openai_compatible")); + assert!(!fallback_fired(Some("open_router"), "openrouter")); + } + + #[test] + fn fallback_fired_ignores_empty_or_unset_configured() { + // When the DB never had llm_backend set, the default of "nearai" + // resolves naturally — there is nothing to sync back. + assert!(!fallback_fired(None, "nearai")); + assert!(!fallback_fired(Some(""), "nearai")); + assert!(!fallback_fired(Some(" "), "nearai")); + } + + #[test] + fn fallback_fired_treats_case_insensitively() { + // DB values can be lowercase or mixed-case; don't treat case-only + // drift as a meaningful change. + assert!(!fallback_fired(Some("NearAI"), "nearai")); + assert!(!fallback_fired(Some("OPENAI"), "openai")); + } + + #[test] + fn fallback_fired_same_backend_no_sync() { + // A properly-configured backend must not trigger a DB rewrite. + assert!(!fallback_fired(Some("nearai"), "nearai")); + assert!(!fallback_fired(Some("anthropic"), "anthropic")); + assert!(!fallback_fired(Some("openrouter"), "openrouter")); + } } diff --git a/src/settings.rs b/src/settings.rs index 40f7fe8714..791124fcca 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -15,7 +15,7 @@ use serde::{Deserialize, Serialize}; use crate::bootstrap::ironclaw_base_dir; /// A custom LLM provider defined by the user through the web UI. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct CustomLlmProviderSettings { /// Unique identifier (used as `llm_backend` value). pub id: String, @@ -37,6 +37,20 @@ pub struct CustomLlmProviderSettings { pub builtin: bool, } +impl std::fmt::Debug for CustomLlmProviderSettings { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CustomLlmProviderSettings") + .field("id", &self.id) + .field("name", &self.name) + .field("adapter", &self.adapter) + .field("base_url", &self.base_url) + .field("default_model", &self.default_model) + .field("api_key", &self.api_key.as_ref().map(|_| "[REDACTED]")) + .field("builtin", &self.builtin) + .finish() + } +} + /// Per-provider overrides for built-in LLM providers (API key and/or model). /// /// Stored as `llm_builtin_overrides` in the settings store, keyed by provider ID @@ -44,7 +58,7 @@ pub struct CustomLlmProviderSettings { /// /// Note: The global `selected_model` (if set) takes precedence over these /// per-provider overrides, which in turn take precedence over environment variables. -#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[derive(Clone, Default, Serialize, Deserialize)] pub struct LlmBuiltinOverride { /// API key override. Takes precedence over environment variables. #[serde(default, skip_serializing_if = "Option::is_none")] @@ -57,6 +71,16 @@ pub struct LlmBuiltinOverride { pub base_url: Option, } +impl std::fmt::Debug for LlmBuiltinOverride { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("LlmBuiltinOverride") + .field("api_key", &self.api_key.as_ref().map(|_| "[REDACTED]")) + .field("model", &self.model) + .field("base_url", &self.base_url) + .finish() + } +} + /// Canonical secret name for a built-in provider's API key. pub fn builtin_secret_name(provider_id: &str) -> String { format!("llm_builtin_{provider_id}_api_key") @@ -2809,4 +2833,44 @@ mod tests { "TOML selected_model should be preserved when DB has no value" ); } + + #[test] + fn test_custom_provider_debug_redacts_api_key() { + let provider = CustomLlmProviderSettings { + id: "test".to_string(), + name: "Test".to_string(), + adapter: "open_ai_completions".to_string(), + base_url: Some("https://api.example.com".to_string()), + default_model: Some("gpt-4".to_string()), + api_key: Some("sk-super-secret-key".to_string()), + builtin: false, + }; + let debug_output = format!("{:?}", provider); + assert!( + !debug_output.contains("sk-super-secret-key"), + "Debug output must not contain the real API key" + ); + assert!( + debug_output.contains("[REDACTED]"), + "Debug output must show [REDACTED] for api_key" + ); + } + + #[test] + fn test_builtin_override_debug_redacts_api_key() { + let override_val = LlmBuiltinOverride { + api_key: Some("sk-secret-123".to_string()), + model: Some("gpt-4".to_string()), + base_url: None, + }; + let debug_output = format!("{:?}", override_val); + assert!( + !debug_output.contains("sk-secret-123"), + "Debug output must not contain the real API key" + ); + assert!( + debug_output.contains("[REDACTED]"), + "Debug output must show [REDACTED] for api_key" + ); + } }