From 88775bc3a17cce655baceec56961eedbba59cf55 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 17:09:42 -0700 Subject: [PATCH 01/11] fix(sales): reject fractional order-line quantities instead of leaking a JSON-binding error (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a sales order line with a fractional quantity (e.g. 2.5) failed during minimal-API JSON binding (AddOrderItemRequest.Quantity / UpdateOrderItemRequest.Quantity are int) and the SPA rendered the raw internal message: `Failed to read parameter "AddOrderItemRequest request" from the request body as JSON.` Two parts, matching the two ways this could reach a user: 1. Global (every typed-body endpoint, not just Sales): outside Development, ASP.NET Core's minimal-API binding defaults RouteHandlerOptions.ThrowOnBadRequest to false, so a binding failure sets Response.StatusCode directly and returns WITHOUT throwing — UseExceptionHandler's /error mapping never even ran, and the client got a bare 400 with a completely empty body (worse than the leak: not even a generic ProblemDetails). Program.cs now forces ThrowOnBadRequest=true in every environment, and /error maps a 400-status BadHttpRequestException through the same ValidationResponse.Problem shape every other 400 in this app uses, instead of echoing bad.Message. A non-400 BadHttpRequestException (the #309 413 body-cap, a 415 content-type mismatch) is untouched — RequestBodyLimit.cs's WriteBodyTooLargeAsync depends on that shape staying as-is. 2. Sales-specific SPA: the add-line and edit-line quantity controls (SalesPage.tsx) now reject a non-integer quantity before sending, with a localized "Quantity must be a whole number." message (en/es/tl, es/tl machine-drafted pending native review). Decimal unit prices are untouched. Tests: two API integration tests pin the safe ValidationProblem shape for AddOrderItem and UpdateOrderItem (status, content-type, JSON shape, and that the old leaked text is absent), one control test pins that an ordinary integer-quantity/decimal-priced add still works end to end, and two SPA Vitest tests pin that the add-line/edit-line controls refuse to call the API at all on a fractional quantity. Every new test was mutation-checked (fix reverted -> red, restored -> green); the UpdateOrderItem test's first draft was too weak to catch the missing ThrowOnBadRequest registration (an empty body trivially satisfies "does not contain the leaked text") and was strengthened to also check content-type and the parsed ValidationProblem shape. Updates specs/product/GLOSSARY.md and the SPA Help page (en/es/tl) to note quantity is always a whole number and price stays decimal-capable. --- specs/product/GLOSSARY.md | 13 ++- src/Cluckwork.Api/Program.cs | 49 ++++++++- .../CustomerAndOrderTests.cs | 101 ++++++++++++++++++ .../RequestLoggingTests.cs | 8 +- web/src/i18n/en.ts | 15 ++- web/src/i18n/es.ts | 17 +-- web/src/i18n/tl.ts | 17 +-- web/src/routes/SalesPage.test.tsx | 43 ++++++++ web/src/routes/SalesPage.tsx | 8 ++ 9 files changed, 240 insertions(+), 31 deletions(-) diff --git a/specs/product/GLOSSARY.md b/specs/product/GLOSSARY.md index 59d452e5f..196098dda 100644 --- a/specs/product/GLOSSARY.md +++ b/specs/product/GLOSSARY.md @@ -261,11 +261,14 @@ exist in the status enum for later phases; nothing sets them yet, and only **Sales line (#99)** — sells a **product** in a packed unit. At line creation the line snapshots the product type, the grade the product mapped to, and the unit's eggs-per-unit factor (`base_unit_factor`, spec §10.5/§9.7); `quantity` -is selling units, `quantity_base = quantity × factor` is individual eggs — -allocation and the stock guard run on `quantity_base`. Re-pointing a -product's grade or redefining a packed unit only affects future lines, never -recorded ones. Price is per selling unit, prefilled from the product's -default and editable per line. +is selling units and always a whole number — the add/edit-line controls +reject a fractional value before sending, and a direct API request gets a +stable validation response rather than a raw JSON-binding error (#398) — +while `quantity_base = quantity × factor` is individual eggs; allocation and +the stock guard run on `quantity_base`. Re-pointing a product's grade or +redefining a packed unit only affects future lines, never recorded ones. +Price is per selling unit (decimal money, stored as integer minor units), +prefilled from the product's default and editable per line. **Void** — undo of a mistaken confirm (requires a reason): the allocated quantities return to the *exact* egg lots they were drawn from (recorded at diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index b655e3da1..b5a2ad6be 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -21,6 +21,7 @@ using Cluckwork.Api.Hosting; using Cluckwork.Api.Middleware; using Cluckwork.Api.Security; +using Cluckwork.Api.Validation; using Cluckwork.Infrastructure.Persistence; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Diagnostics; @@ -59,6 +60,23 @@ builder.Services.Configure( builder.Configuration.GetSection(IdempotencyOptions.SectionName)); +// #398 — ASP.NET Core's minimal-API parameter binding defaults +// RouteHandlerOptions.ThrowOnBadRequest to true only in Development; outside +// it (Testing, Production — everywhere this app actually runs client +// traffic) a binding failure (malformed JSON, a fractional quantity into an +// `int`, an unparseable date/guid, …) is swallowed INSIDE the framework's +// generated code: it sets Response.StatusCode itself and returns WITHOUT +// throwing, so UseExceptionHandler's `/error` mapping below never runs at +// all — the client gets a bare 400 with an EMPTY body, not even a generic +// ProblemDetails. Force the throw in every environment so this app's own +// `/error` handler is always the thing shaping the response, instead of that +// environment-conditional framework default. (Only ONE test factory — +// RequestLoggingFactory — used to carry its own copy of this override, added +// because ITS test specifically needed the real throw; this registration +// supersedes it globally, so that copy was removed.) +builder.Services.Configure( + o => o.ThrowOnBadRequest = true); + // --- OpenAPI --- builder.Services.AddOpenApi(); @@ -396,9 +414,34 @@ var exception = context.Features.Get()?.Error; return exception switch { - // Minimal-API body binding failures (malformed JSON, unparseable - // dates/guids) throw this with a 400 — without the mapping the - // exception handler swallowed it into a 500. + // #398 — a JSON-binding failure (a fractional quantity into an `int` + // field, an unparseable date/guid, malformed JSON syntax, …) throws + // this deep inside minimal-API's generated body-reader, BEFORE any + // FluentValidation validator or handler runs. bad.Message names the + // exact parameter/type it failed to bind (e.g. "Failed to read + // parameter \"AddOrderItemRequest request\" from the request body as + // JSON.") and its InnerException carries the raw System.Text.Json / + // FormatException detail — both are framework internals, never meant + // for an end user, and echoing them (as this branch used to) leaked + // parameter/type names straight into the SPA's error banner. Route + // THIS specific case — always StatusCode 400 by construction, the + // same default every JSON-binding BadHttpRequestException carries — + // through the SAME ValidationProblem shape every other 400 in this + // app uses (ValidationResponse.Problem), instead of echoing the + // exception text. + // + // A NON-400 BadHttpRequestException (413 from the #309 request-body + // cap, 415 from an unrecognised Content-Type, …) falls through to the + // unchanged branch below: RequestBodyLimit.cs's WriteBodyTooLargeAsync + // depends on THIS handler staying byte-identical to its own 413 shape + // for the (rare, non-JSON-bound-endpoint) case that reaches /error + // instead of being short-circuited there — don't fold that case into + // the 400-only ValidationProblem mapping above. + BadHttpRequestException { StatusCode: StatusCodes.Status400BadRequest } => + ValidationResponse.Problem(new Dictionary + { + ["body"] = ["The request body has an invalid or incorrectly formatted value."], + }), BadHttpRequestException bad => Results.Problem( detail: bad.Message, statusCode: bad.StatusCode, diff --git a/tests/Cluckwork.Api.IntegrationTests/CustomerAndOrderTests.cs b/tests/Cluckwork.Api.IntegrationTests/CustomerAndOrderTests.cs index 2fb958ed6..be1022210 100644 --- a/tests/Cluckwork.Api.IntegrationTests/CustomerAndOrderTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/CustomerAndOrderTests.cs @@ -185,6 +185,107 @@ public async Task AddItem_OverflowingLineTotal_Rejected() Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); } + // #398 — a fractional quantity (e.g. 2.5) used to fail deep inside + // minimal-API JSON binding, before AddOrderItemValidator or the handler + // ever ran, and Program.cs's /error handler echoed the raw + // BadHttpRequestException.Message straight back: `Failed to read + // parameter "AddOrderItemRequest request" from the request body as + // JSON.` This pins the safe replacement — the same ValidationProblem + // `errors` shape every other 400 in this app uses, with none of the + // parameter/type internals in the body. + [Fact] + public async Task AddItem_FractionalQuantity_StableProblemDetails_NotBindingExceptionText() + { + var (client, _, _, _, products) = await SetupAsync("Large"); + var customerId = await CreatedId(await client.PostWithKeyAsync( + "/api/v1/customers", Guid.NewGuid().ToString(), new { name = "C", phone = "1" })); + var orderId = await CreatedId(await client.PostWithKeyAsync( + "/api/v1/sales", Guid.NewGuid().ToString(), + new { customerId, orderDate = DateOnly.FromDateTime(DateTime.UtcNow.Date) })); + + var response = await client.PostWithKeyAsync( + $"/api/v1/sales/{orderId}/items", Guid.NewGuid().ToString(), + new { productId = products["Large"], quantity = 2.5m, unitPriceMinorUnits = 100 }); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Equal("application/problem+json", response.Content.Headers.ContentType?.MediaType); + var body = await response.Content.ReadAsStringAsync(); + // The old leak: the raw parameter-binding exception text must be gone. + Assert.DoesNotContain("Failed to read parameter", body, StringComparison.Ordinal); + Assert.DoesNotContain("AddOrderItemRequest", body, StringComparison.Ordinal); + Assert.DoesNotContain("FormatException", body, StringComparison.Ordinal); + Assert.DoesNotContain("Utf8JsonReader", body, StringComparison.Ordinal); + // Same shape as every other 400 in this app: a ValidationProblem errors dict. + using var doc = System.Text.Json.JsonDocument.Parse(body); + Assert.Equal(400, doc.RootElement.GetProperty("status").GetInt32()); + Assert.Equal(System.Text.Json.JsonValueKind.Object, doc.RootElement.GetProperty("errors").ValueKind); + } + + // The control: without this, the fractional-quantity test above proves + // nothing — a change that broke ordinary integer-quantity/decimal-priced + // adds entirely would look identical from that test's point of view alone. + [Fact] + public async Task AddItem_IntegerQuantityAndDecimalUnitPrice_StillWorks() + { + var (client, _, _, _, products) = await SetupAsync("Large"); + var customerId = await CreatedId(await client.PostWithKeyAsync( + "/api/v1/customers", Guid.NewGuid().ToString(), new { name = "C", phone = "1" })); + var orderId = await CreatedId(await client.PostWithKeyAsync( + "/api/v1/sales", Guid.NewGuid().ToString(), + new { customerId, orderDate = DateOnly.FromDateTime(DateTime.UtcNow.Date) })); + + // $2.50/unit (decimal money, carried on the wire as minor units per + // the app's existing decimal-to-minor-units convention) × 7 whole units. + var response = await client.PostWithKeyAsync( + $"/api/v1/sales/{orderId}/items", Guid.NewGuid().ToString(), + new { productId = products["Large"], quantity = 7, unitPriceMinorUnits = 250 }); + Assert.Equal(HttpStatusCode.Created, response.StatusCode); + + var order = await client.GetFromJsonAsync($"/api/v1/sales/{orderId}"); + var item = Assert.Single(order!.Items); + Assert.Equal(7, item.Quantity); + Assert.Equal(250, item.UnitPriceMinorUnits); + Assert.Equal(1750, order.TotalMinorUnits); + } + + // Same JSON-binding defect class, the OTHER request DTO + // (UpdateOrderItemRequest — the edit-line endpoint): proves the /error + // fix isn't accidentally tied to one specific parameter type name. + [Fact] + public async Task UpdateItem_FractionalQuantity_StableProblemDetails_NotBindingExceptionText() + { + var (client, _, _, _, products) = await SetupAsync("Large"); + var customerId = await CreatedId(await client.PostWithKeyAsync( + "/api/v1/customers", Guid.NewGuid().ToString(), new { name = "C", phone = "1" })); + var orderId = await CreatedId(await client.PostWithKeyAsync( + "/api/v1/sales", Guid.NewGuid().ToString(), + new { customerId, orderDate = DateOnly.FromDateTime(DateTime.UtcNow.Date) })); + var added = await client.PostWithKeyAsync( + $"/api/v1/sales/{orderId}/items", Guid.NewGuid().ToString(), + new { productId = products["Large"], quantity = 3, unitPriceMinorUnits = 100 }); + var itemId = (await added.Content.ReadFromJsonAsync())!.ItemId; + + var response = await client.PutWithKeyAsync( + $"/api/v1/sales/{orderId}/items/{itemId}", Guid.NewGuid().ToString(), + new { quantity = 1.5m, unitPriceMinorUnits = 100 }); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + // Content-Type + a parseable ValidationProblem body — not just "no + // leak", which an EMPTY body would also trivially satisfy. Outside + // Development, ASP.NET Core's own binding failure sets StatusCode + // directly with NO body at all unless RouteHandlerOptions. + // ThrowOnBadRequest is forced true (Program.cs, #398) so this + // reaches /error in the first place — a weaker check here would stay + // green even with that registration missing. + Assert.Equal("application/problem+json", response.Content.Headers.ContentType?.MediaType); + var body = await response.Content.ReadAsStringAsync(); + Assert.DoesNotContain("Failed to read parameter", body, StringComparison.Ordinal); + Assert.DoesNotContain("UpdateOrderItemRequest", body, StringComparison.Ordinal); + using var doc = System.Text.Json.JsonDocument.Parse(body); + Assert.Equal(400, doc.RootElement.GetProperty("status").GetInt32()); + Assert.Equal(System.Text.Json.JsonValueKind.Object, doc.RootElement.GetProperty("errors").ValueKind); + } + [Fact] public async Task CancelDraft_Succeeds_CancelConfirmed_409() { diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index 430de6207..1a3aa3391 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -25,10 +25,10 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) // Program.cs pulls DI-registered sinks into the logger via // ReadFrom.Services; this hands the test a live tap on every event. services.AddSingleton(Sink); - // Outside Development, minimal-API binding failures return 400 - // without throwing; the exception-path test needs the real throw. - services.Configure( - o => o.ThrowOnBadRequest = true); + // #398 — RouteHandlerOptions.ThrowOnBadRequest=true (so a binding + // failure really throws, which the exception-path test below + // needs) is now Program.cs's own global registration, not a + // per-factory override — every environment behaves the same way. }); } diff --git a/web/src/i18n/en.ts b/web/src/i18n/en.ts index 8418c6885..353ec3915 100644 --- a/web/src/i18n/en.ts +++ b/web/src/i18n/en.ts @@ -314,6 +314,10 @@ export const en = { atMostDecimals: "At most {{count}} decimal places for this currency.", enterAmountGreaterThanZero: "Enter an amount greater than zero.", invalidUnitPrice: "Invalid unit price.", + // #398 — sales quantities are whole selling units; the add-line and + // edit-line quantity controls reject a fractional value before sending, + // rather than letting the server's JSON binding fail. + quantityMustBeWholeNumber: "Quantity must be a whole number.", loadSalesDataFailed: "Could not load sales data. Is the API up?", loadOrdersFailed: "Could not load orders.", loadPaymentsFailed: "Could not load this order's payments.", @@ -2133,9 +2137,10 @@ export const en = { salesHeading: "Customers & sales", salesDrafts: "Orders start as drafts: add lines by picking a product, a packed " - + "unit (dozen, carton, …), a quantity, and a price per unit (prefilled from the product's default) — " - + "edit freely, or cancel (the draft is kept, read-only). Each line remembers how many " - + "eggs its unit held when it was added, so redefining a carton later never changes old orders.", + + "unit (dozen, carton, …), a whole-number quantity, and a price per unit (prefilled from the " + + "product's default, decimals allowed) — edit freely, or cancel (the draft is kept, " + + "read-only). Each line remembers how many eggs its unit held when it was added, so redefining a " + + "carton later never changes old orders.", salesConfirming: "Confirming an order allocates real stock — oldest lots first — and is the point " + "where inventory changes hands.", @@ -2465,8 +2470,8 @@ export const en = { glossarySalesLineTerm: "Sales line", glossarySalesLineDef: - "One product on an order: quantity in selling units, priced per unit; the eggs behind it are " - + "quantity × the unit's egg count.", + "One product on an order: a whole-number quantity in selling units, priced per unit (the price may " + + "have decimals); the eggs behind it are quantity × the unit's egg count.", glossaryConfirmOrderTerm: "Confirm (order)", glossaryConfirmOrderDef: "Turns a draft order into a real sale and allocates stock. Undone only by voiding.", diff --git a/web/src/i18n/es.ts b/web/src/i18n/es.ts index dff86d181..548fb27ee 100644 --- a/web/src/i18n/es.ts +++ b/web/src/i18n/es.ts @@ -287,6 +287,8 @@ export const es = { atMostDecimals: "Como máximo {{count}} decimales para esta moneda.", enterAmountGreaterThanZero: "Ingrese un monto mayor que cero.", invalidUnitPrice: "Precio unitario inválido.", + // machine-drafted (#182) — pending native review. + quantityMustBeWholeNumber: "La cantidad debe ser un número entero.", loadSalesDataFailed: "No se pudieron cargar los datos de ventas. ¿Está la API activa?", loadOrdersFailed: "No se pudieron cargar los pedidos.", loadPaymentsFailed: "No se pudieron cargar los pagos de este pedido.", @@ -1807,11 +1809,11 @@ export const es = { salesHeading: "Clientes y ventas", salesDrafts: "Los pedidos comienzan como borradores: agregue líneas eligiendo un " - + "producto, una unidad empacada (docena, cartón, …), una cantidad, y un precio por " - + "unidad (precargado desde el valor predeterminado del producto) — edite libremente, o " - + "cancele (el borrador se conserva, de solo lectura). Cada línea recuerda cuántos " - + "huevos contenía su unidad cuando se agregó, así que redefinir un cartón más tarde nunca cambia " - + "pedidos antiguos.", + + "producto, una unidad empacada (docena, cartón, …), una cantidad entera, y un " + + "precio por unidad (precargado desde el valor predeterminado del producto, se permiten decimales) — " + + "edite libremente, o cancele (el borrador se conserva, de solo lectura). Cada línea " + + "recuerda cuántos huevos contenía su unidad cuando se agregó, así que redefinir un cartón más tarde " + + "nunca cambia pedidos antiguos.", salesConfirming: "Confirmar un pedido asigna existencias reales — los lotes más antiguos primero — y " + "es el punto donde el inventario cambia de manos.", @@ -2153,8 +2155,9 @@ export const es = { glossarySalesLineTerm: "Línea de venta", glossarySalesLineDef: - "Un producto en un pedido: cantidad en unidades de venta, con precio por unidad; los huevos detrás " - + "de ella son la cantidad × el conteo de huevos de la unidad.", + "Un producto en un pedido: una cantidad entera en unidades de venta, con precio por unidad (el " + + "precio puede tener decimales); los huevos detrás de ella son la cantidad × el conteo de huevos de " + + "la unidad.", glossaryConfirmOrderTerm: "Confirmar (pedido)", glossaryConfirmOrderDef: diff --git a/web/src/i18n/tl.ts b/web/src/i18n/tl.ts index bc499da21..6489d1152 100644 --- a/web/src/i18n/tl.ts +++ b/web/src/i18n/tl.ts @@ -299,6 +299,8 @@ export const tl = { atMostDecimals: "Pinakamarami {{count}} decimal para sa currency na ito.", enterAmountGreaterThanZero: "Maglagay ng halagang higit sa zero.", invalidUnitPrice: "Di-wastong presyo bawat yunit.", + // machine-drafted (#182) — pending native review. + quantityMustBeWholeNumber: "Dapat buong bilang ang dami.", loadSalesDataFailed: "Hindi na-load ang datos ng benta. Gumagana ba ang API?", loadOrdersFailed: "Hindi na-load ang mga order.", loadPaymentsFailed: "Hindi na-load ang mga bayad ng order na ito.", @@ -1877,11 +1879,11 @@ export const tl = { salesHeading: "Mga Customer at Benta", salesDrafts: "Nagsisimula ang mga order bilang draft: magdagdag ng linya sa pamamagitan ng " - + "pagpili ng produkto, isang packed unit (dosena, karton, …), isang dami, at isang " - + "presyo kada unit (naka-prefill mula sa default ng produkto) — i-edit nang malaya, o " - + "kanselahin (mananatili ang draft, read-only). Naaalala ng bawat linya kung ilang " - + "itlog ang laman ng unit nito noong idinagdag ito, kaya hindi kailanman binabago ng muling " - + "pagtukoy sa isang karton ang mga lumang order.", + + "pagpili ng produkto, isang packed unit (dosena, karton, …), isang buong bilang " + + "na dami, at isang presyo kada unit (naka-prefill mula sa default ng produkto, pinapayagan ang " + + "decimal) — i-edit nang malaya, o kanselahin (mananatili ang draft, read-only). " + + "Naaalala ng bawat linya kung ilang itlog ang laman ng unit nito noong idinagdag ito, kaya hindi " + + "kailanman binabago ng muling pagtukoy sa isang karton ang mga lumang order.", salesConfirming: "Ang Pagkumpirma ng isang order ay naglalaan ng aktwal na stock — pinakalumang lote " + "muna — at ito ang sandali kung saan lumilipat ang inventory.", @@ -2227,8 +2229,9 @@ export const tl = { glossarySalesLineTerm: "Linya ng benta", glossarySalesLineDef: - "Isang produkto sa isang order: dami sa selling unit, may presyo kada unit; ang mga itlog sa likod " - + "nito ay dami × ang bilang ng itlog ng unit.", + "Isang produkto sa isang order: isang buong bilang na dami sa selling unit, may presyo kada unit " + + "(maaaring may decimal ang presyo); ang mga itlog sa likod nito ay dami × ang bilang ng itlog ng " + + "unit.", glossaryConfirmOrderTerm: "Kumpirmahin (order)", glossaryConfirmOrderDef: diff --git a/web/src/routes/SalesPage.test.tsx b/web/src/routes/SalesPage.test.tsx index 2ca9bdd8e..10ae2c01f 100644 --- a/web/src/routes/SalesPage.test.tsx +++ b/web/src/routes/SalesPage.test.tsx @@ -225,6 +225,49 @@ describe("SalesPage quantity steppers (#250)", () => { }); }); +// #398 — a fractional quantity (e.g. 2.5) used to reach the server and fail +// during minimal-API JSON binding (Quantity is an int), surfacing the raw +// internal "Failed to read parameter ..." message. These pin the CLIENT-side +// half of the fix: reject before any network call, with a localized message. +// NumberField's typed input isn't step-constrained (no wrapping
, per +// the "Deliberately NOT a " comment in SalesPage.tsx), so typing "2.5" +// really does land a fractional value in `qty`/`editQty` here, same as #250's +// steppers tests above prove integer steps land cleanly. +describe("SalesPage quantity must be a whole number (#398)", () => { + it("rejects a fractional add-line quantity before sending, with a localized message", async () => { + await renderReady(); + await createDraft(draftEmpty(2, "USD")); + + const qty = screen.getByRole("spinbutton", { name: "Quantity" }); + fireEvent.change(qty, { target: { value: "2.5" } }); + expect(qty).toHaveValue(2.5); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Add line" })); + }); + + expect(mockAddOrderItem).not.toHaveBeenCalled(); + expect(await screen.findByText(i18n.t("sales:quantityMustBeWholeNumber"))).toBeInTheDocument(); + }); + + it("rejects a fractional inline-edit quantity before sending, with a localized message", async () => { + const row = await openOrder(DRAFT_TWO, /Grade A Dozen/); + fireEvent.click(within(row).getByRole("button", { name: "edit" })); + const editRow = screen.getByRole("row", { name: /Grade A Dozen/ }); + + const qty = within(editRow).getByRole("spinbutton", { name: "Edit quantity" }); + fireEvent.change(qty, { target: { value: "1.5" } }); + expect(qty).toHaveValue(1.5); + + await act(async () => { + fireEvent.click(within(editRow).getByRole("button", { name: "save" })); + }); + + expect(mockUpdateOrderItem).not.toHaveBeenCalled(); + expect(await screen.findByText(i18n.t("sales:quantityMustBeWholeNumber"))).toBeInTheDocument(); + }); +}); + describe("SalesPage line display", () => { it("shows per-line base eggs and money, with the order total distinct from any single line", async () => { const rowA = await openOrder(DRAFT_TWO, /Grade A Dozen/); diff --git a/web/src/routes/SalesPage.tsx b/web/src/routes/SalesPage.tsx index 44698e108..c1578b09c 100644 --- a/web/src/routes/SalesPage.tsx +++ b/web/src/routes/SalesPage.tsx @@ -250,6 +250,12 @@ export function SalesPage() { const onAddItem = () => run("add-item", async () => { if (!active) return; + // #398 — sales quantities are whole selling units; reject a fractional + // value BEFORE sending rather than letting the server's JSON binding + // fail with an internal parameter-binding message. NumberField's typed + // input isn't step-constrained (no wrapping — see the comment + // below), so `qty` can legitimately hold e.g. 2.5 here. + if (!Number.isInteger(qty)) throw new Error(i18n.t("sales:quantityMustBeWholeNumber")); // Empty price → omit it: the server falls back to the product's default. let minorUnits: number | undefined; if (price.trim() !== "") { @@ -266,6 +272,8 @@ export function SalesPage() { const onUpdateItem = (itemId: string) => run(`update-item:${itemId}`, async () => { if (!active) return; + // #398 — same whole-number guard as the add-line control, above. + if (!Number.isInteger(editQty)) throw new Error(i18n.t("sales:quantityMustBeWholeNumber")); const minorUnits = parseMoneyToMinorUnits(editPrice, active.currencyMinorUnit); if (!Number.isFinite(minorUnits) || minorUnits < 0) throw new Error(i18n.t("sales:invalidUnitPrice")); const scope = `update-item:${itemId}`; From 9e593283bc7b07ec3f51ef7dba2bc610e392502b Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 17:40:43 -0700 Subject: [PATCH 02/11] fix(api): stop a JSON-binding 400 from being mis-logged as a 500 RouteHandlerOptions.ThrowOnBadRequest=true (#398) makes a binding failure (fractional quantity, bad date, malformed JSON, ...) throw a BadHttpRequestException that propagates through UseSerilogRequestLogging on its way to the correct 400 at /error. Serilog.AspNetCore hardcodes StatusCode 500 at Error for any exception that passes through it, so the client's correct 400 was mis-logged as a server fault, inflating 5xx/error telemetry. Add BindingFailureResponse, registered immediately after UseSerilogRequestLogging (inside it, so Serilog never sees the exception). It catches a sub-500 BadHttpRequestException, answers the 400 itself for the JSON-binding case, and rethrows everything else (413/415, an already- started response) untouched so their existing handling is undisturbed. Factor the ValidationProblem body shape out to ValidationResponse.BindingFailureProblem() so the new middleware and the /error backstop mapping share one definition instead of two copies that can drift. Add/adjust RequestLoggingTests: a binding failure now logs exactly one completion at Information/400 with the same client-visible body; the existing unhandled-exception guarantee (Error/500, no /error double-log) moves to a genuine fault trigger since malformed JSON no longer reaches Serilog as an exception. --- .../Hosting/BindingFailureResponse.cs | 61 ++++++++++ src/Cluckwork.Api/Program.cs | 38 +++++- .../Validation/ValidationResponse.cs | 19 +++ .../RequestLoggingTests.cs | 108 ++++++++++++++++-- 4 files changed, 212 insertions(+), 14 deletions(-) create mode 100644 src/Cluckwork.Api/Hosting/BindingFailureResponse.cs diff --git a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs new file mode 100644 index 000000000..4bd973a62 --- /dev/null +++ b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs @@ -0,0 +1,61 @@ +namespace Cluckwork.Api.Hosting; + +using Cluckwork.Api.Validation; + +// #398 review (Codex) — RouteHandlerOptions.ThrowOnBadRequest is forced true +// in every environment (see Program.cs), so a JSON-binding failure (a +// fractional quantity into an int, an unparseable date/guid, malformed JSON +// syntax, …) throws a BadHttpRequestException deep inside minimal-API's +// generated body-reader instead of the framework silently setting a 400 and +// returning. Before this middleware existed, that exception propagated all +// the way up through UseSerilogRequestLogging (registered ABOVE this +// middleware in Program.cs's pipeline) before UseExceptionHandler (registered +// above THAT) finally caught it and mapped it to the correct 400 +// ValidationProblem at /error. Serilog.AspNetCore's RequestLoggingMiddleware +// catches ANY exception that passes through it and logs that request's +// completion with a HARDCODED StatusCode 500 at Error — so an ordinary +// malformed body (a fractional quantity, a bad date, scanner traffic) was +// logged as a 500 server fault while the client correctly received a 400. +// That inflated 5xx/error telemetry and would trigger false alerts; +// RequestLoggingTests pins the fix. +// +// Registered in Program.cs IMMEDIATELY AFTER UseSerilogRequestLogging — i.e. +// INSIDE it in the pipeline — so THIS middleware is the first thing to see +// the exception. It catches it, writes the real 400 body itself, and +// (critically) does NOT rethrow: control returns to Serilog with a normal +// 400 response and no exception in flight, so GetLevel's ordinary +// "exception is not null || StatusCode >= 500" check sees neither and logs +// Information — exactly like any other well-formed 400. No re-execution at +// /error happens either, since UseExceptionHandler (which sits ABOVE Serilog) +// never sees anything propagate that far. +public static class BindingFailureResponse +{ + public static IApplicationBuilder UseCluckworkBindingFailureResponse(this IApplicationBuilder app) => + app.Use(static async (context, next) => + { + try + { + await next(); + } + catch (BadHttpRequestException ex) when (ex.StatusCode < StatusCodes.Status500InternalServerError) + { + // A NON-400 BadHttpRequestException — 413 from the #309 + // request-body cap's rare non-JSON-bound-endpoint escape, 415 + // from an unrecognised Content-Type, … — is NOT this + // middleware's concern. Rethrow untouched so it keeps + // flowing to the existing /error mapping exactly as before + // this change: RequestBodyLimit.cs's WriteBodyTooLargeAsync + // depends on THAT handler staying byte-identical to its own + // 413 shape. + // + // Same for a response that has already started: writing here + // would throw a second, more confusing exception on top of + // the first (Response.HasStarted), so rethrow and let it + // propagate as an ordinary unhandled exception instead. + if (ex.StatusCode != StatusCodes.Status400BadRequest || context.Response.HasStarted) + throw; + + await ValidationResponse.BindingFailureProblem().ExecuteAsync(context); + } + }); +} diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index b5a2ad6be..5f30cb217 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -233,6 +233,17 @@ // suite runs many) reassigns and disposes — completions would silently go // to another host's pipeline. options.Logger = app.Services.GetRequiredService(); + // No dedicated BadHttpRequestException arm here (#398 review, Codex): a + // 400 binding failure is now caught by UseCluckworkBindingFailureResponse + // immediately below, which does NOT rethrow, so by the time a completion + // reaches this callback it is either (a) an ordinary sub-500 response + // with `exception` null — falls through to Information via the existing + // status-code check, no special-casing needed — or (b) a genuine + // unhandled exception (anything that middleware didn't catch, including + // a non-400 BadHttpRequestException like the #309 413/415 cases), which + // correctly stays Error. Adding an arm for the exception TYPE here would + // be dead code: nothing reaching this point is a 400 + // BadHttpRequestException anymore. options.GetLevel = (httpContext, _, exception) => // Health first — a FAILING probe (503 during a DB outage) must not // escalate to Error while orchestrators poll every few seconds. The @@ -247,6 +258,13 @@ : Serilog.Events.LogEventLevel.Information; }); +// #398 review (Codex) — must be registered IMMEDIATELY after +// UseSerilogRequestLogging (i.e. INSIDE it), so a JSON-binding failure is +// caught and answered here, before Serilog's own exception handling ever +// sees it. Full reasoning in Hosting/BindingFailureResponse.cs. Does not +// change the relative order of anything already below it. +app.UseCluckworkBindingFailureResponse(); + // Endpoint rate-limit policies (#143) — no global limiter, only routes that // opt in via RequireRateLimiting are affected. app.UseRateLimiter(); @@ -427,8 +445,19 @@ // THIS specific case — always StatusCode 400 by construction, the // same default every JSON-binding BadHttpRequestException carries — // through the SAME ValidationProblem shape every other 400 in this - // app uses (ValidationResponse.Problem), instead of echoing the - // exception text. + // app uses (ValidationResponse.BindingFailureProblem), instead of + // echoing the exception text. + // + // #398 review (Codex) — this branch is now a BACKSTOP, not the + // primary path: UseCluckworkBindingFailureResponse + // (Hosting/BindingFailureResponse.cs) intercepts the overwhelming + // majority of these before they ever reach here, specifically so + // Serilog's request-logging middleware (which sits between it and + // here) never sees the exception and mis-logs the completion as a + // 500 at Error for what the client correctly receives as a 400. This + // mapping stays — unreachable only in the ordinary case — for any + // binding failure that somehow bypasses that middleware (e.g. the + // response had already started). // // A NON-400 BadHttpRequestException (413 from the #309 request-body // cap, 415 from an unrecognised Content-Type, …) falls through to the @@ -438,10 +467,7 @@ // instead of being short-circuited there — don't fold that case into // the 400-only ValidationProblem mapping above. BadHttpRequestException { StatusCode: StatusCodes.Status400BadRequest } => - ValidationResponse.Problem(new Dictionary - { - ["body"] = ["The request body has an invalid or incorrectly formatted value."], - }), + ValidationResponse.BindingFailureProblem(), BadHttpRequestException bad => Results.Problem( detail: bad.Message, statusCode: bad.StatusCode, diff --git a/src/Cluckwork.Api/Validation/ValidationResponse.cs b/src/Cluckwork.Api/Validation/ValidationResponse.cs index e86bfa3da..0fe0e47da 100644 --- a/src/Cluckwork.Api/Validation/ValidationResponse.cs +++ b/src/Cluckwork.Api/Validation/ValidationResponse.cs @@ -33,6 +33,25 @@ public static IResult Problem(ValidationResult validation) public static IResult Problem(IDictionary errors) => Results.ValidationProblem(errors); + // #398 review (Codex) — the canonical ValidationProblem shape for a + // JSON-binding failure (a fractional quantity into an int, an + // unparseable date/guid, malformed JSON syntax, …): a + // BadHttpRequestException with StatusCode 400, thrown deep inside + // minimal-API's generated body-reader before any FluentValidation + // validator or handler runs. TWO sites render this and must never drift + // apart, hence the single factory: BindingFailureResponse + // (Hosting/BindingFailureResponse.cs), the primary path — it intercepts + // the exception INSIDE UseSerilogRequestLogging so the failure never + // reaches Serilog's own completion log as a (mis-logged 500/Error) + // exception; and the `/error` mapping in Program.cs, kept as the + // backstop for any binding failure that somehow bypasses that + // middleware (e.g. the response had already started). + public static IResult BindingFailureProblem() => + Problem(new Dictionary + { + ["body"] = ["The request body has an invalid or incorrectly formatted value."], + }); + // Exposed for unit testing without executing an IResult. public static (Dictionary Errors, Dictionary? Codes) Build(ValidationResult validation) diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index 1a3aa3391..1930860be 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -3,12 +3,33 @@ namespace Cluckwork.Api.IntegrationTests; using System.Collections.Concurrent; using System.Diagnostics; using Cluckwork.Api.IntegrationTests.Infrastructure; +using Cluckwork.Application.Features.Users.ChangeOwnPassword; +using FluentValidation; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; using Serilog.Core; using Serilog.Events; +// #398 review (Codex) — a genuine unhandled fault for +// Unhandled_exception_logs_one_completion_for_the_real_path_only below. That +// test used to provoke malformed JSON at /api/v1/auth/login, but that request +// no longer reaches this middleware as an exception at all: BindingFailureResponse +// (Hosting/BindingFailureResponse.cs) catches it before Serilog ever sees it +// (see Binding_failure_logs_one_completion_at_information_with_status_400 +// instead). This class needs its own genuine, non-binding fault to keep +// proving the Error/500 guarantee. Mirrors ExceptionHandlerReExecutionTests.cs's +// validator-replacement pattern: the throw originates INSIDE the endpoint — +// downstream of IdempotencyMiddleware, exactly where a real handler fault +// would — rather than from binding. +internal sealed class ThrowingChangeOwnPasswordValidatorForRequestLogging + : AbstractValidator +{ + public ThrowingChangeOwnPasswordValidatorForRequestLogging() => + RuleFor(c => c.NewPassword).Custom((_, _) => + throw new InvalidOperationException("unhandled-exception probe")); +} + // #214 — every request emits one structured completion log (method, path, // status, elapsed) carrying the request's TraceId, so a single id joins the // request line, handler logs, and exported spans. Health probes are demoted @@ -26,9 +47,10 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) // ReadFrom.Services; this hands the test a live tap on every event. services.AddSingleton(Sink); // #398 — RouteHandlerOptions.ThrowOnBadRequest=true (so a binding - // failure really throws, which the exception-path test below + // failure really throws, which the binding-failure test below // needs) is now Program.cs's own global registration, not a // per-factory override — every environment behaves the same way. + services.AddScoped, ThrowingChangeOwnPasswordValidatorForRequestLogging>(); }); } @@ -119,10 +141,62 @@ public async Task Failing_readiness_probe_is_not_logged_at_error() // An unhandled exception re-executes the pipeline at /error; without // demotion that produced TWO completion lines per failed request (codex + - // agent review of PR #226). The malformed-JSON body is the one guaranteed - // in-repo trigger: minimal-API binding throws BadHttpRequestException. + // agent review of PR #226). Malformed JSON used to be the guaranteed + // in-repo trigger (minimal-API binding throws BadHttpRequestException), + // but #398 review (Codex) gave THAT specific exception its own middleware + // (BindingFailureResponse) that catches it before Serilog ever sees it — + // see Binding_failure_logs_one_completion_at_information_with_status_400 + // for that case. This test now needs a genuine, non-binding fault instead + // (ThrowingChangeOwnPasswordValidatorForRequestLogging, registered above) + // to keep proving the guarantee for a REAL unhandled exception. [Fact] public async Task Unhandled_exception_logs_one_completion_for_the_real_path_only() + { + var email = $"reqlog-fault-{Guid.NewGuid():N}@test.local"; + await factory.SeedAccountWithUserAsync(email); + var client = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(email)); + + var response = await client.PostAsJsonAsync("/api/v1/auth/change-password", new + { + currentPassword = TestHarness.Password, + newPassword = $"{Guid.NewGuid():N}aA1!" + }); + + Assert.Equal(System.Net.HttpStatusCode.InternalServerError, response.StatusCode); + // The exception-pass completion (Serilog logs these with StatusCode 500 + // + the exception attached) — filtered by status so other tests' + // traffic on this path can't interfere. + var completion = Assert.Single(CompletionEventsFor("/api/v1/auth/change-password"), + e => ScalarOf(e, "StatusCode") == "500"); + Assert.Equal(LogEventLevel.Error, completion.Level); + Assert.Empty(CompletionEventsFor("/error")); + } + + // #398 review (Codex) — the regression this branch fixes, and the + // client-visible half of that same fix, pinned together from ONE request. + // Filtered by StatusCode, like Unhandled_exception_logs_... above: other + // tests' factory.LoginForAccessTokenAsync setup calls ALSO hit + // /api/v1/auth/login on the same shared, never-reset factory.Sink — those + // always succeed (200), so filtering this request's own 400 out keeps + // this test's assertion independent of what else in the class ran first. + // + // The regression: a JSON-binding failure (RouteHandlerOptions. + // ThrowOnBadRequest, forced true in every environment) is a + // BadHttpRequestException that, unhandled, propagates through + // UseSerilogRequestLogging on its way to being correctly mapped to a 400 + // at /error. Serilog.AspNetCore hardcodes StatusCode 500 at Error for ANY + // exception that passes through it, so the client's correct 400 was + // being mis-logged as a server fault — inflating 5xx/error telemetry and + // risking false alerts. BindingFailureResponse + // (Hosting/BindingFailureResponse.cs) now answers the request itself, + // one layer inside Serilog, so Serilog only ever sees a normal 400 + // completion — and moving WHERE the response is written must not change + // WHAT is written, so this also pins the exact ValidationProblem shape + // ValidationResponse.BindingFailureProblem() produces (the single + // factory both BindingFailureResponse and the /error backstop in + // Program.cs call, so they cannot drift apart). + [Fact] + public async Task Binding_failure_logs_one_completion_at_information_with_status_400() { var client = factory.CreateClient(); var malformed = new StringContent("{not json", System.Text.Encoding.UTF8, "application/json"); @@ -130,12 +204,30 @@ public async Task Unhandled_exception_logs_one_completion_for_the_real_path_only var response = await client.PostAsync("/api/v1/auth/login", malformed); Assert.Equal(System.Net.HttpStatusCode.BadRequest, response.StatusCode); - // The exception-pass completion (Serilog logs these with StatusCode 500 - // + the exception attached) — filtered by status so the healthy logins - // other tests perform can't interfere. - Assert.Single(CompletionEventsFor("/api/v1/auth/login"), - e => ScalarOf(e, "StatusCode") == "500"); + + // (1) Exactly one completion for THIS request, at Information, + // StatusCode 400 — not 500, not Error — and no exception-handler + // re-execution at /error (the failure never propagated that far up + // the pipeline). + var completion = Assert.Single(CompletionEventsFor("/api/v1/auth/login"), + e => ScalarOf(e, "StatusCode") == "400"); + Assert.Equal(LogEventLevel.Information, completion.Level); Assert.Empty(CompletionEventsFor("/error")); + + // (3) The client-visible body is still the ValidationProblem shape + // #398 introduced, not a bare 400 and not the framework's raw + // binding-exception text. + Assert.Equal("application/problem+json", response.Content.Headers.ContentType?.MediaType); + var body = await response.Content.ReadAsStringAsync(); + Assert.DoesNotContain("Failed to read parameter", body, StringComparison.Ordinal); + Assert.DoesNotContain("Utf8JsonReader", body, StringComparison.Ordinal); + using var doc = System.Text.Json.JsonDocument.Parse(body); + Assert.Equal(400, doc.RootElement.GetProperty("status").GetInt32()); + var bodyErrors = doc.RootElement.GetProperty("errors").GetProperty("body"); + Assert.Equal(System.Text.Json.JsonValueKind.Array, bodyErrors.ValueKind); + Assert.Equal( + "The request body has an invalid or incorrectly formatted value.", + bodyErrors[0].GetString()); } // Spec §10: account_id on every log scope. The tenant middleware feeds the From 4f0ae3db9d19279b63cbdf684a24e10b33ce60d9 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 17:44:02 -0700 Subject: [PATCH 03/11] docs(help): say a sale quantity is a whole number (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stepper note ended "Prices and fractional amounts are still typed", which reads as permission to type a fractional QUANTITY — the exact input this PR makes an error. Replaced with the whole-number rule in en/es/tl, and covered by a HelpPage test that also asserts the retired sentence is gone rather than merely joined by the new one. AGENTS.md requires the SPA Help page to move with user-visible behaviour in the same PR; this PR had updated the GLOSSARY and i18n but not Help. --- web/src/i18n/en.ts | 3 ++- web/src/i18n/es.ts | 3 ++- web/src/i18n/tl.ts | 3 ++- web/src/routes/HelpPage.test.tsx | 18 ++++++++++++++++++ 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/web/src/i18n/en.ts b/web/src/i18n/en.ts index 353ec3915..980c65eaf 100644 --- a/web/src/i18n/en.ts +++ b/web/src/i18n/en.ts @@ -1991,7 +1991,8 @@ export const en = { dialogsSteppers: "Whole-number counts — egg counts, bird counts, sale quantities, eggs per unit — have thumb-sized " + "− and + buttons: tap for one, hold to speed up. " - + "A sale line's quantity never steps below 1. Prices and fractional amounts are still typed.", + + "A sale line's quantity never steps below 1, and it is always a whole number — " + + "stepped or typed. Decimals belong in prices, which are typed.", dialogsConfirm: "Actions that cannot be undone ask first. Submitting a day, confirming or cancelling " + "an order, depleting or archiving a flock — each one says what is about to happen and waits. The " diff --git a/web/src/i18n/es.ts b/web/src/i18n/es.ts index 548fb27ee..a409236ec 100644 --- a/web/src/i18n/es.ts +++ b/web/src/i18n/es.ts @@ -1658,7 +1658,8 @@ export const es = { "Los conteos de números enteros — conteos de huevos, conteos de aves, cantidades de venta, huevos por " + "unidad — tienen botones − y + del tamaño del pulgar: toque para " + "uno, mantenga presionado para acelerar. La cantidad de una línea de venta nunca " - + "baja de 1. Los precios y montos fraccionarios se siguen escribiendo.", + + "baja de 1 y siempre es un número entero, ya sea con los botones o escrita. Los " + + "decimales van en los precios, que se escriben.", dialogsConfirm: "Las acciones que no se pueden deshacer preguntan primero. Enviar un día, confirmar " + "o cancelar un pedido, agotar o archivar un lote — cada una indica lo que está por suceder y " diff --git a/web/src/i18n/tl.ts b/web/src/i18n/tl.ts index 6489d1152..33dbb5410 100644 --- a/web/src/i18n/tl.ts +++ b/web/src/i18n/tl.ts @@ -1722,7 +1722,8 @@ export const tl = { "Ang mga bilang na buong numero — bilang ng itlog, bilang ng ibon, dami ng benta, itlog bawat unit — " + "ay may − at + na button na kasya sa hinlalaki: i-tap para sa isa, " + "pindutin nang matagal para bumilis. Ang dami ng isang linya ng benta ay hindi " - + "bababa sa 1. Ang mga presyo at fractional na halaga ay tina-type pa rin.", + + "bababa sa 1 at palaging buong numero, gamit man ang button o tina-type. Ang mga " + + "desimal ay para sa presyo, na tina-type.", dialogsConfirm: "Nagtatanong muna ang mga aksyong hindi na maaaring i-undo. Ang pagsumite ng isang " + "araw, pagkumpirma o pagkansela ng order, pag-ubos o pag-archive ng kawan — sinasabi ng bawat isa " diff --git a/web/src/routes/HelpPage.test.tsx b/web/src/routes/HelpPage.test.tsx index 8188cc329..66b4cd676 100644 --- a/web/src/routes/HelpPage.test.tsx +++ b/web/src/routes/HelpPage.test.tsx @@ -352,4 +352,22 @@ describe("HelpPage glossary i18n wiring (#182, Task 33)", () => { }, ); }); + + // #398 — the SPA now refuses a fractional sale-line quantity before sending. + // The stepper note used to end "Prices and fractional amounts are still + // typed", which reads as permission to TYPE a fractional quantity — the exact + // thing that is now an error. The assertion has to be about that sentence + // saying quantities are whole numbers, not merely that the section renders: + // the section rendered perfectly happily with the misleading copy in it. + it("tells the reader a sale quantity is a whole number, not that fractional amounts are typeable (#398)", () => { + render(); + expect(screen.getByText(/always a/i)).toBeInTheDocument(); + expect(screen.getByText(/stepped or typed/i)).toBeInTheDocument(); + expect(screen.getByText(/Decimals belong in prices/i)).toBeInTheDocument(); + // The retired wording must be gone, not merely joined by the new sentence — + // leaving both in place would still tell a worker fractional amounts are + // acceptable input for a quantity. + expect(screen.queryByText(/Prices and fractional amounts are still typed/i)) + .not.toBeInTheDocument(); + }); }); From 1e1435c1e18c8b9ca9ce91a3c94df985750132f5 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 18:23:47 -0700 Subject: [PATCH 04/11] test(api): pin the streamed 413's telemetry, not just its response shape (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 2 predicted that forcing ThrowOnBadRequest=true breaks the streamed/chunked 413: RequestBodyLimit's recovery is a post-next() STATUS CHECK rather than a catch, so if the generated binder rethrew instead of swallowing ByteCappedRequestStream's 413, next() would throw, the recovery would never run, and Serilog would record the hardcoded 500/Error. Measured: it does not. The binder still swallows that exception, the recovery still runs, and the completion is logged Information/413. AuthBodyLimitTests already pinned the response shape (9/9 green), but no test could see the telemetry — and a mis-logged completion is invisible to every response-level assertion, which is how the original #398 defect survived in the first place. So this pins it directly. Mutation-checked against exactly the predicted path: making the recovery rethrow instead of writing turns the test RED; restoring it green. 17/17 across RequestLoggingTests + AuthBodyLimitTests. Filters by status rather than Assert.Single on the path — the sink is class-scoped and never reset, and other tests reach /auth/login via the login helper. --- .../RequestLoggingTests.cs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index 1930860be..fc3d98b95 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -246,6 +246,54 @@ public async Task Authenticated_request_completion_carries_the_account_id() var completion = Assert.Single(CompletionEventsFor("/api/v1/customers")); Assert.Equal(accountId.ToString(), ScalarOf(completion, "AccountId")); } + + // #398 review round 2 (Codex) — the STREAMED 413, which is a different path + // from the 400 above and was not covered by anything. + // + // RequestBodyLimit's recovery for this case is a post-`next()` STATUS CHECK, + // not a catch: it relies on minimal-API's generated binder catching + // ByteCappedRequestStream's 413 itself, setting Response.StatusCode = 413, + // and returning WITHOUT rethrowing. Forcing ThrowOnBadRequest=true globally + // (#398) is exactly the kind of change that could invalidate that premise — + // if the binder rethrew instead, `next()` would throw, the recovery would + // never run, BindingFailureResponse would rethrow it (413 != 400), and + // Serilog would log the hardcoded 500/Error this PR exists to eliminate. + // + // AuthBodyLimitTests pins the RESPONSE shape for this path and still passes, + // but a response assertion cannot see a mis-logged completion — the original + // #398 defect was invisible to every response-level test in the suite. So + // assert the telemetry directly: one completion, Information, 413. + [Fact] + public async Task Streamed_413_logs_one_completion_at_information_with_status_413() + { + var client = factory.CreateClient(); + + // Declares 10 bytes, actually sends ~8 KB: the cap is breached mid-read, + // inside the generated binder, rather than by the declared-length + // short-circuit that never reaches binding at all. + var payload = System.Text.Encoding.UTF8.GetBytes( + $"{{\"email\":\"nobody@example.com\",\"password\":\"{new string('a', 8192)}\"}}"); + var content = new StreamContent(new MemoryStream(payload)); + content.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("application/json"); + content.Headers.ContentLength = 10; + + var response = await client.SendAsync( + new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/login") { Content = content }); + + Assert.Equal(System.Net.HttpStatusCode.RequestEntityTooLarge, response.StatusCode); + + // Filter by status rather than Assert.Single on the path: the sink is + // class-scoped and never reset, and several other tests here reach + // /api/v1/auth/login through the login helper, so a bare Single() fails + // on their traffic rather than on this guarantee. Exactly one 413 + // completion is still the assertion — a re-executed /error would add a + // second, and a mis-logged 500 would leave zero. + var completion = Assert.Single( + CompletionEventsFor("/api/v1/auth/login"), + e => ScalarOf(e, "StatusCode") == "413"); + Assert.Equal(LogEventLevel.Information, completion.Level); + Assert.Null(completion.Exception); + } } // Own collection (not "integration"): this class uses its own factory/container From db68b822bf41694ee4f9e7f609ed07b261a0dcc6 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 19:19:29 -0700 Subject: [PATCH 05/11] test(api): use the shared NonSeekableStream for the streamed-413 probe (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex proposed swapping the MemoryStream for a non-seekable one. Its stated risk — that a seekable stream lets HttpClient compute the real length and divert to the declared-length short-circuit — does not hold, because the Content-Length header is set explicitly and wins over TryComputeLength; the mutation check already proved the streamed path was taken, since it mutates RequestBodyLimit's POST-next() recovery, which the declared-length branch returns before ever reaching. The concern underneath it is real, though: both paths answer 413, so nothing in the test pinned WHICH one it took. A later change in transport handling could drift it onto the short-circuit and it would still pass while proving nothing about the streamed path — a test passing for the wrong reason. So: adopt the non-seekable stream, but share AuthBodyLimitTests' existing one rather than duplicating ~35 lines. Two copies of a helper whose whole job is to pin a transport shape could drift apart, at which point one of the two tests silently stops exercising the path it names. Mutation re-run after the change: recovery rethrows -> RED, restored -> GREEN. 17/17 across both classes. --- .../AuthBodyLimitTests.cs | 24 ----------- .../Infrastructure/NonSeekableStream.cs | 40 +++++++++++++++++++ .../RequestLoggingTests.cs | 9 ++++- 3 files changed, 48 insertions(+), 25 deletions(-) create mode 100644 tests/Cluckwork.Api.IntegrationTests/Infrastructure/NonSeekableStream.cs diff --git a/tests/Cluckwork.Api.IntegrationTests/AuthBodyLimitTests.cs b/tests/Cluckwork.Api.IntegrationTests/AuthBodyLimitTests.cs index b3cafd16e..c672136b2 100644 --- a/tests/Cluckwork.Api.IntegrationTests/AuthBodyLimitTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/AuthBodyLimitTests.cs @@ -266,28 +266,4 @@ public async Task Oversized_authenticated_and_owner_only_bodies_are_413() return (Str("title"), Str("detail"), Int("status")); } - // A read stream that can't report its own Length (CanSeek = false), so it - // pairs with an explicit/lying Content-Length override (see - // LyingLengthRequest) to simulate an undeclared-length body under TestServer. - private sealed class NonSeekableStream(byte[] data) : Stream - { - private int _pos; - public override int Read(byte[] buffer, int offset, int count) - { - var n = Math.Min(count, data.Length - _pos); - if (n <= 0) return 0; - Array.Copy(data, _pos, buffer, offset, n); - _pos += n; - return n; - } - public override bool CanRead => true; - public override bool CanSeek => false; - public override bool CanWrite => false; - public override long Length => throw new NotSupportedException(); - public override long Position { get => _pos; set => throw new NotSupportedException(); } - public override void Flush() { } - public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); - public override void SetLength(long value) => throw new NotSupportedException(); - public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); - } } diff --git a/tests/Cluckwork.Api.IntegrationTests/Infrastructure/NonSeekableStream.cs b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/NonSeekableStream.cs new file mode 100644 index 000000000..d3158006d --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/NonSeekableStream.cs @@ -0,0 +1,40 @@ +namespace Cluckwork.Api.IntegrationTests.Infrastructure; + +// A read stream that can't report its own Length (CanSeek = false), so it pairs +// with an explicit/lying Content-Length override to simulate an undeclared-length +// body under TestServer. +// +// This is what reaches RequestBodyLimit's LAYER-3 read cap +// (ByteCappedRequestStream), which throws its 413 mid-binding — as opposed to the +// declared-length short-circuit, which refuses before the request ever reaches the +// endpoint. Both produce a 413 to the client, so a test that accidentally takes +// the declared-length path still passes while proving nothing about the streamed +// one. Using this stream is what keeps that distinction real. +// +// Shared (#398 review round 2, Codex): AuthBodyLimitTests pins the streamed 413's +// RESPONSE shape and RequestLoggingTests pins its TELEMETRY. Both need the same +// transport shape, and two copies could drift apart — at which point one of them +// would silently stop exercising the path it names. +internal sealed class NonSeekableStream(byte[] data) : Stream +{ + private int _pos; + + public override int Read(byte[] buffer, int offset, int count) + { + var n = Math.Min(count, data.Length - _pos); + if (n <= 0) return 0; + Array.Copy(data, _pos, buffer, offset, n); + _pos += n; + return n; + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position { get => _pos; set => throw new NotSupportedException(); } + public override void Flush() { } + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); +} diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index fc3d98b95..e0b176cd5 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -271,9 +271,16 @@ public async Task Streamed_413_logs_one_completion_at_information_with_status_41 // Declares 10 bytes, actually sends ~8 KB: the cap is breached mid-read, // inside the generated binder, rather than by the declared-length // short-circuit that never reaches binding at all. + // + // NonSeekableStream, shared with AuthBodyLimitTests, NOT a MemoryStream + // (#398 review round 2, Codex): both paths answer 413, so a test that + // drifted onto the declared-length short-circuit would still pass while + // proving nothing about the streamed one. Matching the transport shape + // AuthBodyLimitTests already uses for this path keeps that honest by + // construction rather than by argument. var payload = System.Text.Encoding.UTF8.GetBytes( $"{{\"email\":\"nobody@example.com\",\"password\":\"{new string('a', 8192)}\"}}"); - var content = new StreamContent(new MemoryStream(payload)); + var content = new StreamContent(new NonSeekableStream(payload)); content.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("application/json"); content.Headers.ContentLength = 10; From 98a955afa7356d62cf8359eaa857b320d51c7b96 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 19:43:37 -0700 Subject: [PATCH 06/11] test(api): pin that an unsupported Content-Type never reaches Error telemetry Codex round 3 predicted that ThrowOnBadRequest=true makes the binder throw a 415 for an unsupported Content-Type, which BindingFailureResponse rethrows (not a 400) and Serilog logs as its hardcoded 500/Error. Measured: no 415 is produced at all. A JSON-bound minimal-API endpoint carries a content-type constraint on the ROUTE, so a form-encoded body fails to match and is refused with 404 before any binder runs. No exception, nothing to rethrow, nothing to mis-classify. Pinned anyway, because the guarantee is what matters and is easy to lose silently: an ordinary client mistake must never be logged at Error. Asserted against whatever status the path returns rather than hardcoding 404 twice, so if this ever does become a real 415 the test keeps proving the telemetry guarantee instead of merely failing on the status. Mutation-checked: escalating GetLevel to treat 404+ as Error turns it RED; restored, green. 18/18 across both classes, build 0 warnings 0 errors. --- .../RequestLoggingTests.cs | 47 +++++++++++ .../TestResults/r.trx | 82 +++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index e0b176cd5..769bc17c6 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -301,6 +301,53 @@ public async Task Streamed_413_logs_one_completion_at_information_with_status_41 Assert.Equal(LogEventLevel.Information, completion.Level); Assert.Null(completion.Exception); } + + // #398 review round 3 (Codex) — the unsupported-Content-Type path, raised as + // "binding-generated 415s inflate error telemetry". The reasoning was that + // ThrowOnBadRequest governs the binder's OWN failures, an unsupported media + // type is one of those, so it would throw, BindingFailureResponse would + // rethrow it (not a 400), and Serilog would log its hardcoded 500/Error. + // + // Measured: no 415 is ever generated. A JSON-bound minimal-API endpoint + // carries a content-type constraint on the ROUTE, so a form-encoded body + // fails to match and is refused with 404 before any binder runs. There is no + // exception, so there is nothing for the middleware to rethrow and nothing + // for Serilog to mis-classify. + // + // Pinned anyway, because the guarantee is the one that matters and is easy + // to lose: an ordinary client mistake must never reach Error level. If a + // future change did start producing a real 415 through the binder, this goes + // red rather than silently inflating 5xx alerts. + [Fact] + public async Task Unsupported_content_type_is_refused_without_error_telemetry() + { + var client = factory.CreateClient(); + + var content = new StringContent( + "email=nobody@example.com&password=whatever", + System.Text.Encoding.UTF8, + "application/x-www-form-urlencoded"); + + var response = await client.SendAsync( + new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/login") { Content = content }); + + // Routing refuses it; the endpoint is never reached. + Assert.Equal(System.Net.HttpStatusCode.NotFound, response.StatusCode); + + // The real assertion: whatever status this path produces, no completion + // for it is logged at Error, and none carries an exception. Written + // against the status rather than pinning 404 twice, so a future change + // to 415 keeps proving the telemetry guarantee instead of just failing. + var completions = CompletionEventsFor("/api/v1/auth/login") + .Where(e => ScalarOf(e, "StatusCode") == ((int)response.StatusCode).ToString()) + .ToList(); + Assert.NotEmpty(completions); + Assert.All(completions, e => + { + Assert.Equal(LogEventLevel.Information, e.Level); + Assert.Null(e.Exception); + }); + } } // Own collection (not "integration"): this class uses its own factory/container diff --git a/tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx b/tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx new file mode 100644 index 000000000..a9176212f --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx @@ -0,0 +1,82 @@ + + + + + + + + + + + Assert.Equal() Failure: Values differ +Expected: UnsupportedMediaType +Actual: NotFound + at Cluckwork.Api.IntegrationTests.RequestLoggingTests.Unsupported_media_type_logs_one_completion_at_information_with_status_415() in /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs:line 329 +--- End of stack trace from previous location --- + + + + + + + + + + + + + + + + + + + + + [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v3.1.5+1b188a7b0a (64-bit .NET 10.0.10) +[xUnit.net 00:00:00.08] Discovering: Cluckwork.Api.IntegrationTests +[xUnit.net 00:00:00.14] Discovered: Cluckwork.Api.IntegrationTests +[xUnit.net 00:00:00.15] Starting: Cluckwork.Api.IntegrationTests +[testcontainers.org 00:00:00.08] Connected to Docker: + Host: unix:///var/run/docker.sock + Server Version: 29.6.1 + Kernel Version: 6.18.39 + API Version: 1.55 + Operating System: NixOS 26.05 (Yarara) + Total Memory: 60.39 GB +[testcontainers.org 00:00:00.21] Docker container dfcdf8470367 created +[testcontainers.org 00:00:00.24] Start Docker container dfcdf8470367 +[testcontainers.org 00:00:00.33] Wait for Docker container dfcdf8470367 to complete readiness checks +[testcontainers.org 00:00:01.36] Docker container dfcdf8470367 ready +[testcontainers.org 00:00:01.48] Docker container bdcaa5b6ecd0 created +[testcontainers.org 00:00:01.48] Start Docker container bdcaa5b6ecd0 +[testcontainers.org 00:00:01.54] Wait for Docker container bdcaa5b6ecd0 to complete readiness checks +[testcontainers.org 00:00:01.55] Execute "pg_isready --host localhost --dbname postgres --username postgres" at Docker container bdcaa5b6ecd0 +[testcontainers.org 00:00:02.60] Execute "pg_isready --host localhost --dbname postgres --username postgres" at Docker container bdcaa5b6ecd0 +[testcontainers.org 00:00:02.64] Docker container bdcaa5b6ecd0 ready +[19:41:37 INF] [:] OTLP export disabled (Otlp:Endpoint not set) +[19:41:38 ERR] [:] Failed executing DbCommand (11ms) [Parameters=[], CommandType='Text', CommandTimeout='30'] +SELECT "MigrationId", "ProductVersion" +FROM "__EFMigrationsHistory" +ORDER BY "MigrationId"; +[19:41:38 WRN] [:] The WebRootPath was not found: /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/src/Cluckwork.Api/wwwroot. Static files may be unavailable. +[19:41:38 WRN] [:] The WebRootPath was not found: /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/src/Cluckwork.Api/wwwroot. Static files may be unavailable. +[19:41:38 WRN] [e3e8289d0fa83dd18020e3f32e4f8a1b:1ab9e9e55dbeb1b2] Failed to determine the https port for redirect. +[19:41:38 INF] [e3e8289d0fa83dd18020e3f32e4f8a1b:1ab9e9e55dbeb1b2] HTTP POST /api/v1/auth/login responded 404 in 27.4477 ms +[xUnit.net 00:00:04.42] Assert.Equal() Failure: Values differ +[xUnit.net 00:00:04.42] Expected: UnsupportedMediaType +[xUnit.net 00:00:04.42] Actual: NotFound +[xUnit.net 00:00:04.42] Stack Trace: +[xUnit.net 00:00:04.42] /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs(329,0): at Cluckwork.Api.IntegrationTests.RequestLoggingTests.Unsupported_media_type_logs_one_completion_at_information_with_status_415() +[xUnit.net 00:00:04.42] --- End of stack trace from previous location --- +[testcontainers.org 00:00:04.21] Delete Docker container bdcaa5b6ecd0 +[xUnit.net 00:00:04.57] Finished: Cluckwork.Api.IntegrationTests + + + + + [xUnit.net 00:00:04.42] Cluckwork.Api.IntegrationTests.RequestLoggingTests.Unsupported_media_type_logs_one_completion_at_information_with_status_415 [FAIL] + + + + \ No newline at end of file From 94f550673b25dfe29fd83e34a9b04f1d5331b171 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 20:06:57 -0700 Subject: [PATCH 07/11] fix(api): don't blame the request body for a query-binding failure (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4, both findings. 1. ThrowOnBadRequest is global, so a failed TYPED QUERY PARAMETER throws the same 400 BadHttpRequestException as a JSON body failure and reaches the same middleware. Every one of them rendered as ["body"] = ["The request body has an invalid or incorrectly formatted value."] — so GET /api/v1/reports/production?from=not-a-date told the caller its body was malformed. The request has no body. Reproduced before fixing: the response really was {"errors":{"body":[...]}}. hasBody is derived from the REQUEST (ContentLength / Transfer-Encoding), not from ex.Message: minimal-API's binding messages are framework internals that differ per binding source and can change between versions, so matching on them would be a silent trap on the next upgrade. A request that does carry a body still reports "body" — a bad query parameter on a POST is genuinely ambiguous from there, and this fixes the case that was certainly wrong rather than guessing at the one that isn't. Applied to both the middleware and the /error backstop so the two cannot disagree. 2. Removed tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx, which I committed by accident via 'git add -A tests' while extracting a failure message. It is generated output nothing consumes, and it embedded the machine name and an absolute home path. TestResults/ and *.trx are now gitignored so it cannot recur. Mutation-checked: forcing hasBody = true turns the new query test RED while the body-binding test stays green, so the two are orthogonal. 33/33 across RequestLoggingTests, AuthBodyLimitTests and CustomerAndOrderTests; build 0 warnings 0 errors. --- .gitignore | 6 ++ .../Hosting/BindingFailureResponse.cs | 12 ++- src/Cluckwork.Api/Program.cs | 7 +- .../Validation/ValidationResponse.cs | 29 +++++-- .../RequestLoggingTests.cs | 36 ++++++++ .../TestResults/r.trx | 82 ------------------- 6 files changed, 83 insertions(+), 89 deletions(-) delete mode 100644 tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx diff --git a/.gitignore b/.gitignore index f26b71518..c49ef32f4 100644 --- a/.gitignore +++ b/.gitignore @@ -33,3 +33,9 @@ node_modules/ *.key *.p12 *.pfx + +# Generated test-run reports. `dotnet test --logger trx` writes these under a +# TestResults/ folder next to the project; nothing in the repo consumes them, +# and they embed the developer's machine name and absolute home path. +TestResults/ +*.trx diff --git a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs index 4bd973a62..e5e7e293b 100644 --- a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs +++ b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs @@ -55,7 +55,17 @@ public static IApplicationBuilder UseCluckworkBindingFailureResponse(this IAppli if (ex.StatusCode != StatusCodes.Status400BadRequest || context.Response.HasStarted) throw; - await ValidationResponse.BindingFailureProblem().ExecuteAsync(context); + // #398 review round 4 (Codex) — a failed TYPED QUERY PARAMETER + // throws this same 400, so blaming the body unconditionally lies + // to the caller of a bodyless GET. Decide from the request, not + // from ex.Message: the binding messages are framework internals + // that differ per binding source and can change between + // versions. A request with no body cannot have had a body + // binding failure, which is the case that was certainly wrong. + var hasBody = context.Request.ContentLength > 0 + || context.Request.Headers.TransferEncoding.Count > 0; + + await ValidationResponse.BindingFailureProblem(hasBody).ExecuteAsync(context); } }); } diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index 5f30cb217..d8ab40be5 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -466,8 +466,13 @@ // for the (rare, non-JSON-bound-endpoint) case that reaches /error // instead of being short-circuited there — don't fold that case into // the 400-only ValidationProblem mapping above. + // hasBody derived from the request, matching the primary path — a + // bodyless GET whose typed query parameter failed to bind must not be + // told its body was malformed (#398 review round 4). BadHttpRequestException { StatusCode: StatusCodes.Status400BadRequest } => - ValidationResponse.BindingFailureProblem(), + ValidationResponse.BindingFailureProblem( + context.Request.ContentLength > 0 + || context.Request.Headers.TransferEncoding.Count > 0), BadHttpRequestException bad => Results.Problem( detail: bad.Message, statusCode: bad.StatusCode, diff --git a/src/Cluckwork.Api/Validation/ValidationResponse.cs b/src/Cluckwork.Api/Validation/ValidationResponse.cs index 0fe0e47da..196da603f 100644 --- a/src/Cluckwork.Api/Validation/ValidationResponse.cs +++ b/src/Cluckwork.Api/Validation/ValidationResponse.cs @@ -46,11 +46,30 @@ public static IResult Problem(IDictionary errors) => // exception; and the `/error` mapping in Program.cs, kept as the // backstop for any binding failure that somehow bypasses that // middleware (e.g. the response had already started). - public static IResult BindingFailureProblem() => - Problem(new Dictionary - { - ["body"] = ["The request body has an invalid or incorrectly formatted value."], - }); + // + // #398 review round 4 (Codex) — ThrowOnBadRequest is global, so a failed + // TYPED QUERY PARAMETER (`?from=not-a-date`, `?limit=abc`) throws the same + // 400 through the same path. Reporting those under `body` told the caller of + // a bodyless GET that its body was malformed. `hasBody` selects the key, and + // callers derive it from the REQUEST rather than from the exception's + // message: minimal-API's binding messages are framework internals that vary + // by binding source and can change between versions, so matching on them + // would be a silent trap on the next upgrade. + // + // A request that does carry a body still reports `body`, even though a bad + // query parameter on a POST would land there too. That case is genuinely + // ambiguous from here, and `body` is the likelier culprit; the falsehood + // this fixes is the one that was certain. + public static IResult BindingFailureProblem(bool hasBody = true) => + Problem(hasBody + ? new Dictionary + { + ["body"] = ["The request body has an invalid or incorrectly formatted value."], + } + : new Dictionary + { + ["query"] = ["A query parameter has an invalid or incorrectly formatted value."], + }); // Exposed for unit testing without executing an IResult. public static (Dictionary Errors, Dictionary? Codes) diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index 769bc17c6..7941b89b6 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -302,6 +302,42 @@ public async Task Streamed_413_logs_one_completion_at_information_with_status_41 Assert.Null(completion.Exception); } + // #398 review round 4 (Codex) — ThrowOnBadRequest is global, so it also makes + // a failed TYPED QUERY PARAMETER throw a 400 BadHttpRequestException. That + // reaches BindingFailureResponse exactly like a JSON-body failure does, and + // the first version of this PR rendered every one of them as + // ["body"] = ["The request body has an invalid or incorrectly formatted + // value."] — telling the caller of a bodyless GET that its body was wrong. + // + // The fix keys off whether the request HAS a body at all, not off the + // exception's message text: minimal-API's binding messages are framework + // internals that differ per binding source and can change between versions, + // so matching on them would be a silent trap on the next upgrade. + [Fact] + public async Task Query_binding_failure_does_not_blame_the_request_body() + { + var email = $"reqlog-query-{Guid.NewGuid():N}@test.local"; + await factory.SeedAccountWithUserAsync(email); + var token = await factory.LoginForAccessTokenAsync(email); + var client = factory.CreateAuthedClient(token); + + var response = await client.GetAsync("/api/v1/reports/production?from=not-a-date"); + + Assert.Equal(System.Net.HttpStatusCode.BadRequest, response.StatusCode); + var payload = await response.Content.ReadAsStringAsync(); + + // A GET carries no body, so blaming one is simply false. + Assert.DoesNotContain("\"body\"", payload); + Assert.Contains("\"query\"", payload); + + // And it must still be an ordinary client error in telemetry, not a 500. + var completion = Assert.Single( + CompletionEventsFor("/api/v1/reports/production"), + e => ScalarOf(e, "StatusCode") == "400"); + Assert.Equal(LogEventLevel.Information, completion.Level); + Assert.Null(completion.Exception); + } + // #398 review round 3 (Codex) — the unsupported-Content-Type path, raised as // "binding-generated 415s inflate error telemetry". The reasoning was that // ThrowOnBadRequest governs the binder's OWN failures, an unsupported media diff --git a/tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx b/tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx deleted file mode 100644 index a9176212f..000000000 --- a/tests/Cluckwork.Api.IntegrationTests/TestResults/r.trx +++ /dev/null @@ -1,82 +0,0 @@ - - - - - - - - - - - Assert.Equal() Failure: Values differ -Expected: UnsupportedMediaType -Actual: NotFound - at Cluckwork.Api.IntegrationTests.RequestLoggingTests.Unsupported_media_type_logs_one_completion_at_information_with_status_415() in /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs:line 329 ---- End of stack trace from previous location --- - - - - - - - - - - - - - - - - - - - - - [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v3.1.5+1b188a7b0a (64-bit .NET 10.0.10) -[xUnit.net 00:00:00.08] Discovering: Cluckwork.Api.IntegrationTests -[xUnit.net 00:00:00.14] Discovered: Cluckwork.Api.IntegrationTests -[xUnit.net 00:00:00.15] Starting: Cluckwork.Api.IntegrationTests -[testcontainers.org 00:00:00.08] Connected to Docker: - Host: unix:///var/run/docker.sock - Server Version: 29.6.1 - Kernel Version: 6.18.39 - API Version: 1.55 - Operating System: NixOS 26.05 (Yarara) - Total Memory: 60.39 GB -[testcontainers.org 00:00:00.21] Docker container dfcdf8470367 created -[testcontainers.org 00:00:00.24] Start Docker container dfcdf8470367 -[testcontainers.org 00:00:00.33] Wait for Docker container dfcdf8470367 to complete readiness checks -[testcontainers.org 00:00:01.36] Docker container dfcdf8470367 ready -[testcontainers.org 00:00:01.48] Docker container bdcaa5b6ecd0 created -[testcontainers.org 00:00:01.48] Start Docker container bdcaa5b6ecd0 -[testcontainers.org 00:00:01.54] Wait for Docker container bdcaa5b6ecd0 to complete readiness checks -[testcontainers.org 00:00:01.55] Execute "pg_isready --host localhost --dbname postgres --username postgres" at Docker container bdcaa5b6ecd0 -[testcontainers.org 00:00:02.60] Execute "pg_isready --host localhost --dbname postgres --username postgres" at Docker container bdcaa5b6ecd0 -[testcontainers.org 00:00:02.64] Docker container bdcaa5b6ecd0 ready -[19:41:37 INF] [:] OTLP export disabled (Otlp:Endpoint not set) -[19:41:38 ERR] [:] Failed executing DbCommand (11ms) [Parameters=[], CommandType='Text', CommandTimeout='30'] -SELECT "MigrationId", "ProductVersion" -FROM "__EFMigrationsHistory" -ORDER BY "MigrationId"; -[19:41:38 WRN] [:] The WebRootPath was not found: /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/src/Cluckwork.Api/wwwroot. Static files may be unavailable. -[19:41:38 WRN] [:] The WebRootPath was not found: /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/src/Cluckwork.Api/wwwroot. Static files may be unavailable. -[19:41:38 WRN] [e3e8289d0fa83dd18020e3f32e4f8a1b:1ab9e9e55dbeb1b2] Failed to determine the https port for redirect. -[19:41:38 INF] [e3e8289d0fa83dd18020e3f32e4f8a1b:1ab9e9e55dbeb1b2] HTTP POST /api/v1/auth/login responded 404 in 27.4477 ms -[xUnit.net 00:00:04.42] Assert.Equal() Failure: Values differ -[xUnit.net 00:00:04.42] Expected: UnsupportedMediaType -[xUnit.net 00:00:04.42] Actual: NotFound -[xUnit.net 00:00:04.42] Stack Trace: -[xUnit.net 00:00:04.42] /home/mforce/dev/cluckwork/.claude/worktrees/agent-a91a3b121035be3db/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs(329,0): at Cluckwork.Api.IntegrationTests.RequestLoggingTests.Unsupported_media_type_logs_one_completion_at_information_with_status_415() -[xUnit.net 00:00:04.42] --- End of stack trace from previous location --- -[testcontainers.org 00:00:04.21] Delete Docker container bdcaa5b6ecd0 -[xUnit.net 00:00:04.57] Finished: Cluckwork.Api.IntegrationTests - - - - - [xUnit.net 00:00:04.42] Cluckwork.Api.IntegrationTests.RequestLoggingTests.Unsupported_media_type_logs_one_completion_at_information_with_status_415 [FAIL] - - - - \ No newline at end of file From d71bf866d68f7cb15032c65fba3d1ad78d77656f Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 20:28:07 -0700 Subject: [PATCH 08/11] fix(api): decide body-vs-query from the endpoint contract, not the byte count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 5, against the round-4 fix. Real, and the round-4 fix caused it. Round 4 asked whether the REQUEST carried payload bytes. That inverts for the case it matters most in: a caller who omits a REQUIRED body sends no bytes, yet that is unambiguously a body failure — and it was being reported under "query". Reproduced before fixing: POST /api/v1/auth/login with Content-Type: application/json and an empty body answered 400 with no "body" key in the payload. The signal is the ENDPOINT's contract, not the request's bytes. IAcceptsMetadata carries a RequestType exactly when a route declares a request body, so for those routes a binding failure is a body failure whether the body was malformed or absent. The byte check survives only as a fallback for the unexpected case of missing endpoint metadata. Still deliberately not ex.Message. Note a contentless POST (no Content-Type at all) never reaches binding — the JSON-bound route constrains content type, so it 404s, same as the unsupported-type case. Declaring the type is what carries the request as far as the binder; the test says so, since the difference is not obvious. Both call sites now go through one shared ConcernsRequestBody helper, so the middleware and the /error backstop cannot disagree — the same reason the response shape is a single factory. Mutation-checked: reverting to the round-4 byte-only check turns the new missing-body test RED while the query test stays green, so the two are independent and neither covers for the other. 34/34 across three classes; build 0 warnings 0 errors. --- .../Hosting/BindingFailureResponse.cs | 39 ++++++++++++++----- src/Cluckwork.Api/Program.cs | 11 +++--- .../RequestLoggingTests.cs | 30 ++++++++++++++ 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs index e5e7e293b..c288f4b36 100644 --- a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs +++ b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs @@ -1,6 +1,7 @@ namespace Cluckwork.Api.Hosting; using Cluckwork.Api.Validation; +using Microsoft.AspNetCore.Http.Metadata; // #398 review (Codex) — RouteHandlerOptions.ThrowOnBadRequest is forced true // in every environment (see Program.cs), so a JSON-binding failure (a @@ -57,15 +58,35 @@ public static IApplicationBuilder UseCluckworkBindingFailureResponse(this IAppli // #398 review round 4 (Codex) — a failed TYPED QUERY PARAMETER // throws this same 400, so blaming the body unconditionally lies - // to the caller of a bodyless GET. Decide from the request, not - // from ex.Message: the binding messages are framework internals - // that differ per binding source and can change between - // versions. A request with no body cannot have had a body - // binding failure, which is the case that was certainly wrong. - var hasBody = context.Request.ContentLength > 0 - || context.Request.Headers.TransferEncoding.Count > 0; - - await ValidationResponse.BindingFailureProblem(hasBody).ExecuteAsync(context); + // to the caller of a bodyless GET. + // + // Round 5 (Codex) corrected HOW to tell them apart. The first + // attempt asked whether the REQUEST carried payload bytes, which + // inverts for the case that matters most: a caller who omits a + // REQUIRED body sends no bytes, yet that is a body failure, and + // it was being reported as a query one. + // + // So ask the ENDPOINT what it accepts, not the request what it + // sent. IAcceptsMetadata is present with a RequestType exactly + // when a route declares a request body — for those, a binding + // failure is a body failure whether the body was malformed or + // absent. The byte check stays only as a fallback for the + // (unexpected) case of no endpoint metadata. + // + // Still deliberately NOT ex.Message: the binding messages are + // framework internals that differ per binding source and can + // change between versions. + await ValidationResponse + .BindingFailureProblem(ConcernsRequestBody(context)) + .ExecuteAsync(context); } }); + + // Shared by the middleware above and Program.cs's `/error` backstop, so the + // two can never disagree about which key a binding failure is reported + // under — the same reason the response shape itself is a single factory. + public static bool ConcernsRequestBody(HttpContext context) => + context.GetEndpoint()?.Metadata.GetMetadata()?.RequestType is not null + || context.Request.ContentLength > 0 + || context.Request.Headers.TransferEncoding.Count > 0; } diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index d8ab40be5..2208e173e 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -466,13 +466,14 @@ // for the (rare, non-JSON-bound-endpoint) case that reaches /error // instead of being short-circuited there — don't fold that case into // the 400-only ValidationProblem mapping above. - // hasBody derived from the request, matching the primary path — a - // bodyless GET whose typed query parameter failed to bind must not be - // told its body was malformed (#398 review round 4). + // Same body-vs-query decision as the primary path, via the one shared + // helper: a bodyless GET whose typed query parameter failed to bind must + // not be told its body was malformed, and a caller who omitted a + // REQUIRED body must not be told the problem was a query parameter + // (#398 review rounds 4 and 5). BadHttpRequestException { StatusCode: StatusCodes.Status400BadRequest } => ValidationResponse.BindingFailureProblem( - context.Request.ContentLength > 0 - || context.Request.Headers.TransferEncoding.Count > 0), + BindingFailureResponse.ConcernsRequestBody(context)), BadHttpRequestException bad => Results.Problem( detail: bad.Message, statusCode: bad.StatusCode, diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index 7941b89b6..5d58deb1a 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -338,6 +338,36 @@ public async Task Query_binding_failure_does_not_blame_the_request_body() Assert.Null(completion.Exception); } + // #398 review round 5 (Codex) — the inverse of the round-4 case, and a + // defect the round-4 fix introduced. A caller that omits a REQUIRED JSON + // body entirely also fails binding, and carries no payload bytes — so a + // "does the request have bytes?" test reports it as a query failure, which + // is exactly as wrong as blaming the body was. + // + // The signal has to be the ENDPOINT's contract (does it accept a body?), + // not the request's byte count. + [Fact] + public async Task Missing_required_body_is_reported_as_a_body_failure_not_a_query_one() + { + var client = factory.CreateClient(); + + // Declares JSON but sends nothing. A POST with NO Content-Type at all + // does not reach binding — the JSON-bound endpoint constrains content + // type on the route, so it 404s exactly like the unsupported-type case + // below. Declaring the type is what gets the request as far as the + // binder, where the required body is then found to be absent. + var content = new StringContent( + string.Empty, System.Text.Encoding.UTF8, "application/json"); + var response = await client.SendAsync( + new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/login") { Content = content }); + + Assert.Equal(System.Net.HttpStatusCode.BadRequest, response.StatusCode); + var payload = await response.Content.ReadAsStringAsync(); + + Assert.Contains("\"body\"", payload); + Assert.DoesNotContain("\"query\"", payload); + } + // #398 review round 3 (Codex) — the unsupported-Content-Type path, raised as // "binding-generated 415s inflate error telemetry". The reasoning was that // ThrowOnBadRequest governs the binder's OWN failures, an unsupported media From a7d0462fb3aca836d2b756ebd3bd176f0a7508d4 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 20:38:51 -0700 Subject: [PATCH 09/11] fix(api): branch the byte fallback, and correlate log assertions by TraceId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 6, both real. 1. ConcernsRequestBody OR-ed the byte checks with the endpoint check, so incidental payload bytes on a query-only route reported "body": a GET to /api/v1/reports/production?from=not-a-date carrying a non-empty body has ContentLength > 0 and returned true, despite that endpoint accepting no body at all. The comment already said the byte checks were a fallback for "no endpoint metadata" — the code applied them unconditionally. The code now does what the comment claimed: when an endpoint IS matched its contract is authoritative, and the byte checks run only when routing resolved nothing. 2. Binding_failure_logs_... asserted Assert.Single over every 400 completion for /api/v1/auth/login, from a class-scoped sink that is never reset. The round-5 test added another 400 on that same path, so the assertion's correctness depended on xUnit execution order, which is not guaranteed — correct application behaviour could fail the suite purely on ordering. Requests now carry a stamped traceparent and completions are matched by TraceId, which removes the coupling rather than compensating for it with a count. Applied to the 413 and query tests too, not just the one reported. Mutation checks: - Re-OR-ing the byte checks turns the query test RED (stray-body case). - Order-dependence cannot be killed by a single run — that is the defect. Made it deterministic instead: with a decoy same-path 400 in the test, TraceId matching PASSES and status matching FAILS. Both directions verified. 34/34 across RequestLoggingTests, AuthBodyLimitTests and CustomerAndOrderTests; build 0 warnings 0 errors. --- .../Hosting/BindingFailureResponse.cs | 27 ++++++-- .../RequestLoggingTests.cs | 68 +++++++++++++++---- 2 files changed, 78 insertions(+), 17 deletions(-) diff --git a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs index c288f4b36..e37840c33 100644 --- a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs +++ b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs @@ -85,8 +85,27 @@ await ValidationResponse // Shared by the middleware above and Program.cs's `/error` backstop, so the // two can never disagree about which key a binding failure is reported // under — the same reason the response shape itself is a single factory. - public static bool ConcernsRequestBody(HttpContext context) => - context.GetEndpoint()?.Metadata.GetMetadata()?.RequestType is not null - || context.Request.ContentLength > 0 - || context.Request.Headers.TransferEncoding.Count > 0; + // + // #398 review round 6 (Codex) — the byte checks are a fallback for "no + // endpoint was matched", and must therefore be BRANCHED, not OR-ed. Or-ing + // them let incidental payload bytes on a query-only route report `body`: + // a GET to /api/v1/reports/production?from=not-a-date carrying a non-empty + // body has ContentLength > 0, so the expression returned true even though + // that endpoint accepts no body at all. The comment claimed "fallback" + // while the code applied the checks unconditionally — the code is now what + // the comment says. + // + // When an endpoint IS matched, its own contract is authoritative and + // nothing about the request can override it. + public static bool ConcernsRequestBody(HttpContext context) + { + var endpoint = context.GetEndpoint(); + if (endpoint is not null) + return endpoint.Metadata.GetMetadata()?.RequestType is not null; + + // No endpoint (404, or a failure before routing resolved one): fall + // back to whether the request carried a payload at all. + return context.Request.ContentLength > 0 + || context.Request.Headers.TransferEncoding.Count > 0; + } } diff --git a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs index 5d58deb1a..ef1bcc025 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RequestLoggingTests.cs @@ -76,6 +76,25 @@ [.. factory.Sink.Events.Where(e => ? scalar.Value?.ToString() : null; + // #398 review round 6 (Codex) — correlate a completion to THIS request, + // not merely to its path and status. + // + // The sink is class-scoped and never reset, and several tests here POST to + // /api/v1/auth/login and produce a 400. A path+status filter therefore + // depends on execution order, which xUnit does not guarantee — so a + // correct application could fail the suite purely because two tests ran in + // the other order. Stamping a fresh traceparent and matching on TraceId + // removes the coupling entirely rather than papering over it with a count. + private static ActivityTraceId StampTrace(HttpRequestMessage request) + { + var traceId = ActivityTraceId.CreateRandom(); + request.Headers.Add("traceparent", $"00-{traceId}-{ActivitySpanId.CreateRandom()}-01"); + return traceId; + } + + private LogEvent CompletionFor(string path, ActivityTraceId traceId) => + Assert.Single(CompletionEventsFor(path), e => e.TraceId == traceId); + [Fact] public async Task Request_emits_one_completion_log_with_method_path_status_elapsed_and_traceid() { @@ -200,17 +219,20 @@ public async Task Binding_failure_logs_one_completion_at_information_with_status { var client = factory.CreateClient(); var malformed = new StringContent("{not json", System.Text.Encoding.UTF8, "application/json"); + var request = new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/login") { Content = malformed }; + var traceId = StampTrace(request); - var response = await client.PostAsync("/api/v1/auth/login", malformed); + var response = await client.SendAsync(request); Assert.Equal(System.Net.HttpStatusCode.BadRequest, response.StatusCode); // (1) Exactly one completion for THIS request, at Information, // StatusCode 400 — not 500, not Error — and no exception-handler // re-execution at /error (the failure never propagated that far up - // the pipeline). - var completion = Assert.Single(CompletionEventsFor("/api/v1/auth/login"), - e => ScalarOf(e, "StatusCode") == "400"); + // the pipeline). Matched by TraceId, not path+status: another test in + // this class also produces a 400 on this path (#398 review round 6). + var completion = CompletionFor("/api/v1/auth/login", traceId); + Assert.Equal("400", ScalarOf(completion, "StatusCode")); Assert.Equal(LogEventLevel.Information, completion.Level); Assert.Empty(CompletionEventsFor("/error")); @@ -284,8 +306,10 @@ public async Task Streamed_413_logs_one_completion_at_information_with_status_41 content.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("application/json"); content.Headers.ContentLength = 10; - var response = await client.SendAsync( - new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/login") { Content = content }); + var request = new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/login") { Content = content }; + var traceId = StampTrace(request); + + var response = await client.SendAsync(request); Assert.Equal(System.Net.HttpStatusCode.RequestEntityTooLarge, response.StatusCode); @@ -295,9 +319,8 @@ public async Task Streamed_413_logs_one_completion_at_information_with_status_41 // on their traffic rather than on this guarantee. Exactly one 413 // completion is still the assertion — a re-executed /error would add a // second, and a mis-logged 500 would leave zero. - var completion = Assert.Single( - CompletionEventsFor("/api/v1/auth/login"), - e => ScalarOf(e, "StatusCode") == "413"); + var completion = CompletionFor("/api/v1/auth/login", traceId); + Assert.Equal("413", ScalarOf(completion, "StatusCode")); Assert.Equal(LogEventLevel.Information, completion.Level); Assert.Null(completion.Exception); } @@ -321,7 +344,11 @@ public async Task Query_binding_failure_does_not_blame_the_request_body() var token = await factory.LoginForAccessTokenAsync(email); var client = factory.CreateAuthedClient(token); - var response = await client.GetAsync("/api/v1/reports/production?from=not-a-date"); + var request = new HttpRequestMessage( + HttpMethod.Get, "/api/v1/reports/production?from=not-a-date"); + var traceId = StampTrace(request); + + var response = await client.SendAsync(request); Assert.Equal(System.Net.HttpStatusCode.BadRequest, response.StatusCode); var payload = await response.Content.ReadAsStringAsync(); @@ -330,10 +357,25 @@ public async Task Query_binding_failure_does_not_blame_the_request_body() Assert.DoesNotContain("\"body\"", payload); Assert.Contains("\"query\"", payload); + // #398 review round 6 (Codex) — incidental payload bytes on a + // query-only route must not flip the verdict. This endpoint accepts no + // body, so its own contract decides; before the fix the byte checks + // were OR-ed in and a non-empty body here reported `body`. + var withBody = new HttpRequestMessage( + HttpMethod.Get, "/api/v1/reports/production?from=not-a-date") + { + Content = new StringContent( + "{}", System.Text.Encoding.UTF8, "application/json"), + }; + var strayBody = await client.SendAsync(withBody); + Assert.Equal(System.Net.HttpStatusCode.BadRequest, strayBody.StatusCode); + var strayPayload = await strayBody.Content.ReadAsStringAsync(); + Assert.DoesNotContain("\"body\"", strayPayload); + Assert.Contains("\"query\"", strayPayload); + // And it must still be an ordinary client error in telemetry, not a 500. - var completion = Assert.Single( - CompletionEventsFor("/api/v1/reports/production"), - e => ScalarOf(e, "StatusCode") == "400"); + var completion = CompletionFor("/api/v1/reports/production", traceId); + Assert.Equal("400", ScalarOf(completion, "StatusCode")); Assert.Equal(LogEventLevel.Information, completion.Level); Assert.Null(completion.Exception); } From 4064232a6fbdbcc11ad05a50cd479c28b5195852 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 20:46:28 -0700 Subject: [PATCH 10/11] fix(api): treat manually-read request bodies as body failures (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 7. A matched endpoint without IAcceptsMetadata is not necessarily query-only: PUT /api/v1/accounts/logo reads the raw image body via http.Request.Body.ReadAsync, and POST /api/v1/client-errors binds HttpRequest and reads it itself. Neither declares a typed body parameter, so round 6's branch returned false and a 400 body failure on either would have been reported as errors.query — on endpoints that take no query input at all. Marked both with a new ReadsRequestBodyAttribute that ConcernsRequestBody recognises alongside IAcceptsMetadata. Deliberately a marker rather than .Accepts(contentType), which Codex offered as the first option: Accepts also imposes a content-type constraint on the ROUTE, so adding it would change which requests match and could start 404/415-ing callers that work today. This must affect only how a failure is described, never what is accepted. Swept for other raw-body endpoints rather than fixing only the two reported. AuthEndpoints' Refresh and Logout also bind HttpRequest, but read the refresh COOKIE, not a body, so they correctly stay unmarked. The four rounds of rules now have pure-function tests (ConcernsRequestBodyTests) rather than only being exercised incidentally through endpoints — one per round, including the two cases earlier versions got backwards. Mutation-checked: dropping the marker clause turns Manually_read_body_endpoint_is_a_body_failure RED while the other four stay green. 94/94 across the affected classes; build 0 warnings 0 errors. --- .../Endpoints/Accounts/FarmLogoEndpoints.cs | 5 ++ .../ClientErrors/ClientErrorEndpoints.cs | 4 ++ .../Hosting/BindingFailureResponse.cs | 20 ++++++- .../ValidationResponseTests.cs | 58 +++++++++++++++++++ 4 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/Cluckwork.Api/Endpoints/Accounts/FarmLogoEndpoints.cs b/src/Cluckwork.Api/Endpoints/Accounts/FarmLogoEndpoints.cs index e1a31094f..1bea985c6 100644 --- a/src/Cluckwork.Api/Endpoints/Accounts/FarmLogoEndpoints.cs +++ b/src/Cluckwork.Api/Endpoints/Accounts/FarmLogoEndpoints.cs @@ -11,6 +11,7 @@ namespace Cluckwork.Api.Endpoints.Accounts; using Microsoft.AspNetCore.Http.Features; using Microsoft.Extensions.Options; using Microsoft.Net.Http.Headers; +using Cluckwork.Api.Hosting; // #123 — the farm logo: upload, serve, remove. // @@ -32,6 +33,10 @@ public static RouteGroupBuilder MapFarmLogoEndpoints(this RouteGroupBuilder grou .WithSummary("The farm logo image. 404 when none is set; the chrome falls back to app branding."); group.MapPut("/logo", SetLogo) + // #398 — reads the raw body directly, so it declares no typed body + // parameter and carries no IAcceptsMetadata. Without this marker a + // 400 body failure here would be reported as a query error. + .WithMetadata(new ReadsRequestBodyAttribute()) .RequireAuthorization(AuthPolicies.AdminOnly) .WithName("SetFarmLogo") .WithSummary( diff --git a/src/Cluckwork.Api/Endpoints/ClientErrors/ClientErrorEndpoints.cs b/src/Cluckwork.Api/Endpoints/ClientErrors/ClientErrorEndpoints.cs index 80e98e245..b4a4e4e51 100644 --- a/src/Cluckwork.Api/Endpoints/ClientErrors/ClientErrorEndpoints.cs +++ b/src/Cluckwork.Api/Endpoints/ClientErrors/ClientErrorEndpoints.cs @@ -3,6 +3,7 @@ namespace Cluckwork.Api.Endpoints.ClientErrors; using System.Text.Json; using Cluckwork.Api.RateLimiting; using Microsoft.AspNetCore.Http.Features; +using Cluckwork.Api.Hosting; // #217 — the SPA's ErrorBoundary reports render crashes here so the operator // learns a screen is crashing without a support screenshot. The report is @@ -30,6 +31,9 @@ public static class ClientErrorEndpoints public static RouteGroupBuilder MapClientErrorEndpoints(this RouteGroupBuilder group) { group.MapPost("/", Report) + // #398 — binds HttpRequest and reads the body itself, so it carries + // no IAcceptsMetadata; see ReadsRequestBodyAttribute. + .WithMetadata(new ReadsRequestBodyAttribute()) .AllowAnonymous() .RequireRateLimiting(RateLimitingOptions.ClientErrorsPolicyName) .WithName("ReportClientError") diff --git a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs index e37840c33..b38986037 100644 --- a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs +++ b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs @@ -29,6 +29,20 @@ namespace Cluckwork.Api.Hosting; // Information — exactly like any other well-formed 400. No re-execution at // /error happens either, since UseExceptionHandler (which sits ABOVE Serilog) // never sees anything propagate that far. + +// #398 review round 7 (Codex) — marks an endpoint that reads the request body +// MANUALLY (raw stream, or a bound `HttpRequest`) instead of declaring a typed +// body parameter. Those carry no IAcceptsMetadata, so without this marker +// ConcernsRequestBody would classify a body failure on them as a query one — +// and they have no query input at all. +// +// Deliberately a private marker rather than `.Accepts(contentType)`: +// Accepts also imposes a CONTENT-TYPE CONSTRAINT ON THE ROUTE, so adding it +// would change which requests match and could start 404/415-ing callers that +// work today. This affects only how a failure is described. +[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class)] +public sealed class ReadsRequestBodyAttribute : Attribute; + public static class BindingFailureResponse { public static IApplicationBuilder UseCluckworkBindingFailureResponse(this IApplicationBuilder app) => @@ -101,7 +115,11 @@ public static bool ConcernsRequestBody(HttpContext context) { var endpoint = context.GetEndpoint(); if (endpoint is not null) - return endpoint.Metadata.GetMetadata()?.RequestType is not null; + // IAcceptsMetadata covers endpoints with a declared typed body. + // ReadsRequestBodyAttribute covers the ones that read it manually + // and therefore declare nothing — see the attribute's own comment. + return endpoint.Metadata.GetMetadata()?.RequestType is not null + || endpoint.Metadata.GetMetadata() is not null; // No endpoint (404, or a failure before routing resolved one): fall // back to whether the request carried a payload at all. diff --git a/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs b/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs index 586da3b54..95893248b 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs @@ -1,7 +1,10 @@ namespace Cluckwork.Api.IntegrationTests; +using Cluckwork.Api.Hosting; using Cluckwork.Api.Validation; using FluentValidation.Results; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Metadata; // Pure-function tests: no server, no Testcontainers collection. public sealed class ValidationResponseTests @@ -59,3 +62,58 @@ public void Mixed_field_keeps_index_alignment_with_null_for_uncoded() $"Expected [Me.Language.Format, null], got [{string.Join(", ", codes!["Language"].Select(c => c ?? "null"))}]"); } } + +// #398 review rounds 4-7 — BindingFailureResponse.ConcernsRequestBody decides +// whether a 400 binding failure is reported under `body` or `query`. It took +// four review rounds to get right, each earlier version wrong in a different +// direction, so the rules are pinned here as pure functions rather than only +// through the endpoints that happen to exercise them. +public sealed class ConcernsRequestBodyTests +{ + private static Endpoint EndpointWith(params object[] metadata) => + new(_ => Task.CompletedTask, new EndpointMetadataCollection(metadata), "test"); + + private static HttpContext Context(Endpoint? endpoint, long? contentLength = null) + { + var context = new DefaultHttpContext(); + if (endpoint is not null) context.SetEndpoint(endpoint); + context.Request.ContentLength = contentLength; + return context; + } + + // Round 4: a bodyless GET whose typed query parameter failed to bind must + // not be told its body was malformed. + [Fact] + public void Query_only_endpoint_is_not_a_body_failure() => + Assert.False(BindingFailureResponse.ConcernsRequestBody(Context(EndpointWith()))); + + // Round 5: a caller who omits a REQUIRED body sends no bytes, yet that is a + // body failure — so the ENDPOINT's contract decides, not the byte count. + [Fact] + public void Endpoint_declaring_a_body_is_a_body_failure_even_with_no_bytes() => + Assert.True(BindingFailureResponse.ConcernsRequestBody( + Context(EndpointWith(new AcceptsMetadata(["application/json"], typeof(object)))))); + + // Round 6: incidental payload bytes must NOT override a matched endpoint + // that accepts no body. This is the case an OR-ed expression got wrong. + [Fact] + public void Stray_bytes_do_not_override_a_query_only_endpoint() => + Assert.False(BindingFailureResponse.ConcernsRequestBody( + Context(EndpointWith(), contentLength: 42))); + + // Round 7: an endpoint that reads the body MANUALLY declares no + // IAcceptsMetadata, but is still a body endpoint (raw logo upload, + // client-error report). + [Fact] + public void Manually_read_body_endpoint_is_a_body_failure() => + Assert.True(BindingFailureResponse.ConcernsRequestBody( + Context(EndpointWith(new ReadsRequestBodyAttribute())))); + + // Only with NO matched endpoint does the byte fallback apply. + [Fact] + public void Without_an_endpoint_the_byte_fallback_decides() + { + Assert.True(BindingFailureResponse.ConcernsRequestBody(Context(null, contentLength: 1))); + Assert.False(BindingFailureResponse.ConcernsRequestBody(Context(null))); + } +} From 0e2d39e20fbee6ee5d28d07a25838b3d5749acfe Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 21:19:48 -0700 Subject: [PATCH 11/11] fix(api): mark /auth/refresh as a body reader, and enforce the marker from the endpoint table #398 review round 8 (Codex). /auth/refresh drains Request.Body to enforce its #309 1 KB cap, so Kestrel can raise a 400 there for a truncated or malformed body. It declares no typed body parameter, so it carried no IAcceptsMetadata, and ConcernsRequestBody reported that failure as `errors.query` on an endpoint with no query input at all. That is the second missing marker in consecutive review rounds (round 7 was PUT /account/logo and POST /client-errors). Both were found by a reviewer because the marker is applied by hand and forgetting it is the silent default, so this adds BodyReadingEndpointTests: it walks the running application's own EndpointDataSource and requires every handler that can reach the raw body (HttpContext/HttpRequest/Stream/PipeReader) to either declare a typed body, carry ReadsRequestBodyAttribute, or appear in a reviewed exemption list with a reason. A second test fails a stale exemption, so a renamed route cannot leave one behind to excuse a later endpoint that takes the same key. That guard found a third defect on its first run, in the /error backstop rather than an endpoint. UseExceptionHandler re-executes the pipeline, which clears the endpoint and routes again, so context.GetEndpoint() inside /error is the /error endpoint itself. Since round 6 made a matched endpoint authoritative, the backstop answered `query` for every failure it handled, including a malformed JSON body. IExceptionHandlerFeature.Endpoint is the framework's own record of what was matched before the throw, and now takes precedence; outside re-execution the feature is absent and nothing changes. Mutation-checked, baseline green (122/122 across the affected classes): dropping the refresh marker turns the guard red naming that endpoint; reverting to context.GetEndpoint() turns During_error_reexecution_the_original_endpoint_wins red while the other four classification cases stay green; a fabricated exemption turns the stale-exemption test red. --- .../Endpoints/Auth/AuthEndpoints.cs | 7 + .../Hosting/BindingFailureResponse.cs | 16 ++- .../BodyReadingEndpointTests.cs | 124 ++++++++++++++++++ .../ValidationResponseTests.cs | 18 +++ 4 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 tests/Cluckwork.Api.IntegrationTests/BodyReadingEndpointTests.cs diff --git a/src/Cluckwork.Api/Endpoints/Auth/AuthEndpoints.cs b/src/Cluckwork.Api/Endpoints/Auth/AuthEndpoints.cs index c293e2383..425536c6d 100644 --- a/src/Cluckwork.Api/Endpoints/Auth/AuthEndpoints.cs +++ b/src/Cluckwork.Api/Endpoints/Auth/AuthEndpoints.cs @@ -54,6 +54,13 @@ public static RouteGroupBuilder MapAuthEndpoints(this RouteGroupBuilder group) // #309 — refresh carries no body of its own (the token rides in the // cookie); 1 KB is a defensive cap so a junk body can't be streamed in. .WithMaxRequestBodyBytes(1024) + // #398 review round 8 — the handler DRAINS Request.Body to enforce + // that cap (see Refresh), so Kestrel can raise a 400 here for a + // truncated/malformed body. It declares no typed body parameter, so + // without this marker that failure would be reported as `errors.query` + // on an endpoint with no query input at all. BodyReadingEndpointTests + // is what keeps this from being forgotten again. + .WithMetadata(new ReadsRequestBodyAttribute()) .WithName("RefreshToken") .WithSummary("Rotate the refresh-token cookie and return a fresh access token."); diff --git a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs index b38986037..a6ce8e063 100644 --- a/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs +++ b/src/Cluckwork.Api/Hosting/BindingFailureResponse.cs @@ -1,6 +1,7 @@ namespace Cluckwork.Api.Hosting; using Cluckwork.Api.Validation; +using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Http.Metadata; // #398 review (Codex) — RouteHandlerOptions.ThrowOnBadRequest is forced true @@ -113,7 +114,20 @@ await ValidationResponse // nothing about the request can override it. public static bool ConcernsRequestBody(HttpContext context) { - var endpoint = context.GetEndpoint(); + // #398 review round 8 (found by BodyReadingEndpointTests, not by report) + // — the `/error` backstop calls this from INSIDE exception-handler + // re-execution, and that re-execution clears the endpoint and routes + // again, so context.GetEndpoint() there is the `/error` endpoint itself. + // `/error` declares no body and carries no marker, so once round 6 made a + // matched endpoint authoritative, the backstop answered `query` for + // EVERY failure it handled, including a malformed JSON body. + // + // IExceptionHandlerFeature.Endpoint is the framework's own record of + // what was matched before the throw — the original contract, which is + // the one this question is about. Outside re-execution the feature is + // absent and the current endpoint is already the right one. + var endpoint = context.Features.Get()?.Endpoint + ?? context.GetEndpoint(); if (endpoint is not null) // IAcceptsMetadata covers endpoints with a declared typed body. // ReadsRequestBodyAttribute covers the ones that read it manually diff --git a/tests/Cluckwork.Api.IntegrationTests/BodyReadingEndpointTests.cs b/tests/Cluckwork.Api.IntegrationTests/BodyReadingEndpointTests.cs new file mode 100644 index 000000000..fd2b0d3a8 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/BodyReadingEndpointTests.cs @@ -0,0 +1,124 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.IO.Pipelines; +using System.Reflection; +using Cluckwork.Api.Hosting; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Metadata; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; + +// #398 review rounds 7-8 — BindingFailureResponse.ConcernsRequestBody decides +// whether a 400 binding failure is reported under `body` or `query`, and it +// reads the ENDPOINT's contract to do it. An endpoint that declares a typed body +// parameter carries IAcceptsMetadata and is classified correctly for free. An +// endpoint that reads the raw body ITSELF declares nothing, so it must be marked +// with ReadsRequestBodyAttribute — otherwise a body failure on it is reported as +// `errors.query`, on endpoints that have no query input at all. +// +// That marker is applied by hand, and being forgotten is the DEFAULT failure: +// review caught two separate missing markers in consecutive rounds +// (PUT /accounts/logo + POST /client-errors, then POST /auth/refresh, which +// drains Request.Body to enforce its #309 cap). A third would have been found +// the same way — by a reviewer, or not at all. +// +// So the invariant is enforced from the running application's own endpoint +// table rather than trusted to a sweep: any handler that CAN reach the raw body +// must either declare a typed body, carry the marker, or be named below with a +// reason. A new endpoint taking HttpContext/HttpRequest/Stream/PipeReader fails +// this test until someone decides which it is. +[Collection(IntegrationCollection.Name)] +public sealed class BodyReadingEndpointTests(CluckworkWebApplicationFactory factory) +{ + // Parameter types through which a handler can reach the request body. A + // handler taking none of these cannot read it, so its binding failures are + // genuinely about the declared parameters (query, route, headers). + private static readonly Type[] BodyCapableParameters = + [typeof(HttpContext), typeof(HttpRequest), typeof(Stream), typeof(PipeReader)]; + + // Endpoints that take one of the types above but provably do NOT read the + // body. Each needs a reason: the point is that leaving the marker off is a + // decision someone made, not one nobody noticed. + private static readonly Dictionary ReviewedAsNotReadingTheBody = new() + { + ["GET /api/v1/account/logo"] = + "GetLogo takes HttpContext to set Cache-Control/ETag on the RESPONSE and to " + + "read If-None-Match; a GET carries no body to fail on.", + ["POST /api/v1/auth/logout"] = + "Logout takes HttpRequest for the refresh COOKIE and the CSRF header only — " + + "it never touches Request.Body (unlike /auth/refresh, which drains it).", + [" /error"] = + "The exception-handler re-execution target. It takes HttpContext to read " + + "IExceptionHandlerFeature and shape the response, never to read the body — and it " + + "must stay unmarked: ConcernsRequestBody deliberately reads the feature's ORIGINAL " + + "endpoint here, so marking `/error` would classify every failure it handles as a " + + "body failure regardless of what was actually requested.", + }; + + private sealed record Candidate(string Key, MethodInfo Handler, EndpointMetadataCollection Metadata); + + private List BodyCapableEndpoints() + { + var endpoints = factory.Services.GetRequiredService().Endpoints; + var candidates = new List(); + + foreach (var endpoint in endpoints.OfType()) + { + // Minimal-API route handlers carry their MethodInfo in metadata. + // Anything without one is not a handler this repo authored (health + // checks, the SPA fallback), and has no parameters to inspect. + var handler = endpoint.Metadata.GetMetadata(); + if (handler is null) continue; + + if (!handler.GetParameters().Any(p => BodyCapableParameters.Contains(p.ParameterType))) + continue; + + var methods = endpoint.Metadata.GetMetadata()?.HttpMethods ?? []; + candidates.Add(new Candidate( + $"{string.Join(",", methods)} {endpoint.RoutePattern.RawText}", + handler, + endpoint.Metadata)); + } + + return candidates; + } + + [Fact] + public void Every_endpoint_that_can_read_the_body_is_classified() + { + var candidates = BodyCapableEndpoints(); + + // Guard the guard: if metadata ever stops carrying MethodInfo, or the + // endpoint table is empty because the host changed shape, this test + // would pass by inspecting nothing at all. + Assert.NotEmpty(candidates); + + var unclassified = candidates + .Where(c => c.Metadata.GetMetadata()?.RequestType is null + && c.Metadata.GetMetadata() is null + && !ReviewedAsNotReadingTheBody.ContainsKey(c.Key)) + .Select(c => $"{c.Key} → {c.Handler.DeclaringType?.Name}.{c.Handler.Name}") + .ToList(); + + Assert.True(unclassified.Count == 0, + "These endpoints can reach the raw request body but say nothing about it, so a 400 " + + "body failure on them would be reported as `errors.query`. Add " + + "`.WithMetadata(new ReadsRequestBodyAttribute())` if the handler reads the body, or " + + "add it to ReviewedAsNotReadingTheBody with a reason if it does not:\n " + + string.Join("\n ", unclassified)); + } + + [Fact] + public void The_reviewed_exemptions_all_still_exist() + { + // A renamed or deleted route would leave its exemption behind, silently + // ready to excuse a DIFFERENT endpoint that later takes the same key. + var keys = BodyCapableEndpoints().Select(c => c.Key).ToHashSet(); + var stale = ReviewedAsNotReadingTheBody.Keys.Where(k => !keys.Contains(k)).ToList(); + + Assert.True(stale.Count == 0, + "These exemptions no longer match a body-capable endpoint — the route was renamed or " + + "removed. Delete them:\n " + string.Join("\n ", stale)); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs b/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs index 95893248b..74395408e 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ValidationResponseTests.cs @@ -3,6 +3,7 @@ namespace Cluckwork.Api.IntegrationTests; using Cluckwork.Api.Hosting; using Cluckwork.Api.Validation; using FluentValidation.Results; +using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Metadata; @@ -109,6 +110,23 @@ public void Manually_read_body_endpoint_is_a_body_failure() => Assert.True(BindingFailureResponse.ConcernsRequestBody( Context(EndpointWith(new ReadsRequestBodyAttribute())))); + // Round 8: the `/error` backstop runs INSIDE exception-handler re-execution, + // which routes again — so the current endpoint there is `/error`, which + // declares no body. The original endpoint (the one the failure is actually + // about) survives on IExceptionHandlerFeature and must win. + [Fact] + public void During_error_reexecution_the_original_endpoint_wins() + { + var context = Context(EndpointWith()); // as re-execution leaves it: `/error` + context.Features.Set(new ExceptionHandlerFeature + { + Error = new BadHttpRequestException("bad", 400), + Endpoint = EndpointWith(new AcceptsMetadata(["application/json"], typeof(object))), + }); + + Assert.True(BindingFailureResponse.ConcernsRequestBody(context)); + } + // Only with NO matched endpoint does the byte fallback apply. [Fact] public void Without_an_endpoint_the_byte_fallback_decides()