diff --git a/lib/apps/fabro-server/src/error.rs b/lib/apps/fabro-server/src/error.rs index 356d68af1..62831c983 100644 --- a/lib/apps/fabro-server/src/error.rs +++ b/lib/apps/fabro-server/src/error.rs @@ -161,7 +161,7 @@ impl From for ApiError { Error::BadGateway(msg) => Self::new(StatusCode::BAD_GATEWAY, msg), Error::Workflow(err) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()), Error::Agent(err) => Self::new(StatusCode::BAD_GATEWAY, err.to_string()), - Error::Llm(err) => Self::new(StatusCode::BAD_GATEWAY, err.to_string()), + Error::Llm(err) => Self::from(err), Error::Store(err) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()), Error::Config(err) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()), Error::Vault(err) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()), @@ -170,6 +170,18 @@ impl From for ApiError { } } +/// LLM errors split at the HTTP boundary: request-validation failures are the +/// caller's fault (400); non-validation LLM failures, including provider, +/// middleware, and local configuration failures, return 502. +impl From for ApiError { + fn from(err: fabro_llm::Error) -> Self { + match err { + fabro_llm::Error::InvalidRequest { message } => Self::bad_request(message), + err => Self::new(StatusCode::BAD_GATEWAY, format!("LLM error: {err}")), + } + } +} + impl IntoResponse for ApiError { fn into_response(self) -> Response { let title = self diff --git a/lib/apps/fabro-server/src/server/handler/completions.rs b/lib/apps/fabro-server/src/server/handler/completions.rs index b3682393c..b6d94d864 100644 --- a/lib/apps/fabro-server/src/server/handler/completions.rs +++ b/lib/apps/fabro-server/src/server/handler/completions.rs @@ -127,10 +127,7 @@ async fn create_completion( // Streaming path: forward all StreamEvents as SSE let stream_result = match client.stream(&request).await { Ok(s) => s, - Err(e) => { - return ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {e}")) - .into_response(); - } + Err(error) => return ApiError::from(error).into_response(), }; llm_sse::stream_response(stream_result, state.shutdown_token()) @@ -177,8 +174,7 @@ async fn create_completion( }) .into_response() } - Err(e) => ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {e}")) - .into_response(), + Err(error) => ApiError::from(error).into_response(), } } else { match client.complete(&request).await { @@ -197,8 +193,7 @@ async fn create_completion( }) .into_response() } - Err(e) => ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {e}")) - .into_response(), + Err(error) => ApiError::from(error).into_response(), } } } diff --git a/lib/apps/fabro-server/src/server/handler/playground.rs b/lib/apps/fabro-server/src/server/handler/playground.rs index d404c2b58..ee52c56e0 100644 --- a/lib/apps/fabro-server/src/server/handler/playground.rs +++ b/lib/apps/fabro-server/src/server/handler/playground.rs @@ -169,8 +169,7 @@ async fn create_playground_chat( Ok(s) => s, Err(e) => { error!(error = ?e, "playground: LLM stream call failed"); - return ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {e}")) - .into_response(); + return ApiError::from(e).into_response(); } }; diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index e7ad29616..2375dd54e 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -15293,6 +15293,57 @@ async fn create_completion_unknown_provider_returns_clear_error() { ); } +#[tokio::test] +async fn create_completion_unsupported_reasoning_efforts_return_bad_request() { + let upstream = MockServer::start(); + let completion = upstream.mock(|when, then| { + when.method(POST); + then.status(500); + }); + let state = TestAppStateBuilder::new() + .provider_base_url("kimi", upstream.url("/v1")) + .vault_entries([(EnvVars::KIMI_API_KEY, "test-kimi-api-key")]) + .build(); + let app = crate::test_support::build_test_router(state); + + for stream in [false, true] { + for effort in ["medium", "xhigh"] { + let req = Request::builder() + .method("POST") + .uri(api("/completions")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({ + "provider": "kimi", + "model": "kimi-k3", + "reasoning_effort": effort, + "stream": stream, + "messages": [ + { + "role": "user", + "content": [{"kind": "text", "data": "hi"}] + } + ] + }) + .to_string(), + )) + .unwrap(); + + let response = app.clone().oneshot(req).await.unwrap(); + let body = response_json!(response, StatusCode::BAD_REQUEST).await; + assert_eq!( + body["errors"][0]["detail"], + format!( + "model 'kimi-k3' does not support reasoning_effort '{effort}'; allowed values: low, high, max" + ), + "stream={stream}" + ); + } + } + + completion.assert_calls(0); +} + #[tokio::test] async fn create_completion_returns_disjoint_usage_buckets() { let upstream = MockServer::start(); diff --git a/lib/components/fabro-llm/src/client.rs b/lib/components/fabro-llm/src/client.rs index 446b0b77a..30d5c05a0 100644 --- a/lib/components/fabro-llm/src/client.rs +++ b/lib/components/fabro-llm/src/client.rs @@ -410,24 +410,22 @@ impl Client { if let Some(effort) = request.reasoning_effort { if !settings.controls.reasoning_effort.contains(&effort) { - return Err(Error::Configuration { + return Err(Error::InvalidRequest { message: format!( "model '{model_id}' does not support reasoning_effort '{effort}'; allowed values: {}", format_control_values(&settings.controls.reasoning_effort), ), - source: None, }); } } if let Some(speed) = request.speed { if speed != Speed::Standard && !settings.controls.speed.contains(&speed) { - return Err(Error::Configuration { + return Err(Error::InvalidRequest { message: format!( "model '{model_id}' does not support speed '{speed}'; allowed values: standard{}", format_additional_speeds(&settings.controls.speed), ), - source: None, }); } } @@ -439,7 +437,8 @@ impl Client { /// /// # Errors /// - /// Returns `Error::Configuration` if no provider is specified or + /// Returns `Error::InvalidRequest` when a catalog-declared request control + /// is unsupported, `Error::Configuration` if no provider is specified or /// registered, or any provider/middleware error encountered during the /// request. pub async fn complete(&self, request: &Request) -> Result { @@ -475,7 +474,8 @@ impl Client { /// /// # Errors /// - /// Returns `Error::Configuration` if no provider is specified or + /// Returns `Error::InvalidRequest` when a catalog-declared request control + /// is unsupported, `Error::Configuration` if no provider is specified or /// registered, or any provider/middleware error encountered during the /// request. pub async fn stream(&self, request: &Request) -> Result { @@ -1481,9 +1481,8 @@ output_cost_per_mtok = 20.0 assert!(matches!( err, - Error::Configuration { + Error::InvalidRequest { ref message, - .. } if message.contains("model 'kimi-k2.5' does not support reasoning_effort 'high'") )); } @@ -1527,9 +1526,8 @@ output_cost_per_mtok = 20.0 assert!(matches!( err, - Error::Configuration { + Error::InvalidRequest { ref message, - .. } if message.contains("model 'gpt-5.4' does not support speed 'fast'") )); } @@ -1616,9 +1614,8 @@ output_cost_per_mtok = 20.0 assert!(matches!( err, - Error::Configuration { + Error::InvalidRequest { ref message, - .. } if message.contains("model 'gpt-5.4' does not support speed 'fast'") )); } diff --git a/lib/components/fabro-llm/src/error.rs b/lib/components/fabro-llm/src/error.rs index fd2987047..000f36943 100644 --- a/lib/components/fabro-llm/src/error.rs +++ b/lib/components/fabro-llm/src/error.rs @@ -95,6 +95,9 @@ pub enum Error { #[error("No object generated: {message}")] NoObjectGenerated { message: String }, + #[error("Invalid request: {message}")] + InvalidRequest { message: String }, + #[error("Configuration error: {message}")] Configuration { message: String, @@ -164,6 +167,7 @@ impl Error { Self::InvalidToolCall { .. } | Self::NoObjectGenerated { .. } | Self::Interrupt { .. } + | Self::InvalidRequest { .. } | Self::Configuration { .. } | Self::UnsupportedToolChoice { .. } | Self::RequestTimeout { .. } => false, @@ -266,6 +270,9 @@ impl Error { Self::NoObjectGenerated { .. } => { format!("api_deterministic|{provider}|no_object") } + Self::InvalidRequest { .. } => { + format!("api_deterministic|{provider}|invalid_request") + } Self::UnsupportedToolChoice { .. } => { format!("api_deterministic|{provider}|unsupported_tool_choice") } @@ -805,6 +812,14 @@ mod tests { source: None, }; assert_eq!(err.to_string(), "Configuration error: no provider"); + + let err = Error::InvalidRequest { + message: "unsupported reasoning effort".into(), + }; + assert_eq!( + err.to_string(), + "Invalid request: unsupported reasoning effort" + ); } #[test] @@ -996,6 +1011,13 @@ mod tests { .failover_eligible() ); + assert!( + !Error::InvalidRequest { + message: "bad".into(), + } + .failover_eligible() + ); + assert!( !Error::UnsupportedToolChoice { message: "nope".into(), @@ -1146,6 +1168,13 @@ mod tests { .failure_signature_hint(), "api_deterministic|unknown|no_object" ); + assert_eq!( + Error::InvalidRequest { + message: "bad".into(), + } + .failure_signature_hint(), + "api_deterministic|unknown|invalid_request" + ); assert_eq!( Error::UnsupportedToolChoice { message: "nope".into(), diff --git a/lib/components/fabro-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index 75b6d317f..f52079a29 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -38,6 +38,7 @@ pub fn classify_sdk_error(err: &LlmError) -> FailureCategory { LlmError::Interrupt { .. } => FailureCategory::Canceled, LlmError::InvalidToolCall { .. } | LlmError::NoObjectGenerated { .. } + | LlmError::InvalidRequest { .. } | LlmError::Configuration { .. } | LlmError::UnsupportedToolChoice { .. } => FailureCategory::Deterministic, } @@ -1222,6 +1223,14 @@ mod tests { assert_eq!(classify_sdk_error(&err), FailureCategory::Deterministic); } + #[test] + fn classify_sdk_invalid_request() { + let err = SdkError::InvalidRequest { + message: "unsupported reasoning effort".into(), + }; + assert_eq!(classify_sdk_error(&err), FailureCategory::Deterministic); + } + // --- hints count guards --- #[test]