From 5606940aafe6defa7150201af34c69d8e5315aad Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 24 Aug 2026 13:02:14 -0400 Subject: [PATCH] Parse create-run bodies strictly per admission lane Both lanes now deserialize the raw request bytes directly instead of round-tripping through a serde_json::Value, which silently collapsed duplicate JSON keys to last-key-wins on the legacy manifest lane and stripped line/column locations from manifest parse errors. When neither lane accepts the body, attribution now recognizes a defective manifest by its required keys, so a legacy manifest carrying a stray workflow_version_id keeps its 400 manifest error instead of being misrouted to a 422 run_intent_invalid describing a schema the caller never used. Co-Authored-By: Claude Fable 5 --- .../fabro-server/src/server/handler/runs.rs | 64 ++++++++++++------- lib/apps/fabro-server/src/server/tests.rs | 39 +++++++++++ 2 files changed, 81 insertions(+), 22 deletions(-) diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 9b3bbd19e..7e8ae521a 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -38,7 +38,6 @@ use fabro_workflow::command_log::{command_log_path, read_json_string_blob, read_ use fabro_workflow::run_status::RunStatus; use fabro_workflow::workflow_bundle::WorkflowBundle; use fabro_workflow::{Error as WorkflowError, operations}; -use serde::Deserialize as _; use strum::VariantArray as _; use tokio::fs; use tracing::info; @@ -536,34 +535,19 @@ async fn create_run( headers: HeaderMap, body: Bytes, ) -> Response { - let value = match serde_json::from_slice::(&body) { - Ok(value) => value, - Err(err) => { - return ApiError::with_code(StatusCode::BAD_REQUEST, err.to_string(), "invalid_json") - .into_response(); - } - }; - let intent_error = match RunIntent::deserialize(&value) { + // Both lanes parse the raw bytes directly so serde_json keeps its + // duplicate-key rejection and line/column error locations; a JSON `Value` + // round-trip would silently collapse duplicate keys to last-key-wins. + let intent_error = match serde_json::from_slice::(&body) { Ok(intent) => { return Box::pin(create_run_from_intent(state, intent, actor, headers)).await; } Err(err) => err, }; - let req = match RunManifest::deserialize(&value) { + let req = match serde_json::from_slice::(&body) { Ok(req) => req, Err(manifest_error) => { - if value - .as_object() - .is_some_and(|object| object.contains_key("workflow_version_id")) - { - return ApiError::with_code( - StatusCode::UNPROCESSABLE_ENTITY, - intent_error.to_string(), - "run_intent_invalid", - ) - .into_response(); - } - return ApiError::bad_request(manifest_error.to_string()).into_response(); + return create_run_parse_error(&body, &intent_error, &manifest_error); } }; let explicit_title_supplied = req.title.is_some(); @@ -582,6 +566,42 @@ async fn create_run( .await } +/// Attribute a create-run body that neither lane accepted. A body carrying +/// any of the legacy manifest's required keys is a defective manifest even +/// when a stray `workflow_version_id` rides along, and keeps the manifest +/// lane's `400` contract; only an intent-shaped body gets the intent `422`. +fn create_run_parse_error( + body: &[u8], + intent_error: &serde_json::Error, + manifest_error: &serde_json::Error, +) -> Response { + let value = match serde_json::from_slice::(body) { + Ok(value) => value, + Err(err) => { + return ApiError::with_code(StatusCode::BAD_REQUEST, err.to_string(), "invalid_json") + .into_response(); + } + }; + let has_key = |key: &str| { + value + .as_object() + .is_some_and(|object| object.contains_key(key)) + }; + if has_key("workflow_version_id") + && !has_key("version") + && !has_key("cwd") + && !has_key("workflows") + { + return ApiError::with_code( + StatusCode::UNPROCESSABLE_ENTITY, + intent_error.to_string(), + "run_intent_invalid", + ) + .into_response(); + } + ApiError::bad_request(manifest_error.to_string()).into_response() +} + async fn create_run_from_intent( state: Arc, intent: RunIntent, diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 70ec20465..a704795ef 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -3757,6 +3757,45 @@ async fn post_runs_run_intent_dispatches_errors_without_changing_legacy_lane() { assert_eq!(legacy["lifecycle"]["status"]["kind"], "submitted"); } +#[tokio::test] +async fn post_runs_attributes_parse_failures_and_rejects_duplicate_keys() { + let state = test_app_state(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let post = |body: String| { + Request::builder() + .method("POST") + .uri(api("/runs")) + .header("content-type", "application/json") + .body(Body::from(body)) + .unwrap() + }; + + // A defective manifest keeps the manifest lane's 400 contract even when + // a stray workflow_version_id rides along. + let mut manifest = minimal_manifest_json(MINIMAL_DOT); + manifest["workflow_version_id"] = json!(fabro_types::test_support::test_workflow_version_id()); + manifest["cwd"] = json!(42); + let response = app + .clone() + .oneshot(post(manifest.to_string())) + .await + .unwrap(); + let body = response_json!(response, StatusCode::BAD_REQUEST).await; + let detail = body["errors"][0]["detail"].as_str().unwrap(); + assert!(detail.contains("invalid type: integer `42`"), "{detail}"); + + // Duplicate JSON keys are ambiguous: they must be rejected, not + // collapsed to last-key-wins by a Value round-trip. + let duplicated = format!( + r#"{{"version":1,"cwd":"/tmp","cwd":"/other","target":{{"path":"workflow.fabro"}},"workflows":{{"workflow.fabro":{{"source":{source},"files":{{}}}}}}}}"#, + source = serde_json::to_string(MINIMAL_DOT).unwrap() + ); + let response = app.clone().oneshot(post(duplicated)).await.unwrap(); + let body = response_json!(response, StatusCode::BAD_REQUEST).await; + let detail = body["errors"][0]["detail"].as_str().unwrap(); + assert!(detail.contains("duplicate field"), "{detail}"); +} + #[tokio::test] async fn post_runs_run_intent_maps_missing_version_environment_and_target_errors() { let state = test_app_state();