From 5aebb17fa229c9d1796386d1b68779da818a6217 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Jul 2026 07:22:47 -0400 Subject: [PATCH 1/3] Return bad request for unsupported reasoning effort --- .../src/server/handler/completions.rs | 22 +++++--- lib/apps/fabro-server/src/server/tests.rs | 50 +++++++++++++++++++ lib/components/fabro-llm/src/client.rs | 12 ++--- lib/components/fabro-llm/src/error.rs | 29 +++++++++++ lib/components/fabro-workflow/src/error.rs | 1 + 5 files changed, 100 insertions(+), 14 deletions(-) diff --git a/lib/apps/fabro-server/src/server/handler/completions.rs b/lib/apps/fabro-server/src/server/handler/completions.rs index b53e56d31..726847d5a 100644 --- a/lib/apps/fabro-server/src/server/handler/completions.rs +++ b/lib/apps/fabro-server/src/server/handler/completions.rs @@ -26,6 +26,17 @@ fn finish_reason_to_api_stop_reason(reason: &FinishReason) -> String { } } +fn llm_error_response(error: fabro_llm::Error) -> Response { + match error { + fabro_llm::Error::InvalidRequest { message } => { + ApiError::bad_request(message).into_response() + } + error => { + ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {error}")).into_response() + } + } +} + async fn create_completion( _auth: RequiredUser, State(state): State>, @@ -127,10 +138,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 llm_error_response(error), }; llm_sse::stream_response(stream_result, state.shutdown_token()) @@ -179,8 +187,7 @@ async fn create_completion( }) .into_response() } - Err(e) => ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {e}")) - .into_response(), + Err(error) => llm_error_response(error), } } else { match client.complete(&request).await { @@ -202,8 +209,7 @@ async fn create_completion( }) .into_response() } - Err(e) => ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {e}")) - .into_response(), + Err(error) => llm_error_response(error), } } } diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 454455be0..372cc1e6c 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -15272,6 +15272,56 @@ 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" + ) + ); + } + } + + completion.assert_calls(0); +} + #[tokio::test] async fn create_completion_default_model_uses_app_state_catalog() { let upstream = MockServer::start(); diff --git a/lib/components/fabro-llm/src/client.rs b/lib/components/fabro-llm/src/client.rs index 446b0b77a..8cfaadf1b 100644 --- a/lib/components/fabro-llm/src/client.rs +++ b/lib/components/fabro-llm/src/client.rs @@ -410,12 +410,11 @@ 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, }); } } @@ -439,7 +438,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 +475,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 +1482,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'") )); } 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..128134480 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, } From 59d5b317dcced86241f45c1871b9ea5b8c93c739 Mon Sep 17 00:00:00 2001 From: Release Repro Date: Fri, 24 Jul 2026 08:29:12 -0400 Subject: [PATCH 2/3] Simplify LLM error mapping and validate reasoning_effort parsing - Return InvalidRequest (400) for unsupported speed too, matching the reasoning_effort check and the complete()/stream() doc comments - Centralize fabro_llm::Error -> ApiError mapping in a From impl so the completions handler, playground handler, and Error::Llm arm agree on the InvalidRequest -> 400 / else -> 502 split - Reject unparseable reasoning_effort values with 400 instead of silently dropping them - Add classify_sdk_invalid_request test per fabro-workflow convention Co-Authored-By: Claude Fable 5 --- lib/apps/fabro-server/src/error.rs | 13 +++++- .../src/server/handler/completions.rs | 39 +++++++++------- .../src/server/handler/playground.rs | 3 +- lib/apps/fabro-server/src/server/tests.rs | 46 ++++++++++++++++++- lib/components/fabro-llm/src/client.rs | 9 ++-- lib/components/fabro-workflow/src/error.rs | 8 ++++ 6 files changed, 92 insertions(+), 26 deletions(-) diff --git a/lib/apps/fabro-server/src/error.rs b/lib/apps/fabro-server/src/error.rs index 356d68af1..70537f714 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,17 @@ impl From for ApiError { } } +/// LLM errors split at the HTTP boundary: request-validation failures are the +/// caller's fault (400); everything else is an upstream failure (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 726847d5a..e8f06a5d2 100644 --- a/lib/apps/fabro-server/src/server/handler/completions.rs +++ b/lib/apps/fabro-server/src/server/handler/completions.rs @@ -1,7 +1,7 @@ use std::collections::HashSet; use std::sync::Arc; -use fabro_model::{Catalog, ModelSelectionError}; +use fabro_model::{Catalog, ModelSelectionError, ReasoningEffort}; use super::super::{ ApiError, AppState, CompletionResponse, CompletionToolChoiceMode, CompletionUsage, @@ -26,17 +26,6 @@ fn finish_reason_to_api_stop_reason(reason: &FinishReason) -> String { } } -fn llm_error_response(error: fabro_llm::Error) -> Response { - match error { - fabro_llm::Error::InvalidRequest { message } => { - ApiError::bad_request(message).into_response() - } - error => { - ApiError::new(StatusCode::BAD_GATEWAY, format!("LLM error: {error}")).into_response() - } - } -} - async fn create_completion( _auth: RequiredUser, State(state): State>, @@ -104,6 +93,24 @@ async fn create_completion( CompletionToolChoiceMode::Named => ToolChoice::named(tc.tool_name.unwrap_or_default()), }); + let reasoning_effort = match req.reasoning_effort.as_deref() { + None => None, + Some(value) => match value.parse::() { + Ok(effort) => Some(effort), + Err(_) => { + return ApiError::bad_request(format!( + "invalid reasoning_effort '{value}'; allowed values: {}", + ReasoningEffort::variants() + .iter() + .map(|v| <&'static str>::from(*v)) + .collect::>() + .join(", ") + )) + .into_response(); + } + }, + }; + // Build the LLM request let request = LlmRequest { model: model_id.clone(), @@ -120,7 +127,7 @@ async fn create_completion( } else { Some(req.stop_sequences) }, - reasoning_effort: req.reasoning_effort.as_deref().and_then(|s| s.parse().ok()), + reasoning_effort, speed: None, metadata: None, provider_options: req.provider_options, @@ -138,7 +145,7 @@ async fn create_completion( // Streaming path: forward all StreamEvents as SSE let stream_result = match client.stream(&request).await { Ok(s) => s, - Err(error) => return llm_error_response(error), + Err(error) => return ApiError::from(error).into_response(), }; llm_sse::stream_response(stream_result, state.shutdown_token()) @@ -187,7 +194,7 @@ async fn create_completion( }) .into_response() } - Err(error) => llm_error_response(error), + Err(error) => ApiError::from(error).into_response(), } } else { match client.complete(&request).await { @@ -209,7 +216,7 @@ async fn create_completion( }) .into_response() } - Err(error) => llm_error_response(error), + 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 372cc1e6c..13d276ca8 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -15314,7 +15314,8 @@ async fn create_completion_unsupported_reasoning_efforts_return_bad_request() { body["errors"][0]["detail"], format!( "model 'kimi-k3' does not support reasoning_effort '{effort}'; allowed values: low, high, max" - ) + ), + "stream={stream}" ); } } @@ -15322,6 +15323,49 @@ async fn create_completion_unsupported_reasoning_efforts_return_bad_request() { completion.assert_calls(0); } +#[tokio::test] +async fn create_completion_unparseable_reasoning_effort_returns_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); + + 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": "bananas", + "messages": [ + { + "role": "user", + "content": [{"kind": "text", "data": "hi"}] + } + ] + }) + .to_string(), + )) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + let body = response_json!(response, StatusCode::BAD_REQUEST).await; + assert_eq!( + body["errors"][0]["detail"], + "invalid reasoning_effort 'bananas'; allowed values: low, medium, high, xhigh, max" + ); + + completion.assert_calls(0); +} + #[tokio::test] async fn create_completion_default_model_uses_app_state_catalog() { let upstream = MockServer::start(); diff --git a/lib/components/fabro-llm/src/client.rs b/lib/components/fabro-llm/src/client.rs index 8cfaadf1b..30d5c05a0 100644 --- a/lib/components/fabro-llm/src/client.rs +++ b/lib/components/fabro-llm/src/client.rs @@ -421,12 +421,11 @@ impl Client { 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, }); } } @@ -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-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index 128134480..f52079a29 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -1223,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] From d611da345b2d2294022a5c75b69c336d03e1e818 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Jul 2026 09:13:26 -0400 Subject: [PATCH 3/3] Clarify non-validation LLM error mapping --- lib/apps/fabro-server/src/error.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/apps/fabro-server/src/error.rs b/lib/apps/fabro-server/src/error.rs index 70537f714..62831c983 100644 --- a/lib/apps/fabro-server/src/error.rs +++ b/lib/apps/fabro-server/src/error.rs @@ -171,7 +171,8 @@ impl From for ApiError { } /// LLM errors split at the HTTP boundary: request-validation failures are the -/// caller's fault (400); everything else is an upstream failure (502). +/// 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 {