Merge pull request #611 from fabro-sh/fix/validate-model-reasoning-effort

Return 400 for unsupported reasoning effort

lib/apps/fabro-server/src/error.rs

@@ -161,7 +161,7 @@ impl From<Error> 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<Error> 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<fabro_llm::Error> 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

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(),
             }
         }
     }

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();
         }
     };
 

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();

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<Response, Error> {
@@ -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<StreamEventStream, Error> {
@@ -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'")
         ));
     }

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(),

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]