fix(sales): reject fractional order-line quantities instead of leaking a JSON-binding error

.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

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

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(

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.");
 

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")

src/Cluckwork.Api/Hosting/BindingFailureResponse.cs

@@ -0,0 +1,143 @@
+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
+// 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.
+
+// #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<T>(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) =>
+        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;
+
+                // #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.
+                //
+                // 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.
+    //
+    // #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)
+    {
+        // #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<IExceptionHandlerFeature>()?.Endpoint
+            ?? context.GetEndpoint();
+        if (endpoint 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<IAcceptsMetadata>()?.RequestType is not null
+                || endpoint.Metadata.GetMetadata<ReadsRequestBodyAttribute>() 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;
+    }
+}

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<IdempotencyOptions>(
     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<Microsoft.AspNetCore.Routing.RouteHandlerOptions>(
+    o => o.ThrowOnBadRequest = true);
+
 // --- OpenAPI ---
 builder.Services.AddOpenApi();
 
@@ -215,6 +233,17 @@
     // suite runs many) reassigns and disposes — completions would silently go
     // to another host's pipeline.
     options.Logger = app.Services.GetRequiredService<Serilog.ILogger>();
+    // 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
@@ -229,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();
@@ -396,9 +432,48 @@
     var exception = context.Features.Get<IExceptionHandlerFeature>()?.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.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
+        // 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.
+        // 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(
+                BindingFailureResponse.ConcernsRequestBody(context)),
         BadHttpRequestException bad => Results.Problem(
             detail: bad.Message,
             statusCode: bad.StatusCode,