fix(infra): AccountId must be a non-nullable Guid or both tenant write layers refuse (#673)

AGENTS.md

@@ -53,7 +53,7 @@ or adding an aggregate state.
 
 ```bash
 dotnet build Cluckwork.sln                 # warnings are errors — keep it clean
-dotnet test  Cluckwork.sln                 # 2278 tests as of 2026-09; integration needs Docker
+dotnet test  Cluckwork.sln                 # 2295 tests as of 2026-09; integration needs Docker
 ```
 
 - **Integration tests** spin up a real Postgres via Testcontainers (`docker` required). No SQLite — EF SQL semantics differ.
@@ -75,7 +75,7 @@ dotnet test  Cluckwork.sln                 # 2278 tests as of 2026-09; integrati
 ### Data and correctness
 
 - **Every aggregate mutation must bump `Version`.** `Version` is an EF concurrency token (`IsConcurrencyToken`): EF puts the *original* value in the UPDATE's `WHERE` but never auto-increments it, so a mutation without `Version++` silently loses concurrent races — both writers match `WHERE Version = N` — instead of 409ing. This shipped three times; each fix carries a parallel-race integration test, and so must any new mutation.
-- **Multi-tenancy:** every tenant-owned entity has `AccountId`, enforced by an EF **global query filter** plus a **`TenantStampInterceptor`** (stamps on insert, and rejects a mismatched write on update/delete). `TenantContext` resolves per-request from the JWT `account_id` claim and is **single-assignment** — a differing re-resolve throws; at startup it is unresolved, so seeders use `IgnoreQueryFilters()`. Several farms now coexist on one deployment: sign-in takes a farm code, and one email address can belong to a user in more than one farm. **`AccountId` is also an EF concurrency token on every entity that carries one (#562):** the `UPDATE`/`DELETE` the database runs carries `AccountId = <original>`, so a detached stub aimed at another farm's row matches nothing — the interceptor alone cannot see a detached write, and an owned-only edit on an attached stub was writing through until the token landed. The token comes from a model walk in `AppDbContext.OnModelCreating`, so a new entity is covered automatically; never remove it, and a database refusal under a resolved tenant is logged as `Tenant.WriteRefusedByDatabase`. **Identity's `AspNetUserRoles` carries a shadow `AccountId` plus a composite foreign key to `AspNetUsers(Id, AccountId)` (#670):** both layers select by property NAME, so the shadow column is stamped, verified and tokened like any other, and the FK makes a role grant to another farm's user — or under no resolved tenant — a database refusal; the other four user-keyed Identity tables have no writer in `src/` and are recorded as accepted risk. → [`530-multi-farm-tenancy.md`](docs/decisions/530-multi-farm-tenancy.md)
+- **Multi-tenancy:** every tenant-owned entity has `AccountId`, enforced by an EF **global query filter** plus a **`TenantStampInterceptor`** (stamps on insert, and rejects a mismatched write on update/delete). `TenantContext` resolves per-request from the JWT `account_id` claim and is **single-assignment** — a differing re-resolve throws; at startup it is unresolved, so seeders use `IgnoreQueryFilters()`. Several farms now coexist on one deployment: sign-in takes a farm code, and one email address can belong to a user in more than one farm. **`AccountId` is also an EF concurrency token on every entity that carries one (#562):** the `UPDATE`/`DELETE` the database runs carries `AccountId = <original>`, so a detached stub aimed at another farm's row matches nothing — the interceptor alone cannot see a detached write, and an owned-only edit on an attached stub was writing through until the token landed. The token comes from a model walk in `AppDbContext.OnModelCreating`, so a new entity is covered automatically; never remove it, and a database refusal under a resolved tenant is logged as `Tenant.WriteRefusedByDatabase`. **`AccountId` must be a non-nullable `Guid`, and both layers now fail closed on anything else (#673):** the walk throws `TenantAccountIdShapeException` at model build for any other CLR type and the interceptor throws it at `SaveChanges` for a mapped type that is not `Guid` or a value that does not box to one — before, a `Guid?` or a strongly-typed id got no token and no check, which is #562's detached-write hole reopened by one mapping with every test green. Map a new tenant-owned entity's `AccountId` as a plain `Guid`; a strongly-typed id there needs this decision reopened, not a cast. **Identity's `AspNetUserRoles` carries a shadow `AccountId` plus a composite foreign key to `AspNetUsers(Id, AccountId)` (#670):** both layers select by property NAME, so the shadow column is stamped, verified and tokened like any other, and the FK makes a role grant to another farm's user — or under no resolved tenant — a database refusal; the other four user-keyed Identity tables have no writer in `src/` and are recorded as accepted risk. → [`530-multi-farm-tenancy.md`](docs/decisions/530-multi-farm-tenancy.md)
 - **Flock-scoped EF reads are discovered from the model (#613).** Every mapped `Flock` or entity with a scalar `FlockId` must keep one structural `AccountId AND flock-scope` query filter; `UserRoleAssignment` is the sole deliberate exclusion because those rows resolve the scope itself. Two no-`FlockId` children rely on named parent/policy gates: daily-entry list/detail loads `DailyEntryGrade` only through a filtered `DailyEntry` aggregate and Worker-readable production totals correlate it through that same filtered parent; the lot movement ledger gates `EggInventoryMovement` through filtered `EggLot`; both children's direct exports remain `AdminOnly`. Any new exclusion or parent-derived child needs an explicit rationale and a causal mutation test; do not replace the model walk with a recalled entity list or expression `ToString()` checks.
 - **Transient-DB retry stops at unreplayable work (#269).** `EnableRetryOnFailure` covers self-contained EF units only; an automatic replay above a stateful detector (a counter, a CAS stamp, a single-use claim) cannot tell "this request racing itself" from the signal the detector exists to catch. Two cures, and picking wrong ships the bug: `SingleAttemptExecution` when the replay is itself observable, a durability probe on a self-minted token when the replay writes nothing. → [`269-transient-db-retry-boundary.md`](docs/decisions/269-transient-db-retry-boundary.md)
 - **`AuditEvents` is not time-partitioned, on purpose (#505).** The dominant read filters on `AccountId`+`EntityType`+`EntityId` with no date predicate, so monthly partitions would turn one index lookup into one per partition for no pruning benefit. If it is ever needed, partition by `AccountId`. → [`505-audit-events-no-time-partition.md`](docs/decisions/505-audit-events-no-time-partition.md)

docs/decisions/530-multi-farm-tenancy.md

@@ -208,6 +208,29 @@ and `RoleManager.DeleteAsync`, which deletes a *global* role and, through
 change tracker never holding an `IdentityUserRole` row; no caller exists, and one would be a
 farm-wide operation by construction, never a per-farm one.
 
+**Both layers now refuse an `AccountId` that is not a `Guid` (#673, 2026-09-05).** Latent, not
+live: every mapped `AccountId` is a non-nullable `Guid` today. But both layers *keyed* on that
+without requiring it — the #562 walk skipped any property whose `ClrType != typeof(Guid)` (so a
+`Guid?` or a strongly-typed id got no token) and `TenantStampInterceptor` skipped any value that
+did not box to a `Guid` (so a null `Guid?` was inserted unstamped and never checked on
+update/delete). One future entity mapped `public Guid? AccountId` therefore reopened #562's
+detached-write hole with every test green. The walk now **throws
+`TenantAccountIdShapeException` at model build** for any other CLR type — checked before the
+primary-key exclusion, so a key `AccountId` must be a `Guid` too, and that ordering is pinned by
+its own probe rather than left to a comment — which fails every boot and every test rather than
+degrading silently; the interceptor throws the same exception at
+`SaveChanges`, under a resolved tenant, on Added, Modified and Deleted alike — for a **mapped
+type** that is not `Guid` as well as for a value that is not a `Guid`, because a non-null `Guid?`
+boxes to `Guid` and the value check alone would accept it while the row carries no token to
+compare (found by the deep review round on #695, and the reason the interceptor holds without the
+walk rather than beside it). The two throws are independent on purpose: the interceptor's case is proved
+against a context that never runs the walk. `AccountIdConcurrencyTokenModelTests`' by-name
+selector stays as the second net for the whole set at once, and
+`AccountIdShapeFailClosedTests` pins both refusals plus the must-still-pass control that a plain
+`Guid` still gets its token. **What this does not change:** an unresolved tenant still disables
+the interceptor entirely (the CLI verbs and the design-time factory depend on it), so the write
+guard's floor is unchanged there.
+
 ---
 
 ## 6. Suspension is immediate for use

src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs

@@ -249,10 +249,28 @@ protected override void OnModelCreating(ModelBuilder builder)
         // The snapshot records the flag but EF emits no schema for it, so the
         // migration that accompanies this walk (AccountIdConcurrencyToken) is
         // deliberately empty — it exists to keep the snapshot equal to the model.
+        ApplyAccountIdTenantTokens(builder);
+    }
+
+    // Extracted so #673's fail-closed case can be proved against a throwaway
+    // entity: this walk is the only thing a probe context needs, and a mapping
+    // it must refuse cannot be added to AppDbContext's own model by definition.
+    internal static void ApplyAccountIdTenantTokens(ModelBuilder builder)
+    {
         foreach (var entityType in builder.Model.GetEntityTypes())
         {
             var accountId = entityType.FindProperty(nameof(Entity<Guid>.AccountId));
-            if (accountId is null || accountId.ClrType != typeof(Guid)) continue;
+            if (accountId is null) continue;
+
+            // #673 — fail CLOSED on any other CLR type instead of walking past
+            // it. A Guid? or a converted AccountId used to get no token here and
+            // no check in the interceptor, which is the #562 hole reopened with
+            // every test green. Throwing at model build makes it fail every boot
+            // and every test on the day it is mapped. Checked before the
+            // primary-key exclusion below, so a key AccountId must be a Guid too.
+            if (accountId.ClrType != typeof(Guid))
+                throw TenantAccountIdShapeException.ForModel(entityType.DisplayName(), accountId.ClrType);
+
             if (accountId.IsPrimaryKey()) continue;
             accountId.IsConcurrencyToken = true;
         }

src/Cluckwork.Infrastructure/Persistence/Interceptors/TenantStampInterceptor.cs

@@ -113,6 +113,17 @@ private void StampTenant(DbContext? context)
                 .FirstOrDefault(p => p.Metadata.Name == nameof(Entity<Guid>.AccountId));
             if (prop is null) continue;
 
+            // #673 review round — the SHAPE is checked here, not just the value
+            // below. A non-null Guid? boxes to Guid, so the value checks alone
+            // accept a nullable AccountId whose row carries no #562 concurrency
+            // token: the value looks right and the database is never told to
+            // compare it. AppDbContext's walk refuses that mapping at model
+            // build, but this layer is meant to hold on its own — a context
+            // configured without the walk is exactly the case it exists for.
+            if (prop.Metadata.ClrType != typeof(Guid))
+                throw TenantAccountIdShapeException.ForMapping(
+                    entry.Metadata.ClrType.Name, entry.State.ToString(), prop.Metadata.ClrType);
+
             switch (entry.State)
             {
                 case EntityState.Added:
@@ -149,7 +160,12 @@ private void StampTenant(DbContext? context)
 
     private void StampOrVerifyAdded(EntityEntry entry, PropertyEntry prop)
     {
-        if (prop.CurrentValue is not Guid accountId) return;
+        // #673 — anything that is not a Guid is unCHECKABLE, not exempt. This
+        // used to return, so a null Guid? was inserted unstamped and every
+        // later write to that row went unverified.
+        if (prop.CurrentValue is not Guid accountId)
+            throw TenantAccountIdShapeException.ForWrite(
+                entry.Metadata.ClrType.Name, nameof(EntityState.Added), prop.CurrentValue);
 
         if (accountId == Guid.Empty)
         {
@@ -164,7 +180,12 @@ private void StampOrVerifyAdded(EntityEntry entry, PropertyEntry prop)
 
     private void Verify(EntityEntry entry, object? value)
     {
-        if (value is not Guid accountId) return;
+        // #673 — same fail-closed rule as StampOrVerifyAdded: a value that is
+        // not a Guid cannot be compared to the tenant, so refuse the write
+        // rather than let it past unchecked.
+        if (value is not Guid accountId)
+            throw TenantAccountIdShapeException.ForWrite(
+                entry.Metadata.ClrType.Name, entry.State.ToString(), value);
 
         if (accountId != tenant.AccountId)
             throw new TenantWriteMismatchException(

src/Cluckwork.Infrastructure/Persistence/TenantAccountIdShapeException.cs

@@ -0,0 +1,40 @@
+namespace Cluckwork.Infrastructure.Persistence;
+
+// #673 — both write-side tenant layers key on AccountId being a non-nullable
+// Guid, and both used to fall THROUGH for any other shape: the model walk in
+// AppDbContext.OnModelCreating gave a Guid? or a converted AccountId no
+// concurrency token, and TenantStampInterceptor skipped any value that did not
+// box to a Guid. A tenant-owned entity mapped with `Guid? AccountId` therefore
+// reopened the #562 hole — a detached Update/Remove naming another farm's row
+// wrote through, with every test green.
+//
+// Both layers now throw this instead of continuing: at model build for the
+// wrong CLR type (so a mis-typed AccountId fails every boot and every test),
+// and at SaveChanges for a value that is not a Guid. Distinct from
+// TenantWriteMismatchException, which means a real cross-tenant VALUE; this one
+// means the mapping itself cannot be checked. Caught nowhere, deliberately.
+public sealed class TenantAccountIdShapeException : InvalidOperationException
+{
+    public string EntityType { get; }
+
+    private TenantAccountIdShapeException(string message, string entityType)
+        : base(message) => EntityType = entityType;
+
+    public static TenantAccountIdShapeException ForModel(string entityType, Type clrType) =>
+        new($"{entityType}.AccountId is mapped as {clrType.Name}, but tenant isolation requires a " +
+            "non-nullable Guid: the concurrency-token walk and the write guard can only check that " +
+            "shape, so any other one is a silent cross-tenant write (#673).",
+            entityType);
+
+    public static TenantAccountIdShapeException ForMapping(string entityType, string state, Type clrType) =>
+        new($"Refusing to save {state} {entityType}: its AccountId is mapped as {clrType.Name} rather than " +
+            "a non-nullable Guid, so the value this guard sees can be right while the row carries no " +
+            "concurrency token to compare it against (#673).",
+            entityType);
+
+    public static TenantAccountIdShapeException ForWrite(string entityType, string state, object? value) =>
+        new($"Refusing to save {state} {entityType}: its AccountId is " +
+            $"{(value is null ? "null" : $"a {value.GetType().Name}")} rather than a Guid, so the write " +
+            "guard cannot check it against the resolved tenant (#673).",
+            entityType);
+}

tests/Cluckwork.Api.IntegrationTests/AccountIdConcurrencyTokenModelTests.cs

@@ -29,15 +29,15 @@ public void EveryNonKeyAccountId_IsAConcurrencyToken()
     {
         using var db = BuildContext();
 
-        // Selected by NAME and non-key only — deliberately NOT by CLR type.
-        // The token walk skips any AccountId whose CLR type is not exactly Guid
-        // (so a Guid? or a converted id gets no token at all), and the
-        // interceptor skips any VALUE that does not box to a Guid (a Guid? that
-        // is null is neither stamped on Added nor checked on Modified/Deleted;
-        // a populated Guid? boxes to Guid and is checked). If this selector
-        // mirrored the walk's own predicate, such a property would vanish from
-        // the guard with every test green; naming the property instead makes it
-        // red the day one is mapped (#673 tracks closing that by construction).
+        // Selected by NAME and non-key only — deliberately NOT by CLR type. A
+        // selector mirroring the walk's own predicate would let a Guid? or a
+        // converted AccountId vanish from this guard with every test green.
+        //
+        // Since #673 the walk itself THROWS on any CLR type other than Guid, so
+        // BuildContext above would already have failed and this assertion is the
+        // second net rather than the first — kept because it names the whole
+        // set at once, and because the day the walk's throw is weakened is the
+        // day this has to be the one that reddens.
         var carriers = db.Model.GetEntityTypes()
             .Select(t => (Type: t, AccountId: t.FindProperty("AccountId")))
             .Where(x => x.AccountId is not null && !x.AccountId.IsPrimaryKey())