refactor(users): put user administration behind IAccessModule and peer reads behind IAccessLookup

[mforce]

Nothing changes for users. Every users, me and flock-assignment route keeps its statuses, bodies, step-up checks and audit rows. Confirming a sale still reads the actor's role and flock assignments inside its transaction, after the account and order locks.

For maintainers, Access now has one surface for HTTP adapters, IAccessModule, and one read port for peer modules, IAccessLookup. The effective-role query exists once.

This is PR B of the Access contract (#857, design v2). It moves confirm's authorization reads, so it needs a security review.

Refs #857

What changed

1. Commit 1, 446cbe25: one new test, green on main. ConfirmSaleAssignmentReadTests covers the one gap in the existing tests. SaleAllocationPolicyTests pins when confirm reads the role and the allocation policy, but nothing pinned when it reads a worker's assignments. The test parks a restricted worker's confirm on the account lock while another transaction deletes the worker's only assignment. The confirm must then allocate as an unrestricted worker. It passes on main and on this head.

2. Commit 2, 516d45bd: the move.

   • IAccessLookup (Application.Features.Users): GetEffectiveRoleAsync and ListFlockAssignmentsAsync, the latter returning FlockAssignmentDetails(Id, FlockId) copies, never entities.
     • AccessLookup (Infrastructure.Identity) depends only on the scoped AppDbContext and, since commit 5, the scoped TenantContext that context's query filter reads. The flock-scope middleware resolves it on every request, so nothing heavier is built per request.
     • Callers: ConfirmSaleHandler, FlockScopeGuard and FlockScopeResolutionMiddleware. ConfirmSaleHandler no longer takes IUserRoleAssignmentRepository or IIdentityProvider.
   • One effective-role routine. AccessLookup.ReadEffectiveRoleAsync holds the query: the active-membership AnyAsync with DisabledAt == null, then the role join, then Roles.ResolveEffective, all verbatim. IdentityProvider.GetEffectiveRoleAsync now calls it, and its constructor is unchanged.
   • IAccessModule (Application.Features.Users), with AccessModule in Infrastructure.Identity. It has 14 members for users, me and assignments. Each one forwards to the existing handler, to IIdentityProvider, or to the joined assignment read (ListByNameByUserAsync, still one LEFT JOIN). It takes IIdentityProvider from DI, never the concrete provider, so the decorators in MustChangePasswordGateTests and StepUpAuthTests stay in the path. UserEndpoints and MeEndpoints take only IAccessModule for Access.
   • Ledger.
     • Access → Farm gains IAccessLookup and AccessLookup (20 → 22 symbols).
     • The Commerce → Access edge reason now names IAccessLookup.
     • UserEndpoints.ListUsers and MeEndpoints.GetMe gain adapter rows. They used to reach only the Platform port IIdentityProvider; now they reach Access, so the matrix cell becomes A (45).
     • The two tenant-bypass rows for the role query follow it to AccessLookup.ReadEffectiveRoleAsync. The query token hash is unchanged (4adcdb84).
     • The matrix is regenerated.

3. Commit 3, 81ec39c0: three more tests, from the mutation run. No suite put two workers with different assignments in one farm, or gave one worker both a flock row and a farm-wide row. So a lookup that returned the whole farm's rows (B5) or dropped farm-wide rows (B4) passed everything. B5 would let a restricted worker inherit a colleague's farm-wide access, and that gap existed on main too. AccessLookupTests seeds exactly those shapes, at the port and through the flock-scope middleware.

4. Commit 4, 4d31a9f5: review hardening (Opus F1, F2). Both gaps predate this PR; the port now owns them.

   • F1, fail closed. IAccessLookup.ListFlockAssignmentsAsync relies on the tenant query filter. Under an unresolved tenant it read no rows, and no rows means account-wide access. It now throws before the query, so the SQL is unchanged. I checked every caller, and each resolves the tenant first:
     • HTTP resolves it together with the actor;
     • both seeders resolve it before acting;
     • confirm reads the tenant's account first.
       Commit 5 moved where the guard reads the tenant from.
   • F2, account scope. Nothing pinned the account predicate in the shared effective-role query. A new test gives one email a different role in two farms and requires each farm to see only its own user's role.

5. Commit 5, e628995a: owner design fix. Commit 4 read the tenant through a new AppDbContext.Tenant getter, which made the DbContext carry request state for one caller. AccessLookup now takes the same scoped TenantContext through its own constructor, next to AppDbContext, and the getter is gone. AppDbContext matches main again (git diff origin/main -- src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs is empty). The guard, its message and the SQL are unchanged. The existing FlockScope getter stays.

The branch is rebased onto a71ec7af (#1033, the peer-module guard). #1033 changed no src. Its guard does not check Access yet, because Access has no contract until PR E. It reads parameter types, and B's new Access members take no Farm type as a parameter. After commit 5, main at 02e8a633 (#1035 and #1036) was merged in with a merge commit. B's diff against main is unchanged: the same 19 files, with the same content.

SQL and response trace

I adapted #854's trace harness (a scratch test, not committed). It records every SQL statement and transaction event, the status, and the normalised body for 21 requests:

• every users route: list, create, update, set password, the assignment list empty and populated, assign, unassign, change role, change email, disable, enable;
• every me route: get, set language, set the stepper unit, clear it;
• a restricted worker's flock list (middleware read), water usage on an assigned and an unassigned flock (guard read), and that worker's confirm;
• an Owner's confirm.

Two runs on main were identical. The head differs from main in exactly these lines, and nowhere else:

Request	Line	main	head
13 worker confirm, the confirm's assignment read	43	ORDER BY u."Id"	(dropped)
10 to 13, the flock-scope middleware's assignment read	4	u."UserId" = @user_UserId	u."UserId" = @userId
11 and 12, FlockScopeGuard's assignment read	19	u."UserId" = @user_UserId	u."UserId" = @userId

Why each difference is safe:

• The dropped ORDER BY was planned in v2. All three callers use the rows as a set: unrestricted when there are no rows or any farm-wide row, otherwise the set of flock ids.
• The parameter rename changes no semantics. The captured variable is now the port's userId argument, not user.UserId.

Every status, every response body and every statement count is unchanged across the 21 requests. The trace was re-run after the rebase, after commit 4 and after commit 5, and is identical each time.

Mutation table

Each mutant was applied by exact single-match replacement, built, and run against AccessLookupTests, SaleAllocationPolicyTests, ConfirmSaleAssignmentReadTests, FlockScopeTests, FlockScopeMiddlewareTests, NamedRowProjectionTests, MeEndpointsTests, StepUpAuthTests, AdminGatingTests, RoleMatrixTests, TenantResolutionMiddlewareTests (176 tests) and the Application FlockScopeGuardTests. Each was then reverted. B1 to B3 and B7 to B11 ran before the rebase; the rebase changed no src. B4 to B6 and the restore check ran on this head.

#	Mutant	Red	Failing assertion, quoted
B1	role routine drops DisabledAt == null	SaleAllocationPolicyTests.ConfirmSale_ParkedOnTheAccountLock_IsForbidden_WhenTheActorIsDisabledWhileItWaits	Expected: Forbidden Actual: OK
B2	confirm reads the role before the account lock	the same test	Expected: Forbidden Actual: OK
B3	confirm reads assignments before the account lock	ConfirmSaleAssignmentReadTests (new; survived without it)	Expected: OK Actual: UnprocessableEntity
B4	lookup drops farm-wide rows	AccessLookupTests ×2 (new; survived without them)	HashSets differ Expected: [<flockA>, null] Actual: [<flockA>]
B5	lookup returns every user's rows in the farm	AccessLookupTests ×2 (new; survived without them)	restricted worker sees [<flockA>, <flockB>], expected [<flockA>]
B6	lookup always returns no rows	15: SaleAllocationPolicyTests ×5, RoleMatrixTests ×2, FlockScopeTests ×6, AccessLookupTests ×2	FlockScopeTests.ScopedWorker_FlocksList_SeesOnlyAssignedFlock: Assert.DoesNotContain() Failure
B7	FlockScopeGuard reads before the actor check	FlockScopeGuardTests (Application)	NpgsqlException : Failed to connect to 127.0.0.1:1, the unreachable database proving a read happened
B8	middleware skips the assignment read	FlockScopeTests ×6	Assert.DoesNotContain() Failure
B9	module swaps account and user ids on SetLanguageAsync	MeEndpointsTests ×7	Expected: NoContent Actual: NotFound
B10	module passes Guid.Empty as the acting user on CreateUserAsync	StepUpAuthTests ×5, AdminGatingTests ×3	Expected: Created Actual: Forbidden
B11	module serves assignments without flock names (not the LEFT JOIN)	NamedRowProjectionTests ×4	Expected: "<flock name>" Actual: null
O2	role routine drops u.AccountId == accountId (Opus; survived before commit 4)	AccessLookupTests.GetEffectiveRole_AnswersOnlyForTheAccountItIsAskedAbout	Assert.Null() Failure Expected: null Actual: Manager
G1	the unresolved-tenant guard deleted	the build, since commit 5	CS9113: Parameter 'tenant' is unread. The constructor dependency exists only for the guard, so deleting the guard no longer compiles.
G1n	the guard neutered to !tenant.IsResolved && userId == Guid.Empty, so it still reads tenant (commit 5)	AccessLookupTests.ListFlockAssignments_RefusesAnUnresolvedTenant	Assert.Throws() Failure: No exception was thrown Expected: typeof(System.InvalidOperationException)
O1	IAccessLookup registered Singleton (captive root context and root tenant), guard in place; re-run after commit 5	31 of 74, the same 31 tests as with the getter; every one fails closed	27 Expected: <status> Actual: InternalServerError; 2 the guard's InvalidOperationException; FlockScopeTests.ScopedWorker_ChildRows_AreScoped reads an error body where it expected a list; the confirm in ConfirmSaleAssignmentReadTests ends before it parks. None shows a widened flock set.
O1+G1n	the same Singleton with the guard neutered (Opus's original O1)	15 of 74; restricted workers become unrestricted (fail open), which is what the guard prevents	AccessLookupTests.ARestrictedWorker_DoesNotInheritAnotherWorkersFarmWideRow: HashSets differ Expected: [<flockA>] Actual: [<flockA>, <flockB>]
—	all reverted	176 of 176 and 1 of 1 pass, then 74 of 74 after commit 4 and again after commit 5

O1+G1 failed 16 tests last round and O1+G1n fails 15. The one difference is SaleAllocationPolicyTests.DefaultPolicy_RestrictedWorker_SufficientAssignedStock_AllocatesFromAssignedFlockOnly. Its two lots share a production date, so under a fail-open mutant FIFO breaks the tie on the lots' random ids, and the test passes whenever the assigned lot sorts first.

Why a Singleton still fails closed with constructor injection. The root provider builds a Singleton AccessLookup from the root scope's AppDbContext and TenantContext. Each is a single instance, and that context's query filter reads that same TenantContext, so the guard and the filter cannot disagree. In the serving process nothing resolves the root TenantContext. Every Resolve call runs on a scoped instance, either the tenant middleware's InvokeAsync parameter or a job's own scope, and CLI verbs run in their own one-shot process. So the captive tenant stays unresolved and the guard throws. This holds only while that stays true, and it held equally for the getter.

Test counts

Measured on this machine.

Suite	main a71ec7af	head 81ec39c0
Cluckwork.Application.Tests (full, including #1033's peer guard and every ledger guard)	702	702 (also after commit 4)
Integration: every class that calls a users, me or assignment route, confirm, or a flock-scoped path (82 classes on main; 84 at the head with the two new ones)	909	913 (+4 new)
After commit 4: the same classes, the five AccessLookupTests, and the six seeder classes (DemoSeedTests, DemoSeedActorTests, SeedCommandTests, SimulationSeedCommandTests, SimulationDeepSeedDrainTests, SimulationDuplicateFlockNameTests), because the guard sits on the seeders' flock-scoped writes	—	931 of 931
After commit 5 and the merge of main 02e8a633: the same 931, plus #1036's CredentialEpochFloorTests. The Application suite is 702 of 702.	—	946 of 946

The full integration suite was not run locally; CI runs it. One run on commit 5, before the merge, timed out SeedCommandTests.SeedCommand_Demo_SeedsDataAndExitsWithoutStartingKestrel at its 60-second limit. The host's load average was 16 on 12 cores, and the seed was still writing rows when it was killed. Alone, SeedCommandTests passed 11 of 11, with that seed taking 26 seconds, and it passed again in the 946-test run.

Test edits

Four existing test lines changed. All are construction lines that now pass the same unreachable AppDbContext, and since commit 5 a TenantContext, through the real AccessLookup, so each "no read before X" proof is unchanged. Three pass the test's own tenant. FlockScopeGuardTests passes a fresh unresolved TenantContext, which keeps the edit to its one line. Its guard returns Unauthorized before the lookup runs, so the instance never matters.

File	Lines
FlockScopeGuardTests.cs	1
FlockScopeMiddlewareTests.cs	2
TenantResolutionMiddlewareTests.cs	1

Correction to design v2: v2 promised three lines. Its census missed TenantResolutionMiddlewareTests.cs:201, which also builds FlockScopeResolutionMiddleware by hand. That class is not on B's required-green list. The only way to avoid the fourth line was to construct AccessLookup by hand inside the middleware and the guard, bypassing DI, and I judged that worse. Every other existing test file is unedited (git diff --numstat origin/main...HEAD -- tests/).

Deslop record

/deslop, and a by-hand /no-comments pass (subagents are off for this slice):

• The src diff is forwarding only: endpoint parameter swaps, three callers onto the port, and one query moved verbatim. Nothing was removable.
• Every new comment states a reason the code can't show: the lookup's dependency limit, the DI decorator path, and the shared role routine.
• One fixture was fixed before commit: a farm-wide assignment row must name its farm.

Change map

BEFORE
 UserEndpoints (11 routes) --> CreateUser/UpdateUser/SetUserPassword/ChangeUserRole/ChangeUserEmail/
                               DisableUser/EnableUser/AssignFlock/UnassignFlock handlers,
                               IUserRoleAssignmentRepository (list), IIdentityProvider (list users)
 MeEndpoints (3 routes)    --> IIdentityProvider (get me), SetLanguage/SetStepperUnit handlers
 ConfirmSaleHandler [Commerce] --> IIdentityProvider.GetEffectiveRoleAsync + IUserRoleAssignmentRepository.ListByUserAsync
 FlockScopeGuard [Platform]                 --> db.UserRoleAssignments
 FlockScopeResolutionMiddleware [Platform]  --> db.UserRoleAssignments
 IdentityProvider.GetEffectiveRoleAsync     --> its own copy of the role query

AFTER
 UserEndpoints, MeEndpoints --> IAccessModule --> AccessModule [Infrastructure.Identity] --> the same handlers,
                                                  IIdentityProvider (from DI), ListByNameByUserAsync (same LEFT JOIN)
 ConfirmSaleHandler, FlockScopeGuard, FlockScopeResolutionMiddleware
                            --> IAccessLookup --> AccessLookup(AppDbContext, TenantContext) [Infrastructure.Identity]
                                                    ReadEffectiveRoleAsync  <-- IdentityProvider.GetEffectiveRoleAsync
                                                    ListFlockAssignmentsAsync
 ledger: Access -> Farm W 20 -> 22 (+IAccessLookup, +AccessLookup); Commerce -> Access R (reason names IAccessLookup);
         Platform/adapters -> Access A 43 -> 45 (+GetMe, +ListUsers)
 unchanged: AssignFlockHandler (Access-internal) still reads the role through IIdentityProvider, which runs the shared routine;
            the seeders' IUserRoleAssignmentRepository and CreateUserAsync wait for PR E (CreateUserAsync moves onto IAccessOperations).

 Other open work: #1033 (#1023, peer-module guard) also edits module-ledger.json and AdapterReachScanner.
 Whichever merges second updates from main and regenerates the matrix.

Review notes

• Security review requested. B moves confirm's authorization reads. The properties to refute:
  • both reads stay inside the confirm transaction, after the account FOR SHARE and the order FOR UPDATE, and before the egg-lot lock;
  • the disabled predicate stays in the role read;
  • a null FlockId still means farm-wide access;
  • no read is cached or shared;
  • the unresolved-tenant guard and the tenant query filter read the same scoped TenantContext, and a Singleton registration fails closed;
  • the module never bypasses the IIdentityProvider registration.
• Not in this PR: the auth endpoints (PR C), the operator verbs and purge (PR D), and the contract declaration with the seeders (PR E).

[mforce]

Independent security review (Claude Opus 5.5, xhigh) at head 81ec39c0. Posted by the coordinator.

PR #1037 security review (Opus): head 81ec39c0 vs main a71ec7af

Reviewer: Opus 5.5. This is an independent review; I did not coordinate with Codex Astra.
Checkout: /home/mforce/.cluckwork-slices/review-1037-opus (detached at 81ec39c0). All mutations are reverted, and the tree was clean before removal.

Bottom line

• The PR has no product or security defect. I found no path where an authorization decision changes after this PR.
• Two findings, both P3. Both are hardening for the new port, and both already exist on main. Neither one blocks the merge.
• CI. Every check passes except Tests (integration), which was still pending at write time.

What I verified (adversarial pass)

Question	Result	Evidence
A restricted worker gains another user's rows or a farm-wide row	No	AccessLookup.ListFlockAssignmentsAsync filters a.UserId == userId over the tenant-filtered UserRoleAssignments. That is the same predicate and DbSet the middleware and guard queried directly on main. Mutant B5 reproduced red (below).
The role resolves differently	No	ReadEffectiveRoleAsync is a verbatim move: the active-membership AnyAsync with AccountId and DisabledAt == null, then the role join, then Roles.ResolveEffective. Roles binds to Cluckwork.Domain.Accounts.Roles, the only type of that name in src/. IdentityProvider.GetEffectiveRoleAsync passes its own scoped db into the same routine, so the two cannot diverge.
Cross-tenant read	No change	The assignment read still relies on the tenant query filter (AppDbContext.cs:114). The role read still takes accountId explicitly. The tenant-bypass TSV rows moved with an unchanged token hash 4adcdb84. Mutant O3 (IgnoreQueryFilters() on the lookup) turns TenantBypassRealTreeTests red.
Confirm reads outside its transaction or before the account lock	No	Both reads remain at steps 5 and 6 inside ExecuteInTransactionAsync, after GetCurrentSharedLockedAsync and GetByIdLockedAsync and before LockForSaleAsync, and both take transactionCt. Mutants B1 and B3 reproduced red.
Unresolved actor treated as account-wide (#787)	No change	FlockScopeGuard still returns Auth.Unauthorized before the lookup. In the middleware, an unresolved user still skips the read and gets Unrestricted reads, by design. The FlockScopeGuardTests unreachable-DB proof still constructs the real AccessLookup.
DI lifetime or AppDbContext instance	Unchanged	IAccessModule and IAccessLookup are Scoped (CluckworkIdentityServiceCollectionExtensions.cs:128-129), registered unconditionally for both process roles. ConfirmSaleHandler, FlockScopeGuard, IUnitOfWork, and all 11 forwarded handlers are Scoped, so every read shares the request's single AppDbContext and its transaction. The middleware is conventional, so its InvokeAsync parameters resolve per request, which avoids a captive dependency.
Dropped ORDER BY "Id" on confirm's assignment read	Proven unobservable	rows is used only for Count, All(FlockId is not null) and ToHashSet(). The set reaches Reservation.Plan as IReadOnlySet<Guid> and is used only in lockedLots.Where(l => fromFlocks.Contains(l.FlockId)). Draw order comes from lockedLots, which are ordered by (ProductionDate, Id), and from SaleAllocationPlanner, never from the set. The middleware and guard never had an ORDER BY. The remaining ListByUserAsync callers (AssignFlockHandler and the sim seeder) are untouched.
Users, me and assignment endpoints: step-up, idempotency, Version, audit, error mapping	Unchanged	The route mapping is untouched, and so are the endpoint bodies apart from the call target. AccessModule forwards to the same handler instances. I checked every forwarded argument order against the handler signatures: all are (command, accountId, actingUserId|userId, ct), and UpdateUser is (command, accountId, ct). GetUserAsync and ListUsersAsync forward to the DI-registered IIdentityProvider, so decorators stay in the path. Mutant O4 reproduced red.
Ledger, adapter-reach, seam rows	Accurate	Access→Farm gains IAccessLookup and AccessLookup, and the reason text matches the code (they return and compute Domain.Accounts.EffectiveAccountRole). The Commerce→Access reason now names the port. The GetMe and ListUsers adapter rows are real new reach, since both were Platform-only through IIdentityProvider before. FlockScopeGuard (Infrastructure.Repositories) and the middleware are Platform, so they need no row. The port returns an enum and records, with no EF or entity types. CI Tests (application) is green.
PR body overclaims	None found	I verified "four test lines", "verbatim" and "module never bypasses the IIdentityProvider registration" by reading the code. I did not re-run the trace claim ("only the ORDER BY and the parameter names differ"), but it matches the code.

Mutation runs (each reverted; one integration class at a time)

#	Mutant	Origin	Result
B5	AccessLookup: a.UserId == userId || true	PR	Red. AccessLookupTests ×2; the restricted worker saw [A, B], expected [A]
B3	Confirm reads assignments before ExecuteInTransactionAsync	PR	Red. ConfirmSaleAssignmentReadTests: Expected: OK, Actual: UnprocessableEntity
B1	Role routine drops DisabledAt == null	PR	Red. SaleAllocationPolicyTests.ConfirmSale_ParkedOnTheAccountLock_IsForbidden_WhenTheActorIsDisabledWhileItWaits: Expected: Forbidden, Actual: OK
O1	IAccessLookup registered as Singleton (captive root AppDbContext)	mine	Red. AccessLookupTests ×2: the port returned [] and the restricted worker saw both flocks. DI validation did not catch it, because the host built fine. ConfirmSaleAssignmentReadTests stayed green because its expected outcome (unrestricted) equals the fail-open outcome. See F1.
O2	Role routine drops u.AccountId == accountId	mine	Survived. SaleAllocationPolicyTests 13/13, RoleMatrixTests 18/18, ChangeUserRoleTests 25/25. See F2.
O3	Lookup adds IgnoreQueryFilters()	mine	Red, structurally only. TenantBypassRealTreeTests.RealSourceTree_AllBypassesAreAllowListed; AccessLookupTests stayed green, as expected, because user ids are globally unique.
O4	AccessModule.EnableUserAsync passes command.UserId as the actor	mine	Red. DisableUserTests ×7: Expected: NoContent, Actual: Forbidden

The baseline before mutation was AccessLookupTests + ConfirmSaleAssignmentReadTests, 4/4 green.

Findings

F1: P3, PLAUSIBLE (latent; pre-existing semantics, now a cross-module contract)

• Where: src/Cluckwork.Application/Features/Users/IAccessLookup.cs:13-14, src/Cluckwork.Infrastructure/Identity/AccessLookup.cs:16.

• Defect: ListFlockAssignmentsAsync takes no accountId. It scopes by the ambient tenant query filter, and under an unresolved TenantContext it returns zero rows. The port's own contract says zero rows means account-wide access, so a missing tenant fails open.

• Failure scenario: A future peer caller has a resolved Worker actor but has not resolved the tenant. Examples are a job, a seeder path, or the PR E callers. FlockScopeGuard checks only user.IsResolved, so it calls the lookup, gets [], and returns Success for an unassigned flock.

• Evidence: Mutant O1 reached exactly this state, because the root-scope context has an unresolved tenant. The port returned [] and a restricted worker became unrestricted.

• Not reachable today:

  • HTTP resolves the tenant together with the actor.
  • DemoDataSeeder.cs:135/146 and SimulationDataSeeder.cs:277 resolve the tenant before the actor.
  • Confirm fails earlier at the account lock.

  main's direct queries had the same shape, so this PR does not introduce it. It does publish the shape as a peer port.

• Suggested fix (small): Give the port an accountId parameter and filter a.AccountId == accountId explicitly, mirroring GetEffectiveRoleAsync, or throw in AccessLookup when the tenant is unresolved. Either can land here or in PR E, before the first peer caller.

F2: P3, CONFIRMED (test gap; pre-existing on main)

• Where: src/Cluckwork.Infrastructure/Identity/AccessLookup.cs:37 (u.AccountId == accountId).
• Defect: No test pins the account predicate in the shared effective-role routine.
• Failure scenario: The predicate is dropped, and a caller passes a userId from another farm. The routine then returns that user's role instead of null. Today every caller passes consistent ids: confirm derives both from the JWT, and AssignFlockHandler pre-checks with ListUsersAsync(accountId). So this is defence in depth, not an exploit. IAccessLookup.GetEffectiveRoleAsync now exposes the routine to peer modules, though.
• Evidence: Mutant O2 survived 56 tests across the three most relevant classes.
• Suggested fix: Add one case to AccessLookupTests: a farm-B user queried with farm A's id must return null.

Product or security defect in this PR: none. F1 and F2 are hardening for the newly published port, and both exist on main.

security-review skill output

• The skill ran against the wrong tree. Its injected context came from the coordinator's worktree (HEAD 05d7e7a7), not this checkout. That gave it an empty "FILES MODIFIED" and "DIFF CONTENT" and an unrelated commit list.
• I ran its method by hand. Its sub-task protocol also conflicts with this brief's no-subagent rule. I applied its categories and its false-positive filter to the real diff (git diff a71ec7af...HEAD: 19 files, +480/−74).
• Result: zero findings at confidence ≥ 8.
  • Authorization and privilege escalation: none; see the table above.
  • Injection: the only raw SQL is in a test fixture, and it uses ExecuteSqlInterpolatedAsync, which is parameterized.
  • Data exposure, crypto and secrets: none touched.
• F1 would score about 4/10 under the skill's filter (theoretical, no current attack path), so the skill would drop it. I keep it as a P3 hardening note.

Verdict

Merge once Tests (integration) is green. F1 and F2 are optional follow-ups, best done before PR E adds peer callers.

[mforce]

Review of record (Codex Astra, xhigh) at head 81ec39c0. Posted by the coordinator.

PR #1037 security review, #857 PR B. Reviewed 81ec39c029bee773e1f14ca9006de5737bd74c47 against a71ec7af on 2026-10-03.

No CONFIRMED or PLAUSIBLE P0–P3 findings. I found no product or security defect in this diff.

I compared all moved reads and all 14 endpoint forwards against the pinned baseline. The B-specific concerns from 857-design-astra.md are addressed: one effective-role routine, a lightweight lookup, the caller's existing context and transaction, the original joined assignment projection, and the registered identity-provider path.

Read	Parity evidence at the reviewed head
Confirm's effective role	src/Cluckwork.Application/Features/Sales/ConfirmSale/ConfirmSaleHandler.cs:167 calls IAccessLookup with the same account, actor and transaction cancellation token. src/Cluckwork.Infrastructure/Identity/AccessLookup.cs:36 retains Id == userId && AccountId == accountId && DisabledAt == null, the null result for inactive or absent membership, and the original role join.
IdentityProvider's effective role	src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1451 calls that same static routine with its existing db. The query and resolver body matches the baseline byte-for-byte after removing the old fully qualified Roles prefix. Roles.ResolveEffective retains empty roles as Worker, unknown-only roles as Denied, and known-role precedence.
Confirm's assignments	ConfirmSaleHandler.cs:205 replaces ListByUserAsync with the lookup at the same execution point. AccessLookup.cs:19 retains AsNoTracking, the exact user predicate, and every row, including null FlockId. It copies Id and FlockId into DTOs. Only the former OrderBy(Id) disappears.
Guard's assignments	src/Cluckwork.Infrastructure/Repositories/UserRoleAssignmentRepository.cs:74 still refuses an unresolved actor before any read. The non-Worker bypass remains before ListFlockAssignmentsAsync at line 81. The lookup reproduces the old inline query; zero rows and any null FlockId retain their unrestricted meaning.
Middleware's assignments	src/Cluckwork.Api/Middleware/FlockScopeResolutionMiddleware.cs:57 reproduces the old inline query through the lookup. Error re-execution, unresolved-user and non-Worker branches still skip it. The read-scope default remains separate from the write guard's #787 refusal.
Assignment display list	src/Cluckwork.Infrastructure/Identity/AccessModule.cs:65 forwards to the unchanged ListByNameByUserAsync. Its single LEFT JOIN, filtered flock names, null-name behavior and assignment ordering remain intact.

The assignment lookup uses the existing AccountId query filter at src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs:114. It introduces no IgnoreQueryFilters. Assignment rows remain deliberately exempt from flock filtering because they establish that scope. The Identity tables used by the role routine remain filter-free, with the original explicit active account-membership check and user-keyed role join. The two bypass-classification rows follow the routine without changing query hash 4adcdb84.

IAccessLookup and IAccessModule are Scoped. AccessLookup takes only the same Scoped AppDbContext used by UnitOfWork, the account repository and the order repository. No new scope, context factory, cache, transaction or retry appears. Confirm still enters ExecuteInTransactionAsync at line 108, locks the account at 113 and order at 142, reads the role at 167 and assignments at 205, then locks stock at 216. The recorded SQL independently shows that sequence inside the transaction. Assignment changes remain ordinary committed reads, with no new serialization guarantee.

The lost assignment sort is safe. Confirm consumes Count, All and a HashSet of flock IDs. EggStock.Reservation.Plan uses that set only through Contains, preserving the already ordered lot list. The guard uses Any; middleware produces scope membership. None takes the first assignment, emits assignment order in an error or audit payload, or uses it to order locks. The FIFO lot lock retains its own order. The administrative assignment list still uses its separate ordered joined query.

Every endpoint keeps its original command construction, validation, step-up token, account/actor IDs, cancellation token and result mapping. AccessModule.cs:35 through line 85 forwards create, update, password, role, email, disable, enable, assign, unassign, language and stepper operations to their original handlers; list-users and get-me to the injected IIdentityProvider; and assignment listing to the original repository query. The eight step-up-gated writes retain their proof-first handlers. /users remains Owner-only; /me retains default authenticated access. The handlers, provider mutation methods, concurrency handling, audit writers, idempotency middleware, step-up registry and middleware ordering are unchanged. The module does not instantiate a concrete identity provider, so registered decorators remain in the path.

I reproduced one 21-request trace on each revision using the supplied Scratch857BSqlTraceTests adaptation of the #854 harness. All statuses, normalized response bodies, command counts and transaction events match. The only SQL differences are six occurrences of @user_UserId becoming @userId, and removal of ORDER BY u."Id" from confirm's assignment read. These occur in four trace files. The parameter still receives the same resolved user's ID. Evidence: trace comparison, main trace, head trace. The harness records SQL text, not parameter values or latency; source comparison and the behavioral tests provide the remaining evidence.

I reproduced these mutants, rebuilding each and restoring it afterward:

Mutant	Observed result	Evidence
B3: read assignments before the account lock	New parked-confirm test fails: expected OK, actual UnprocessableEntity. Its unchanged body also passes on unmodified main.	B3 log, main baseline
B5: return every user's rows in the farm	Two of three AccessLookupTests fail. A restricted worker sees both flocks after inheriting another worker's farm-wide row.	B5 log
B4: discard farm-wide rows	Two of three AccessLookupTests fail, covering the returned rows and HTTP visibility.	B4 log
Additional: bypass assignment tenant filtering with IgnoreQueryFilters	All three lookup tests pass, but TenantBypassRealTreeTests.RealSourceTree_AllBypassesAreAllowListed fails with the exact unexcused bypass at AccessLookup.cs:19.	lookup log, guard log

The reported B5 coverage gap exists on main in the claimed cohort. I applied the equivalent mutation only to the three moved reads: middleware and guard lose their user predicates, and confirm calls ListAllAsync. All 172 pre-existing integration tests in the PR's nine existing classes pass, run one class at a time. I also backported the two new HTTP scenarios without the new interface-dependent test. Both pass on unmodified main; the restricted-worker scenario fails under B5, showing the actual authorization widening. This is a pre-existing test gap, not a defect in unmodified main or this head. Evidence: main mutant, backported probe, baseline, mutant failure. Individual cohort logs are alongside them.

The ledger changes are accurate. Access-to-Farm gains the interface returning EffectiveAccountRole and its implementation, taking the cell from 20 to 22 symbols. The cell remains W because of its existing writers; these additions read only. Commerce-to-Access remains R. GetMe and ListUsers now visibly reach Access, explaining 43 to 45 adapter rows. Access's final contract declaration remains deferred to PR E, as planned; the general public-interface persistence guard already checks these interfaces. Both expose completed DTOs/results rather than EF or Infrastructure types.

The fourth edited test line is mechanical. tests/Cluckwork.Api.IntegrationTests/TenantResolutionMiddlewareTests.cs:201 passes the same unreachable AppDbContext through a real AccessLookup. No assertion, connection setting or control-flow branch changes. Invalid tenant claims still must stop before the flock query; the owner and anonymous paths still prove no assignment I/O. The other three construction-line edits preserve the same proof method.

Final verification on the restored head passed 176/176 targeted integration tests and 13/13 Application checks. This includes all three edited test classes, the new tests, sale allocation, step-up, role/admin gates, me, joined projections, module ledger, adapter reach, interface persistence, generated matrix and tenant-bypass checks. See final check summary. I did not run the full integration suite. All temporary source changes and scratch tests were removed; the assigned checkout is clean at the reviewed head. No commit, push or GitHub comment was made.

Nits: none. The ponytail-review pass found no material simplification within the approved extraction. net: -0 lines possible.

Verdict: APPROVE PR #1037 at 81ec39c0; no blocking findings.

[mforce]

Review hardening: 4d31a9f5 (Opus F1, F2)

Opus's review (#1037 (comment)) and Astra's both approve with no product or security defect. Both P3 findings predate this PR, but the new port publishes the code, so they are fixed here.

Finding	Change	Evidence
F2 (CONFIRMED). No test pinned the account predicate in the shared effective-role query; O2 survived 56 tests.	New test AccessLookupTests.GetEffectiveRole_AnswersOnlyForTheAccountItIsAskedAbout. One email is a Manager in farm A and an Owner in farm B. Each farm must see its own user's role and null for the other farm's user.	O2 (drop u.AccountId == accountId): Assert.Null() Failure Expected: null Actual: Manager.
F1 (PLAUSIBLE, unreachable today). ListFlockAssignmentsAsync read no rows under an unresolved tenant, and no rows means account-wide access.	AccessLookup now throws InvalidOperationException before the query when db.Tenant is unresolved, following AuditWriter's precedent; the SQL is unchanged. AppDbContext exposes its TenantContext, as it already exposes FlockScope. New test ListFlockAssignments_RefusesAnUnresolvedTenant.	G1 (guard removed): Assert.Throws() Failure: No exception was thrown. O1 (your Singleton mutant, guard in place): every worker request now fails closed, with the guard's exception and Expected: OK Actual: InternalServerError across 31 of 74 tests. O1+G1 (your original): restricted workers become unrestricted (16 of 74 red, ScopedWorker_FlocksList_SeesOnlyAssignedFlock: Assert.DoesNotContain() Failure). That is the fail-open case the guard now prevents.

Caller check, done before guarding. Every caller of ListFlockAssignmentsAsync resolves the tenant first, so nothing legitimate trips the guard:

• HTTP: the tenant middleware resolves the tenant and the actor from the same token.
• Seeders: DemoDataSeeder and SimulationDataSeeder resolve the tenant before resolving any actor.
• Confirm: it reads the tenant's account first.
• CLI verbs and jobs: none call it.
• The two direct middleware tests: they skip the read before reaching the lookup (unresolved actor; /error re-execution).

Verification on 4d31a9f5:

• Application suite: 702 of 702.
• The touched integration classes plus the five AccessLookupTests and six seeder classes: 931 of 931.
• The SQL and response trace: identical to the previous head.
• After every mutant was reverted: 74 of 74, tree clean.

The PR body's commit list, mutation table and counts are updated.

[mforce]

Review of record, round 2 (Codex Astra, xhigh) at head 4d31a9f5. Posted by the coordinator.

PR #1037 security review, round 2. Reviewed the four-file delta from 81ec39c0 to 4d31a9f5b20e60d05b89a666271c5117531b1e7c on 2026-10-03.

No CONFIRMED or PLAUSIBLE P0–P3 findings. I found no product or security defect in this delta. The previous approval stands.

The new guard at src/Cluckwork.Infrastructure/Identity/AccessLookup.cs:22 fails closed before constructing or executing the assignment query. An unresolved tenant now faults the operation instead of producing the empty list that callers interpret as unrestricted access. No caller converts that exception into an authorization success. This deliberately changes invalid caller behavior; I found no legitimate production caller that requires the old behavior.

I independently traced all three assignment-lookup consumers and the four handlers that invoke FlockScopeGuard: record/submit daily entry and record feed/water usage.

Caller path	Tenant-resolution evidence
Web middleware and guarded writes	TenantResolutionMiddleware resolves the tenant before the actor and rejects malformed authenticated claims. Program.cs:333 runs it before flock-scope middleware at line 337. Anonymous requests and error re-execution skip the assignment read; the write guard still refuses an unresolved actor first.
Sale confirm	HTTP and seeder callers already resolve the tenant. ConfirmSaleHandler.cs:113 locks the account using that scoped tenant before its assignment read at line 205. The context, transaction and lock order are unchanged. The parked-confirm test passes with the new guard.
Demo seeder	DemoDataSeeder.cs:135 resolves the tenant, line 146 resolves the real actor, and only afterward does SeedDemoAsync invoke the daily-entry and confirm paths.
Simulation seeder	SimulationDataSeeder.cs:277 resolves the tenant before ActAs at line 297 and every subsequent guarded operation. Its separate second-account scope resolves that account at line 1937 and only inserts the account row.
CLI	No direct lookup consumer. The seed verb indirectly reaches it through the two seeders above. The other verbs call identity/account services or database maintenance, not confirm or the four guarded handlers.
Jobs	No lookup invocation. DailyEntryLockSweep resolves a per-account tenant and calls LockSubmittedEntriesAsync, whose handler does not use FlockScopeGuard. Refresh and idempotency purges also do not reach it.

Service construction before tenant resolution remains valid: the check is inside the lookup operation, not its constructor. This matters because the seed command resolves the seeder and its dependencies before the seeder resolves its tenant. The administrative assignment-list endpoint uses the separate IAccessModule method and is unchanged.

src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs:27 exposes the exact TenantContext instance already captured by EF's filters. This widens the context's public API by one getter, without widening an actor's authority. The returned object retains private setters and its single-assignment Resolve method; it cannot silently switch an already resolved account. Infrastructure code holding AppDbContext already has database access, and TenantContext was already publicly registered in DI. The new property's only production consumer reads IsResolved.

No persistence type crosses an Application interface. Both types involved in the new property belong to Platform, and Access already depends on this context. No ledger allowance, adapter permission, mapped entity or migration changes. The real module-ledger, interface-persistence and generated-matrix checks pass unchanged.

Both hardening tests passed on the clean head. I then rebuilt and reproduced these mutations, one class at a time:

Mutation	Observed failure	Evidence
F2: remove u.AccountId == accountId from the shared role routine	AccessLookupTests.GetEffectiveRole_AnswersOnlyForTheAccountItIsAskedAbout fails at line 97: expected null, actual Manager. The other four tests pass.	F2 log, exact mutation
F1: remove the unresolved-tenant guard	ListFlockAssignments_RefusesAnUnresolvedTenant fails at line 111: No exception was thrown. The other four tests pass.	F1 log, exact mutation

F2 correctly tests the explicit account argument. Its same-email users have different IDs and roles in two accounts. The positive lookup for account A while the ambient scope is B also prevents a replacement implementation from silently substituting the ambient tenant for that argument. This preserves the existing effective-role contract.

SQL parity is confirmed. I reran the 21-request trace and compared it with my independently captured 81ec39c0 trace from round 1. All 42 files are byte-identical: 21 SQL/status/transaction traces and 21 normalized response bodies. No additional normalization was needed. The guard adds no SQL on valid calls. Evidence: comparison, new trace.

After restoring both mutants, 27/27 targeted integration tests and 7/7 Application checks passed. These cover the five lookup cases, flock-scope middleware, tenant middleware, parked confirm, unresolved-actor refusal, interface persistence, ledger and matrix. See final summary. Seeder, CLI and job conclusions above come from source tracing; I did not rerun their suites or the full integration suite. Each test class ran alone after free -g showed at least 3 GB available; the recorded checks showed 8–10 GB.

All mutations were restored, the clean head was verified, and the assigned round-2 worktree was removed as requested. I did not touch the checkout used by the concurrent integration run. No commit, push or GitHub comment was made.

Nits: none. The ponytail-review pass found no material simplification needed. net: -0 lines possible.

Verdict: APPROVE PR #1037 at 4d31a9f5; both hardening points are addressed without a product or security regression.

[mforce]

Owner design fix, pushed as e628995a, then main (02e8a633, #1035 and #1036) merged in as 87a2eddf. No rebase and no force-push.

• The change. AccessLookup now takes the request's scoped TenantContext through its own constructor, next to AppDbContext. The AppDbContext.Tenant getter is gone, so AppDbContext matches main again. The FlockScope getter is untouched. The guard, its message and the SQL are unchanged, and the trace is identical to 4d31a9f5's.
• Unresolved tenant. Deleting the guard now fails the build with CS9113: Parameter 'tenant' is unread. A neutered guard that still reads tenant (G1n) turns AccessLookupTests.ListFlockAssignments_RefusesAnUnresolvedTenant red with Assert.Throws() Failure: No exception was thrown.
• Singleton (O1) still fails closed. The same 31 of 74 tests fail as with the getter. 27 get InternalServerError, 2 get the guard's InvalidOperationException, 1 reads an error body, and 1 confirm ends before it parks. None shows a widened flock set. The root provider builds the singleton from the root scope's AppDbContext and TenantContext, and that context's filter reads the same tenant. Nothing in the serving process resolves the root tenant, so the guard throws. With the guard neutered (O1+G1n), 15 fail open, which is what the guard prevents.
• Merge. B's diff against main is unchanged: the same 19 files, with the same content. On 87a2eddf the build has 0 warnings, the Application suite passes 702 of 702, and 946 of 946 pass across the touched classes, the six seeder classes and fix(auth): refuse a stored credential epoch below 1 #1036's CredentialEpochFloorTests.
• Load-sensitive seed test. Before the merge, one run timed out SeedCommand_Demo_SeedsDataAndExitsWithoutStartingKestrel at its 60-second limit, with a host load average of 16. Alone, SeedCommandTests passed 11 of 11, and it passed again in the 946-test run.

The mutation table and test counts in the PR body are updated.

[mforce]

Coverage note on the test counts above. SimulationCrossDayRerunTests.cs declares ten more simulation-seed test classes, from SimulationBaseDataAndOwnerBothMissingTests to SimulationSeedDefinitionChangeTests. My class-name filter missed them, so they were not in the 931 or 946 runs. They exercise the seeders' flock-scoped writes, which pass through FlockScopeGuard and the tenant guard. On 87a2eddf they pass 10 of 10. No other file in B's filter has an unlisted test class; the rest of the extra declarations are factories, interceptors and sinks.

[mforce]

Round-3 review (Claude Opus 5.5: the TenantContext fix, thermo-nuclear and ponytail) at head 87a2eddf. Posted by the coordinator.

PR #1037 review, round 3 (Opus): head 87a2eddf vs main

Reviewer: Opus 5.5, independent. I used no subagents.

• Checkout: /home/mforce/.cluckwork-slices/review-1037-opus, detached at 87a2eddf9f5d041f4dde5ffaaf1bfeac99f3e00c.
• Diff: the merge base is 02e8a633. git diff 02e8a633...HEAD is byte-identical to 1037-87a2eddf.diff (cmp clean): 19 files, +528/−74. origin/main is now 922b208d (test(arch): declare the Insights module contract in the ledger #1040), which touches no file in this PR's diff.
• Mutations: every mutation was reverted, and git status was clean before the worktree was removed.
• Evidence: condensed logs are in 1037-87a2eddf-opus-evidence/.

Bottom line

• The PR has no product or security defect. That covers the fix (e628995a) and the whole PR.
• The fix is correct. The guard is intact and the SQL is unchanged. The injected TenantContext is the same scoped instance the query filter reads. A Singleton-registered AccessLookup fails closed: I reproduced the exception and the HTTP 500.
• Seven P3 findings, all optional:
  • two test-hygiene notes on the fix;
  • three structural notes from the thermo-nuclear pass;
  • two stale comments.
  • None blocks the merge.
• Ponytail-review: about −55 lines possible. Most of that comes from one structural move (T1).
• CI at 87a2eddf: all checks pass, including Tests (integration). Publish is skipped, as expected on a PR.

Pass 1: the fix (e628995a)

Question	Answer	Evidence
Is the fail-closed guard intact?	Yes	AccessLookup.cs:23-26 is unchanged except for the receiver: it was db.Tenant.IsResolved and is now tenant.IsResolved. Mutant M4 (guard deleted) turns ListFlockAssignments_RefusesAnUnresolvedTenant red with Assert.Throws() Failure: No exception was thrown. The other 4 tests stay green.
Is the SQL unchanged?	Yes	git diff 4d31a9f5 HEAD -- AccessLookup.cs shows only the comment, the constructor and the guard's receiver. The LINQ in ListFlockAssignmentsAsync and ReadEffectiveRoleAsync is byte-identical, and the tenant-bypass TSV hash is still 4adcdb84. Round 2 had already shown SQL parity at 4d31a9f5 (Astra, 42/42 files byte-identical), and this commit changes no query.
Do the guard and the filter read the same TenantContext?	Yes, in DI	CluckworkPersistenceServiceCollectionExtensions.cs:20 uses AddScoped<TenantContext>(). AddDbContext<AppDbContext> (line 61) is Scoped by default. AppDbContext's only constructor is (DbContextOptions, TenantContext tenant, FlockScope) (AppDbContext.cs:18), and the filters close over that tenant (:68-94). IAccessLookup is Scoped (CluckworkIdentityServiceCollectionExtensions.cs:129). So AccessLookup's db and tenant come from one scope and share one instance. Nothing in src/ uses AddDbContextPool, AddPooledDbContextFactory or IDbContextFactory<AppDbContext>. The only hand-built AppDbContext instances in src/ are the design-time factory and the ExportQueries snapshot, and neither reaches AccessLookup. No root-provider TenantContext resolve exists in src/ or tests/.
Does a Singleton AccessLookup fail closed?	Yes, reproduced	Mutant M1 registers it with AddSingleton. Three AccessLookupTests go red. The direct call throws the guard's own InvalidOperationException ("Flock assignments require a resolved tenant…"). Both HTTP tests return InternalServerError instead of OK, so a restricted or farm-wide Worker gets a 500, not an unrestricted 200. ConfirmSaleAssignmentReadTests also goes red: the confirm completed before it could park (WaitUntilDoneOrBlockedAsync returns false only on completion), which matches the middleware's 500 before the handler runs. Contrast this with round 1's O1 at 81ec39c0, before the guard existed: the port returned [] and the Worker became unrestricted. The DI container does not catch the captive dependency, because the test host runs in Testing, where scope validation is off. The guard catches it instead.
Did the four test constructions keep their proofs?	Yes	See the mutation table. Three of them pass the same tenant instance that built db, so they behave exactly as db.Tenant did. FlockScopeGuardTests passes a second, equally unresolved instance; see R3-1.
Any stale reference to the removed getter?	None	git grep finds no db.Tenant, AppDbContext.Tenant or .Tenant.IsResolved anywhere. The build passes with 0 errors. TenantContext is in Infrastructure.Persistence, which is Platform, so no ledger row changes.

Mutation runs (each reverted; one test class at a time)

#	Mutant	Result
base	clean 87a2eddf	AccessLookupTests 5/5, FlockScopeMiddlewareTests 13/13, TenantResolutionMiddlewareTests 8/8, FlockScopeGuardTests 1/1
M1	IAccessLookup registered as Singleton	Red, fail-closed. AccessLookupTests 3 of 5: the guard exception ×1, InternalServerError ×2. ConfirmSaleAssignmentReadTests 1 of 1.
M4	unresolved-tenant guard deleted	Red. RefusesAnUnresolvedTenant: no exception thrown
G1	FlockScopeGuard drops if (!user.IsResolved) return Unauthorized	Red. FlockScopeGuardTests: InvalidOperationException from the lookup's guard
G2	middleware drops its !user.IsResolved branch	Red. UnresolvedLivenessRequest… and AnonymousHealthRequest…: the guard's InvalidOperationException. Four HTTP tests also fail, because anonymous login requests now reach the lookup.
G3	middleware drops the IExceptionHandlerFeature bypass	Red. ErrorReExecution…: Npgsql transient failure against the unreachable host

Findings

R3-1: P3, CONFIRMED (introduced by e628995a)

• Where: tests/Cluckwork.Application.Tests/FlockScoping/FlockScopeGuardTests.cs:17-18
• Defect: The test builds AppDbContext with one new TenantContext() and AccessLookup with another. Production can never produce that state, and the fix's correctness rests on exactly this instance identity: the guard and the filter must read the same object. The three integration tests share one tenant; this one does not.
• Scenario: Today it is harmless, because both instances are unresolved and G1 still goes red. Suppose someone later copies this construction and resolves only the lookup's tenant. The guard then passes while the filter's tenant stays unresolved. The query reads zero rows, and that means account-wide access: the fail-open the guard exists to prevent.
• Evidence: the source lines above; the DI trace in pass 1.
• Fix: var tenant = new TenantContext(); and pass it to both constructors.

R3-2: P3, CONFIRMED (stale since round 2, 4d31a9f5, same PR)

• Where: tests/Cluckwork.Api.IntegrationTests/FlockScopeMiddlewareTests.cs:144-145
• Defect: The comment says "Any UserRoleAssignments query attempts the unreachable connection and throws". Since the round-2 guard, an unresolved tenant throws InvalidOperationException before any query or connection. The test's conclusion still holds; the mechanism it names does not.
• Scenario: A reader debugging a red run looks for an Npgsql error and finds the tenant-guard message instead.
• Evidence: G2 output: System.InvalidOperationException : Flock assignments require a resolved tenant… for this exact test.
• Fix: "…reaches the lookup, which throws for the unresolved tenant (or, with a resolved one, at the unreachable connection)."

T1: P3, CONFIRMED (the triplication is pre-existing on main; the port that preserves it is new)

• Where:
  • src/Cluckwork.Api/Middleware/FlockScopeResolutionMiddleware.cs:57-77
  • src/Cluckwork.Infrastructure/Repositories/UserRoleAssignmentRepository.cs:81-91 (FlockScopeGuard)
  • src/Cluckwork.Application/Features/Sales/ConfirmSale/ConfirmSaleHandler.cs:202-208
  • src/Cluckwork.Application/Features/Users/IAccessLookup.cs:13-20
• Defect: All three callers re-derive Access's rule from raw rows: no rows, or any farm-wide row, means unrestricted, and otherwise the result is the set of flock ids. The port's own comment states this rule (IAccessLookup.cs:13), but the port does not apply it. Its record carries an Id that no caller reads.
• Scenario: The rule changes, for example a house-wide row starts to narrow access. Three files in two layers must change together, and a missed copy silently disagrees: a read scope and a write guard that answer differently for the same Worker. "Empty means everything" is also the semantic behind round 1's F1 fail-open. The rule belongs beside the tenant guard that protects it.
• Evidence: the three code sites. I checked that their outcomes are equivalent:
  • middleware: Count == 0 or Any(null) gives unrestricted, otherwise the id list;
  • guard: Count == 0 or Any(null || == flockId);
  • confirm: Count > 0 && All(not null) gives the set, otherwise null.
• Code-judo: Task<IReadOnlySet<Guid>?> GetAssignedFlocksAsync(Guid userId, …), where null means unrestricted.
  • middleware: scope.Resolve(assigned is null, assigned ?? []);
  • guard: assigned is null || assigned.Contains(flockId);
  • confirm: assignedFlockIds = role == Worker ? await … : null.
  • This deletes FlockAssignmentDetails, the three copies and T2's name clash, and leaves the SQL unchanged.
  • Caveat: FlockScope.Resolve's same-scope comparison uses Count, so duplicate flock rows would collapse. Check that duplicates cannot exist, or accept the change.
  • Doing it now or in PR C is the owner's call; it is not a blocker.

T2: P3, CONFIRMED (new)

• Where: src/Cluckwork.Application/Features/Users/IAccessModule.cs:44 vs IAccessLookup.cs:16

• Defect: Access's two new ports each declare ListFlockAssignmentsAsync(Guid userId, …) in the same namespace, with different contracts:

  • the admin one returns named UserFlockAssignment rows and relies on the endpoint's own tenant.IsResolved check;
  • the authorization one returns FlockAssignmentDetails and throws when the tenant is unresolved.

  The names are also inverted: FlockAssignmentDetails (Id, FlockId) carries fewer details than UserFlockAssignment (Id, FlockId, FlockName).

• Scenario: A peer module author picks the wrong port by name. If the author picks IAccessModule's method for an authorization decision, the result skips the unresolved-tenant guard. The ledger would flag a peer reaching IAccessModule, but only after the code is written.

• Evidence: both signatures; IUserRoleAssignmentRepository.cs:32.

• Fix: T1 removes the clash. Without T1, rename the lookup's method to say what it is for (for example ListAssignmentsForScopeAsync).

T3: P3, CONFIRMED (the member is pre-existing; the cross-class static is new)

• Where:
  • src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1455-1459
  • src/Cluckwork.Infrastructure/Identity/AccessLookup.cs:42
  • src/Cluckwork.Application/Common/IIdentityProvider.cs:196-203
• Defect: One routine has two public doors. IdentityProvider reaches into AccessLookup's internal static and passes its own db. IIdentityProvider.GetEffectiveRoleAsync has exactly one production caller, AssignFlockHandler.cs:42, which is itself in Access (Features.Users).
• Scenario: Two interfaces document the same contract in two comments (IIdentityProvider.cs:196-201 and IAccessLookup.cs:9), and they will drift. Test decorators must keep forwarding a member that only one handler uses (MustChangePasswordGateTests.cs:277, StepUpAuthTests.cs:713).
• Fix:
  • Point AssignFlockHandler at IAccessLookup.
  • Delete the IIdentityProvider member, the provider's forwarder and the two decorator forwarders.
  • Make ReadEffectiveRoleAsync a private instance method, and re-key the two TSV rows.
  • This fits [C] #514 slice 15: Access contract — security hold point, runs last #857 step 6 ("retire the exposed identity provider port"), so deferring it is reasonable.

T4: P3, CONFIRMED (stale comment, made stale by this PR; the file is not in the diff)

• Where: tests/Cluckwork.Api.IntegrationTests/SaleAllocationPolicyTests.cs:290
• Defect: The comment says "the fresh identity.GetEffectiveRoleAsync re-check". Confirm now calls access.GetEffectiveRoleAsync (ConfirmSaleHandler.cs:167).
• Scenario: Someone grepping for the confirm role read follows the comment to IIdentityProvider and edits the wrong door (T3).
• Fix: "the fresh IAccessLookup.GetEffectiveRoleAsync re-check".

Skill 1: pstack:thermo-nuclear-code-quality-review

The skill ran over the whole PR diff. I followed the brief's single-file output in place of the skill's report directory, so its findings are T1–T4 above.

• Structural regressions: none. The PR deletes EF code from the middleware and the guard. It also drops two injected dependencies from ConfirmSaleHandler (IUserRoleAssignmentRepository and IIdentityProvider) in favour of one port, and adds no branch to any flow.
• Missed code-judo: T1. The port froze the raw-row shape, so the rule stayed in three places. This is the only high-conviction structural item.
• Boundary and naming: T2 and T3. Two doors to one routine, and two methods with the same name but different contracts.
• File size: no file crosses 1k lines because of this PR. IdentityProvider.cs is 1964 lines, which predates the PR, and the PR shrinks it by 16. UserEndpoints.cs is 468 lines and ConfirmSaleHandler.cs is 340.
• Considered and not raised:
  • AccessModule as a 15-member 1:1 forwarder. [C] #514 slice 15: Access contract — security hold point, runs last #857 scope steps 1–2 specify "contracts around existing use cases, without rewriting Identity", so the facade is the deliverable and the ledger rows are its value. Its cost: a request to any one endpoint constructs all 11 handlers. That is unmeasured and almost certainly negligible next to UserManager, so it is not a finding.
  • AccessModule in Infrastructure although every dependency it has today is Application. This placement is justified by PR C. Moving it later costs one file move.

Skill 2: ponytail:ponytail-review

• IAccessLookup.cs:L20: delete: FlockAssignmentDetails.Id. No caller reads it; nothing replaces it. (New.)
• FlockScopeResolutionMiddleware.cs:L57-77, UserRoleAssignmentRepository.cs:L81-91, ConfirmSaleHandler.cs:L202-208: shrink: the assignment rule exists in three copies. Return IReadOnlySet<Guid>? from the port, which replaces about 38 lines with about 8 and deletes the record (T1). (The copies are pre-existing; the port is new.)
• IIdentityProvider.cs:L196-203, IdentityProvider.cs:L1455-1459, MustChangePasswordGateTests.cs:L277-279, StepUpAuthTests.cs:L713-715: yagni: an interface member with one caller, in the same module as IAccessLookup. AssignFlockHandler injects IAccessLookup instead, about −22 lines (T3). (The member is pre-existing.)
• ConfirmSaleAssignmentReadTests.cs:L23-34: reuse: SeedLotAsync copies SaleAllocationPolicyTests.cs:49's private helper. Add Guid? flockId = null to TestHarness.SeedEggLotAsync (Infrastructure/TestHarness.cs:276, which hardcodes Guid.NewGuid() for the flock) and delete both copies. (The copy is new; the original is pre-existing.)

net: about −55 lines possible.

Product or security defect

None, in the fix or anywhere in the PR. The guard fails closed under the misregistration the brief asked about. The SQL and the transaction placement are unchanged, and every authorization outcome I traced is identical to main. R3-1 through T4 concern maintainability and test hygiene only.

Verdict

APPROVE 87a2eddf. The owner's fix is correct and fails closed. The seven P3s are optional, and T1 is the only one worth doing before peer callers arrive.