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

[mforce]

Two audit-payload correctness items on the code #722 just shipped, in one PR because neither is more than a handful of lines and both are the same subject.

#746 — catalog enums stored as ordinals

AuditWriter serialises details with new JsonSerializerOptions(JsonSerializerDefaults.Web) and registers no JsonStringEnumConverter, so a bare enum in a payload is stored as its underlying integer. Two call sites did exactly that:

File	Field	Was
CreateProductHandler.cs:65	ProductType	ordinal
UpdateProductHandler.cs:78	DefaultUnit	ordinal

A stored ordinal is only meaningful against the enum's member order at the time it was written. Reorder or insert a member and every historical row silently re-reads as a different value, with nothing failing — the JSON is still valid and the number is still a number. That is a data-retention defect in the one table whose purpose is being trustworthy after the fact.

Both now store the name, which is the house idiom for this payload shape (UpdateFarmSettingsHandler.cs:158-164, UpdateEggUnitConversionHandler.cs:30, and #722's own listPriceBasis).

Deliberately not done: adding a converter to AuditWriter's options. That would silently change the shape of every existing payload across ~25 call sites.

Existing rows are not repaired. They carry ordinals and cannot be recovered from the data alone — a backfill would need the enum's member order at write time, which is not recorded. This stops the defect widening.

#743 — nothing notices an exit below the inner save

AddOrderItemHandler calls unitOfWork.SaveChangesAsync(token) inside its ExecuteInTransactionAsync delegate, so the audit row can carry the SalesOrderItem.Id EF assigns at that save. That is deliberate (#722) and correct.

The hazard is what happens after it. If the delegate exits below that save, the transaction rolls back — but the order and the new item are now tracked as Unchanged, i.e. they look persisted. A later flush on the same scoped AppDbContext would not re-write them; it would silently drop them. UpdateFarmSettingsHandler guards the analogous case with accounts.DiscardChanges(account) on its !committed branch (:104-109); this delegate has no equivalent.

It is not a bug today — the only return false sits above the save, and all three of SalesOrder.AddItem's failure paths return before _items.Add. It becomes one the moment someone adds an exit below the save, and the compiler does not stop them: if (result.Value.Id == Guid.Empty) return false; inserted there builds clean. That was verified by running it, not assumed.

So this adds the guard that notices. TransactionDelegateShapeTests walks every .cs under src/ with Roslyn, finds every ExecuteInTransactionAsync delegate that saves inside itself, and fails on a return false or throw below that save. It is deliberately fail-closed in four ways: a parse error fails, a call site whose shape it does not recognise fails rather than being skipped, a file-count floor catches a path-filter bug that silently excludes a subtree, and it asserts the in-scope set is non-empty and still contains AddOrderItemHandler.cs — so a future refactor that moves the save out shows up as "this guard went vacuous" instead of passing forever on an empty set.

Its stated limit, recorded in the file header: it catches a syntactic return false or throw. It does not catch an exception thrown by a call below the save — banning that would ban the audit write itself. That case is real and is covered behaviourally by AddItem_WhenTheAuditWriteFails_RollsBackTheLine, which drives exactly that path.

AddOrderItemHandler now carries a comment naming the guard, per the repo rule that a syntax guard has to be readable from the call site it guards.

Verification

Driver-verified on this head (not quoted from the implementer):

Check	Result
dotnet build Cluckwork.sln	0 Warning(s), 0 Error(s)
Cluckwork.Domain.Tests	388 / 388, unchanged
Cluckwork.Application.Tests	260 → 261, all green (+1: the guard)
Cluckwork.Api.IntegrationTests	1726 → 1728, all green (+2: the payload tests)

The before-counts are read off main's own CI run at 1a07441, not remembered.

Mutation ledger

The control row is the one that matters most — a ledger of only-red rows cannot show that a guard discriminates rather than merely fires.

Row	Mutation	Expected	Observed
C (control)	return false above the inner save — legal	GREEN	GREEN
M3	return false below the inner save — #743 itself	RED	RED
M4	inner save deleted — the guard's anchor gone	RED (fail-closed)	RED
M5	the same hazard with no literal false — return shouldAbort ? false : true;	RED	RED
M1	drop .ToString() on ProductType	RED	RED
M2	drop .ToString() on DefaultUnit	RED	RED

Each mutant was built, run, then restored root-anchored (git restore --source=HEAD --staged --worktree) with git diff --exit-code clean, rebuilt and re-run green. M1 and M2 each reddened only their own test, so they are targeted rather than a blanket break. The whole ledger was re-run on the final head after the last review commit — a value carried from an earlier head is not a value here.

What review actually bought

Three findings, all in the guard, none in the two handler edits:

1. The file-count floor was 300 against a real tree of 454 files (measured, not remembered) — 154 files of slack, enough for a path-filter bug that dropped a whole project to still read as "no violations". Raised to 400, matching GuardScanner.RealTreeFileFloor on the same tree.
2. Assert.Empty(violations) truncated the message xUnit prints, so the guard's instruction — the entire product of Sales: AddOrderItemHandler saves inside its transaction delegate with no rollback cleanup for tracked entities #743 — was unreadable when it fired. Now message-carrying.
3. M5 above. The guard originally matched only a literal false, so return shouldAbort ? false : true; below the save built clean and the guard stayed green — the identical defect, invisible. Found by an adversarial seat that ran the mutation instead of reading the code. The rule is now an allow-list: below the last save, the only permitted exit is return true;, which is also the simpler guard.

Closes #746.
Closes #743.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 2d558afe-3324-447c-b3b6-30eec3653c07

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}