fix(audit): store catalog enums by name and guard the add-item transaction shape

src/Cluckwork.Application/Features/Catalog/CreateProduct/CreateProductHandler.cs

@@ -61,8 +61,15 @@ await products.AddMappingAsync(
                 ProductEggGradeMapping.Create(Guid.NewGuid(), accountId, product.Id, grade.Id),
                 transactionCt);
 
+            // #746 — the NAME, never the ordinal. AuditWriter serialises `details` with
+            // new JsonSerializerOptions(JsonSerializerDefaults.Web) and registers no
+            // JsonStringEnumConverter, so a bare enum stores as its underlying integer —
+            // meaningful only against the member order at write time, and silently
+            // re-read as a different member after any reorder. This is the house idiom
+            // (UpdateFarmSettingsHandler.cs:158-164). Pinned by
+            // CreateProduct_RecordsProductTypeByName.
             await audit.WriteAsync(AuditActions.ProductCreate, nameof(Product), product.Id,
-                details: new { product.Name, product.ProductType, EggGrade = grade.Name }, ct: transactionCt);
+                details: new { product.Name, ProductType = product.ProductType.ToString(), EggGrade = grade.Name }, ct: transactionCt);
 
             outcome = Result.Success(product.Id);
             return true;

src/Cluckwork.Application/Features/Catalog/UpdateProduct/UpdateProductHandler.cs

@@ -74,8 +74,16 @@ await unitOfWork.ExecuteInTransactionAsync(async transactionCt =>
             // (part 2), so history never reinterprets.
             mapping.Repoint(grade.Id);
 
+            // #746 — see CreateProductHandler: the NAME, never the ordinal.
+            // Pinned by UpdateProduct_RecordsDefaultUnitByName.
             await audit.WriteAsync(AuditActions.ProductUpdate, nameof(Product), product.Id,
-                details: new { product.Name, product.DefaultUnit, product.DefaultPriceMinorUnits, EggGrade = grade.Name },
+                details: new
+                {
+                    product.Name,
+                    DefaultUnit = product.DefaultUnit.ToString(),
+                    product.DefaultPriceMinorUnits,
+                    EggGrade = grade.Name,
+                },
                 ct: transactionCt);
 
             outcome = Result.Success();

src/Cluckwork.Application/Features/Sales/AddOrderItem/AddOrderItemHandler.cs

@@ -174,6 +174,13 @@ product.DefaultPriceMinorUnits is not { } catalogListPrice
         // because a joined scope's RollbackAsync is a no-op
         // (AmbientTransaction.cs:95).
         //
+        // #743 — and nothing may exit this delegate BELOW the inner save.
+        // A rollback then leaves the order and the item tracked as Unchanged,
+        // so a later flush on this same context drops them silently instead of
+        // re-writing them (#159's shape, one step worse). Enforced by
+        // TransactionDelegateShapeTests; if you need an exit there, add a
+        // DiscardChanges-style cleanup the way UpdateFarmSettingsHandler does.
+        //
         // Nullable, not a placeholder failure: a future branch that forgets to
         // set it NREs loudly at `return outcome!`, where a synthetic
         // Error.Validation would instead return a silent 400 carrying an error

tests/Cluckwork.Api.IntegrationTests/ProductAuditPayloadTests.cs

@@ -0,0 +1,111 @@
+namespace Cluckwork.Api.IntegrationTests;
+
+using System.Net;
+using System.Text.Json;
+using Cluckwork.Api.IntegrationTests.Infrastructure;
+
+// #746 — CreateProductHandler and UpdateProductHandler stored ProductType and
+// DefaultUnit as bare enums in their audit payloads. AuditWriter serialises
+// `details` with no JsonStringEnumConverter, so a bare enum stores as its
+// underlying ordinal — meaningful only against the member order at write
+// time, and silently re-read as a different member after any reorder.
+[Collection(IntegrationCollection.Name)]
+public sealed class ProductAuditPayloadTests(CluckworkWebApplicationFactory factory)
+{
+    private sealed record IdDto(Guid Id);
+    private sealed record AuditRow(Guid Id, string Action, Guid EntityId, string? DetailsJson);
+
+    private async Task<(HttpClient Client, Guid AccountId, Guid EggGradeId)> SetupAsync()
+    {
+        var email = $"u-{Guid.NewGuid():N}@test.local";
+        var accountId = await factory.SeedAccountWithUserAsync(email);
+        var farmId = Guid.NewGuid();
+        var grades = await factory.SeedEggGradesAsync(accountId, farmId, "Large");
+        var client = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(email));
+        return (client, accountId, grades["Large"]);
+    }
+
+    private async Task<Guid> CreatedId(HttpResponseMessage response)
+    {
+        Assert.Equal(HttpStatusCode.Created, response.StatusCode);
+        return (await response.Content.ReadFromJsonAsync<IdDto>())!.Id;
+    }
+
+    private async Task<AuditRow> SingleAuditRowAsync(HttpClient client, string action, Guid entityId)
+    {
+        var rows = await client.GetFromJsonAsync<List<AuditRow>>(
+            $"/api/v1/audit?action={action}&entityId={entityId}");
+        return Assert.Single(rows!);
+    }
+
+    [Fact]
+    public async Task CreateProduct_RecordsProductTypeByName()
+    {
+        var (client, _, eggGradeId) = await SetupAsync();
+
+        var productId = await CreatedId(await client.PostWithKeyAsync(
+            "/api/v1/products", Guid.NewGuid().ToString(),
+            new
+            {
+                name = "Large Eggs",
+                productType = "Egg",
+                defaultUnit = "Egg",
+                defaultPriceMinorUnits = 500,
+                eggGradeId,
+            }));
+
+        var row = await SingleAuditRowAsync(client, "Product.Create", productId);
+        using var details = JsonDocument.Parse(row.DetailsJson!);
+        var root = details.RootElement;
+
+        // The NAME, never the ordinal (Egg is ordinal 0, so a stored 0 is
+        // invisible to a lazy assertion). AuditWriter registers no
+        // JsonStringEnumConverter, so a bare enum serialises as its
+        // underlying integer here; GetString() throws
+        // InvalidOperationException on a JSON number, which is why this
+        // fails LOUDLY instead of pinning a number that silently re-reads as
+        // a different member after ProductType is reordered.
+        Assert.Equal("Egg", root.GetProperty("productType").GetString());
+        Assert.Equal("Large Eggs", root.GetProperty("name").GetString());
+        Assert.Equal("Large", root.GetProperty("eggGrade").GetString());
+    }
+
+    [Fact]
+    public async Task UpdateProduct_RecordsDefaultUnitByName()
+    {
+        var (client, _, eggGradeId) = await SetupAsync();
+
+        var productId = await CreatedId(await client.PostWithKeyAsync(
+            "/api/v1/products", Guid.NewGuid().ToString(),
+            new
+            {
+                name = "Large Eggs",
+                productType = "Egg",
+                defaultUnit = "Egg",
+                defaultPriceMinorUnits = 500,
+                eggGradeId,
+            }));
+
+        var updated = await client.PutWithKeyAsync(
+            $"/api/v1/products/{productId}", Guid.NewGuid().ToString(),
+            new
+            {
+                name = "Large Eggs Packed",
+                defaultUnit = "Tray",
+                defaultPriceMinorUnits = 650,
+                eggGradeId,
+            });
+        Assert.Equal(HttpStatusCode.NoContent, updated.StatusCode);
+
+        var row = await SingleAuditRowAsync(client, "Product.Update", productId);
+        using var details = JsonDocument.Parse(row.DetailsJson!);
+        var root = details.RootElement;
+
+        // The NAME, never the ordinal (Tray is ordinal 3 — distinct from
+        // Egg's 0, so a transposition of these two fields reddens too).
+        Assert.Equal("Tray", root.GetProperty("defaultUnit").GetString());
+        Assert.Equal("Large Eggs Packed", root.GetProperty("name").GetString());
+        Assert.Equal(650, root.GetProperty("defaultPriceMinorUnits").GetInt64());
+        Assert.Equal("Large", root.GetProperty("eggGrade").GetString());
+    }
+}

tests/Cluckwork.Application.Tests/Sales/TransactionDelegateShapeTests.cs

@@ -0,0 +1,199 @@
+namespace Cluckwork.Application.Tests.Sales;
+
+using System.Linq;
+using Cluckwork.Application.Tests.TenantBypass;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+// #743 — a syntax guard, not a behavioural one. The hazard: a handler that
+// calls unitOfWork.SaveChangesAsync(...) INSIDE its ExecuteInTransactionAsync
+// delegate (so an audit row can carry an EF-assigned id — AddOrderItemHandler
+// does this on purpose, see its #722/#743 comment) must not exit that
+// delegate — `return false` or a throw — BELOW that save. A rollback after
+// the inner save leaves the just-saved entities tracked as Unchanged rather
+// than Added/Modified, so a later flush on the same scoped AppDbContext does
+// not re-write them; it silently drops them. Today the only `return false` in
+// AddOrderItemHandler's delegate sits above the save, so this is not a live
+// bug — it is a shape the compiler happily accepts the moment someone adds an
+// exit below the save (verified: inserting one there still builds clean).
+//
+// This is necessarily SYNTACTIC, not behavioural: the hazard does not exist
+// on the current tree, so no runtime test can exercise it. The rule is an
+// ALLOW-LIST, not a ban-list: `ExecuteInTransactionAsync`'s delegate signals
+// "commit" with exactly `return true;`, so below the last save, every
+// `return` whose expression is not the literal `true` is flagged — a bare
+// `return false;`, a variable, a ternary, a call, all of it — because every
+// one of those means "do not commit", which is the #743 hazard. (Review
+// round 2 found that matching only the literal `false` missed
+// `return shouldAbort ? false : true;` and any non-literal return entirely.)
+// Its stated limit: it catches any `return` or `throw` STATEMENT below the
+// save. It does NOT catch an exception thrown by a call below the save that
+// isn't itself a `throw` statement in this delegate (e.g. a nested call that
+// throws) — banning every call below the save would ban the audit write
+// itself. That case is real but already covered behaviourally by
+// SalesOrderAuditPayloadTests.AddItem_WhenTheAuditWriteFails_RollsBackTheLine,
+// which drives the actual rollback-after-save path that exists today (the
+// audit write, which is itself after the save).
+public sealed class TransactionDelegateShapeTests
+{
+    private sealed record InScopeDelegate(string File, int Line);
+
+    private static string RepoRoot() =>
+        GuardScanner.FindRepoRoot(AppContext.BaseDirectory)
+            ?? throw new InvalidOperationException("repo root not found");
+
+    private static IReadOnlyList<string> EnumerateSrcFiles(string root)
+    {
+        var srcRoot = Path.Combine(root, "src");
+        var files = new List<string>();
+
+        void Walk(string dir)
+        {
+            foreach (var sub in Directory.EnumerateDirectories(dir))
+            {
+                var name = Path.GetFileName(sub);
+                if (name is "bin" or "obj") continue;
+                Walk(sub);
+            }
+
+            files.AddRange(Directory.EnumerateFiles(dir, "*.cs", SearchOption.TopDirectoryOnly));
+        }
+
+        Walk(srcRoot);
+        return files.OrderBy(f => f, StringComparer.Ordinal).ToList();
+    }
+
+    private static string? InvokedName(InvocationExpressionSyntax invocation) => invocation.Expression switch
+    {
+        MemberAccessExpressionSyntax member => member.Name.Identifier.ValueText,
+        IdentifierNameSyntax identifier => identifier.Identifier.ValueText,
+        _ => null,
+    };
+
+    // Descendants of `root`, but stop descending the moment a nested
+    // AnonymousFunctionExpressionSyntax is hit — its body belongs to a
+    // DIFFERENT delegate and must not be attributed to this one. The nested
+    // lambda node itself still comes back from Roslyn (only its children are
+    // skipped), which is harmless: callers only look for specific node types.
+    private static IEnumerable<SyntaxNode> DescendantsExcludingNestedLambdas(SyntaxNode root) =>
+        root.DescendantNodes(n => n is not AnonymousFunctionExpressionSyntax);
+
+    private static string RelativePath(string root, string file) =>
+        Path.GetRelativePath(root, file).Replace('\\', '/');
+
+    [Fact]
+    public void NoExitBelowTheInnerSaveInAnyExecuteInTransactionDelegate()
+    {
+        var root = RepoRoot();
+        var files = EnumerateSrcFiles(root);
+
+        // A path-filter bug that silently excludes a subtree would otherwise
+        // read as "no violations". Measured 2026-09-10:
+        // `find src -name '*.cs' -not -path '*/bin/*' -not -path '*/obj/*' | wc -l`
+        // returns 454; the floor is 400 to match the sibling guard on the same
+        // tree, GuardScanner.RealTreeFileFloor, leaving headroom for growth
+        // while still catching a walk that silently excluded a whole subtree.
+        Assert.True(files.Count >= 400,
+            $"walk saw only {files.Count} .cs files under src/ — the floor is 400. The scanner is not seeing the tree.");
+
+        var parseErrors = new List<string>();
+        var violations = new List<string>();
+        var siteShapeFailures = new List<string>();
+        var inScope = new List<InScopeDelegate>();
+        var skipped = new List<InScopeDelegate>();
+
+        foreach (var file in files)
+        {
+            var text = File.ReadAllText(file);
+            var tree = CSharpSyntaxTree.ParseText(text, path: file);
+
+            foreach (var diag in tree.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error))
+            {
+                parseErrors.Add(
+                    $"{RelativePath(root, file)}:{diag.Location.GetLineSpan().StartLinePosition.Line + 1}: {diag.Id} {diag.GetMessage()}");
+            }
+
+            var root2 = tree.GetCompilationUnitRoot();
+            var executeCalls = root2.DescendantNodes()
+                .OfType<InvocationExpressionSyntax>()
+                .Where(inv => InvokedName(inv) == "ExecuteInTransactionAsync");
+
+            foreach (var call in executeCalls)
+            {
+                var line = tree.GetLineSpan(call.Span).StartLinePosition.Line + 1;
+                var relFile = RelativePath(root, file);
+
+                if (call.ArgumentList.Arguments.Count == 0
+                    || call.ArgumentList.Arguments[0].Expression is not AnonymousFunctionExpressionSyntax anon
+                    || anon.Block is null)
+                {
+                    siteShapeFailures.Add(
+                        $"{relFile}:{line}: ExecuteInTransactionAsync's first argument is not a lambda with a " +
+                        "block body — the guard's assumptions about this call site's shape moved and it must be re-taught, not skipped.");
+                    continue;
+                }
+
+                var saveCalls = DescendantsExcludingNestedLambdas(anon.Block)
+                    .OfType<InvocationExpressionSyntax>()
+                    .Where(inv => InvokedName(inv) == "SaveChangesAsync")
+                    .ToList();
+
+                if (saveCalls.Count == 0)
+                {
+                    // This delegate does not save inside itself — the hazard
+                    // this guard exists for is not present. Not in scope.
+                    skipped.Add(new InScopeDelegate(relFile, line));
+                    continue;
+                }
+
+                inScope.Add(new InScopeDelegate(relFile, line));
+                var lastSaveEnd = saveCalls.Max(s => s.Span.End);
+
+                // Allow-list, not a ban-list: the only legitimate exit below
+                // the save is `return true;` (commit). Any other return
+                // value — a literal `false`, a variable, a ternary, a call —
+                // means "do not commit", which is the hazard, so it is
+                // flagged regardless of how it is spelled.
+                var exitsAfterSave = DescendantsExcludingNestedLambdas(anon.Block)
+                    .Where(n => n.SpanStart > lastSaveEnd)
+                    .Where(n =>
+                        n is ThrowStatementSyntax
+                        || (n is ReturnStatementSyntax ret
+                            && !(ret.Expression is LiteralExpressionSyntax retLit
+                                 && retLit.IsKind(SyntaxKind.TrueLiteralExpression))));
+
+                foreach (var exit in exitsAfterSave)
+                {
+                    var exitLine = tree.GetLineSpan(exit.Span).StartLinePosition.Line + 1;
+                    violations.Add(
+                        $"{relFile}:{exitLine} exits the `ExecuteInTransactionAsync` delegate **after** its inner " +
+                        "`SaveChangesAsync`. The rollback undoes the rows but leaves the entities tracked as " +
+                        "`Unchanged`, so a later flush on the same context silently drops them (#743, #159). " +
+                        "Either move the exit above the save, or add a `DiscardChanges`-style cleanup on the " +
+                        "`!committed` branch the way `UpdateFarmSettingsHandler` does.");
+                }
+            }
+        }
+
+        // A file Roslyn cannot read is a hole, not a pass. Assert.True with an
+        // explicit message (not Assert.Empty) because xUnit's collection
+        // preview truncates around 100 chars — Assert.Empty here would hide
+        // exactly the text (file, line, what to do) the next reader needs.
+        Assert.True(parseErrors.Count == 0,
+            "Roslyn could not parse:\n" + string.Join("\n", parseErrors));
+        // The guard's shape assumptions must be re-taught explicitly, never silently skipped.
+        Assert.True(siteShapeFailures.Count == 0,
+            string.Join("\n\n", siteShapeFailures));
+
+        Assert.True(violations.Count == 0,
+            string.Join("\n\n", violations));
+
+        // Assert the guard has something to guard: if a future refactor moves
+        // the save out of every ExecuteInTransactionAsync delegate, THIS is
+        // what tells the next reader the guard went vacuous instead of
+        // passing forever on an empty set.
+        Assert.NotEmpty(inScope);
+        Assert.Contains(inScope, d => d.File.EndsWith("AddOrderItemHandler.cs", StringComparison.Ordinal));
+    }
+}