Repository navigation
refactor(eggs): put egg operations behind an IEggOperationsModule contract - #1028
Conversation
The three FOR UPDATE reads over EggLots lock in (ProductionDate, Id) order so confirm, sale void and daily-entry void queue instead of deadlocking. Nothing failed when that order reversed: the full integration suite stayed green with GetByIdsLockedAsync and GetByDailyEntryLockedAsync ordered DESC. Each new test holds the later lot, parks the request on it, and checks the earlier lot is already held with FOR UPDATE NOWAIT. Reversing each order turns its test red.
…tract Endpoints, both seeders and the daily-entry lock sweep reach Egg Operations through IEggOperationsModule. SaleEndpoints reads grade names through IEggGradeLookup, and feed and water usage read daily-entry provenance through IDailyEntryLookup. The module only forwards to the existing handlers and repositories; the sweep's lock loop moved into LockDueDailyEntriesHandler unchanged. Commerce handlers are untouched: the stock seam is #854's. The ledger lists the contract, the four repositories as implementations, five seeder reads as compatibility exceptions for #858, and the sweep's new adapter row. Stale FK and edge reasons now name the ports.
EggOperationsModule copies entries, grades, lots and movements into positional records, where two Guids or two counts can swap without a compile error. Each test fails when two copied fields swap, when the lot-existence check before a movement read goes, when the active-grade list drops its farm, or when the daily-entry lookup swaps its key.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
CodeQL check: one alert, dismissed as a false positive (#131).
The same statement on |
|
Review of record: Codex PR #1028 reviewReviewed FindingP2 · CONFIRMED · The confirm and sale-void tests never exercise the Concrete failure scenario: changing either I built and ran both mutations independently. Each returned 3 passed, 0 failed. Evidence: FIFO mutation, FIFO result, by-id mutation, by-id result. Keep the different-date cases and add equal-date cases for both Commerce reads. Obtain canonical id order from PostgreSQL, and make the sale-void setup allocate from both lots. Each NitsP3 · CONFIRMED · The movement-copy test cannot detect a dropped Concrete failure scenario: replacing Verification
All 3,058 tests passed. Build, full run, counts for every suite listed in the PR. This includes the submit/adjust/void races and 409s, ledger balance, sold floors, mortality rollback, audit provenance including Every mutation compiled before its tests ran, and every source mutation was reverted:
Mutation runner and exact transformations, exit-code ledger. Individual build logs, test logs, diffs and TRX results are alongside them. Sweep and contractsThe sweep is behavior-equivalent except for the approved warning move. It retains 200 entries per account, The scheduled call remains behind The locking probes use separate connections, a held later row, observed All returned DTO fields match their previous response projections. The four implementation entries correctly name Matrix regeneration with After restoration, the solution rebuilt cleanly, all 669 Application tests passed with matrix regeneration, and the three lock-order plus two sweep tests passed. 7. CodeQLAlert #131 is a false positive for this change, consistent with #125. At Verdict: REQUEST CHANGES for the missing same-date tiebreaker tests; no product defect found. |
…ment time Astra round 1 on #1028. The confirm and sale-void lock-order tests used lots on different dates, so reversing only the Id part of their ORDER BY stayed green. Both now also run with two lots on one date, take the canonical order from PostgreSQL, and the sale void draws from both lots. Reversing the Id part of GetAvailableFifoLockedAsync, GetByIdsLockedAsync or GetByDailyEntryLockedAsync turns its test red. The module's movement copy test compared default timestamps on both sides, so a dropped CreatedAtUtc passed. The fixture now carries a stamped time.
|
Response to Astra round 1 (at P2: lock-order tests missed the
P3: movement copy could not see a dropped Per the shared-host constraint, I ran only the targeted suites locally (lock order 5, module 8) and left the rest to CI on |
|
Review of record: Codex PR #1028 review, round 2Reviewed the fix from FindingsNone. The new equal-date cases detect the missing tiebreaker mutations, and the movement-copy test detects a dropped timestamp. NitsP3 · CONFIRMED · The new wording incorrectly says PostgreSQL UUID ordering differs from the ordinary Concrete discrepancy: sorting the same UUID values with Keep the database-derived order, but describe it as using the database's canonical order and remove the claimed difference. The test itself orders correctly. Documentation/comment nit only; not a product defect and not blocking approval. Mutation verificationI independently rebuilt and reran all three requested mutations. Every build passed before its tests ran; all mutations were reverted.
The different-date cases stay green under both Evidence: FIFO result, by-id result, timestamp result, runner, exit-code ledger. Exact mutation diffs, build logs and TRX files are alongside them. Determinism and clean runsAll five The tests obtain The holder keeps an open transaction on the later lot. The HTTP request uses another connection, and Adjacent lock paths and scopeThe source census still finds three EggLot locked reads, all represented by the tests:
The tests do not establish that a multi-grade confirm takes one global locking statement rather than separate statements per grade. That is the acknowledged #854 test, outside #853's scope, and is not a finding against this PR. No additional EggLot locked-read implementation was found. The timestamp fixture now supplies a fixed non-default value through the private setter, and the expected response retains that literal value. This closes the previous false-green assertion without changing production timestamp ownership. 7. CodeQLThe fix touches no sweep or logging code. Round 1's conclusion on alert #131 remains unchanged: the logged value is the existing tenant GUID, and the move does not add sensitive content. No GitHub alert or review state was changed. All mutations are reverted, restored binaries were rebuilt, and the checkout is clean at Verdict: APPROVE WITH NIT; both prior findings are resolved, with no product defect found. |
The test and the decision record said PostgreSQL's uuid order differs from System.Guid's. Astra's round 2 notes they agree. The reason the test asks the database is that it owns the lock order.
|
Astra round 2 P3 (wording) is fixed in |
|
Second review (independent), Claude Opus 5.5 (xhigh), at head PR #1028 second review (Opus), head
|
| Mutation | Runs | Result |
|---|---|---|
M3: by-id read ORDER BY "ProductionDate" (drop Id) |
3 | 5/5 green every run. Also 51/51 green over EggLotConcurrencyTests, EggLedgerTests, VoidSaleTests, TenantScopedLockTests, SaleAllocationPolicyTests, FlockScopeTests and EggLotLockOrderTests (m3-related-suites.log). |
M1: by-id read, no ORDER BY |
3 | Red in runs 2 and 3 only, and only in VoidSale(daysApart: 5). The equal-date row is green every time (m1-run*.log). |
M2: FIFO read, no ORDER BY |
3 | A different row goes red each run. P(all green) is about 1/4 per run (m2-run*.log). |
M4: FIFO read ORDER BY "ProductionDate" |
3 | Same coin-flip pattern as M2 (m4-run*.log). |
The daily-entry read is PLAUSIBLY in the same position: its lots also have random Ids and are inserted in
grade-line order. I did not run that mutation.
Fix, cheapest first:
-
Narrow the AGENTS.md sentence to what holds: a reversed order, or a reversed
Idpart, turns the
matching test red. The decision record (853-egg-operations-contract.md:89) already says only that. -
Harden the seed:
- Give
SeedEggLotAsyncan optionallotId. - In the different-date row, give the earlier lot the larger Id (ask PostgreSQL to order two fresh
Guids) and insert it second. - In the equal-date row, insert the larger-Id lot first.
M1, M2 and M4 then fail every run.
- Give
-
M3 cannot be caught behaviourally while the PK plan returns Id order. If the order must be pinned
against a dropped key, add a plain text guard over the three SQL strings (ORDER BY "ProductionDate", "Id"directly beforeFOR UPDATE). The tenant-bypass allow-list keys these three reads by signature
(I checkedtenant-bypass-allowlist.json:118-130), so nothing else reads that text.
Checked and clean (no finding)
- Forwarding parity. I diffed every endpoint before and after. Every forwarded call keeps the same
arguments, cancellation token,Resultmapping and round trips. This includesListLotMovements, which
still does the lot check and then the movement read, and stops on an unknown lot.
SubmitDailyEntryAsync(id, accountId)forwards two Guids in the right order. - JSON parity, by reading. Every
ToResponseis unchanged except for its input type. Every*Details
field comes from the same entity property, andGradeskeeps its order. The records drop only fields no
response used:AccountId, the Standardize business-record timestamps and chronological list ordering #819 timestamps, and lotFlockId/Version. I did not diff live JSON. - Audit entity names. The audit entity-type constants (
nameof(DailyEntry),nameof(EggGrade)) give
the same strings as before. LockDueDailyEntriesHandleragainst the old loop:- The query is identical: Submitted,
Date < cutoff, ordered byDate,Take(200), tracked. - It does one batch save, through
UnitOfWork.SaveChangesAsync. That method only calls
db.SaveChangesAsync, so no new transaction or retry boundary (Add transient-DB resilience (EnableRetryOnFailure) on the request path #269). - A refused
Lockis collected and the rest still save. A concurrency exception still aborts the batch
and reaches the per-accountLogError. - Cancellation flows through unchanged.
- Still no audit row and no actor, the same as before (fix(seed): seeded audit events carry "(unresolved)" as the actor, now visible on record History columns #500/FlockScopeGuard's fail-open branch grants account-wide flock access to an unresolved actor #787).
- Leader gating (Background worker has no single-runner guarantee — double-runs if scaled >1 instance #271) is unchanged: the only caller is
DailyEntryLockSweep, still behind
DurableJobWorker's gate and the seeder drain.
- The query is identical: Submitted,
- DI. The module, the lock handler and both lookups are scoped. The sweep resolves the module from a
fresh scope per account, aftertenant.Resolve. Every handler the module constructs has only scoped,
side-effect-free constructor dependencies, so construction in a background scope cannot throw. The one
registration method serves both serving and one-shot verbs, so the seeders resolve. - Tenant and flock scope (Add discovery guard for flock-scoped EF query filters #613, FlockScopeGuard's fail-open branch grants account-wide flock access to an unresolved actor #787).
- Detail and list reads still go through the filtered
GetReadOnlyAsync/ListAsync. - The movement read is still gated by the filtered lot read.
EggGradeLookupuses the tenant-filteredGetDisplayNamesAsync.DailyEntryLookupwraps the same write-side bypass, still called afterIFlockScopeGuardand still
tracked.- The seeder's
AnyAsynchas the same predicate and the same filters asFindByNaturalKeyAsync(both
keep the filters on).
- Detail and list reads still go through the filtered
- Contract and reach guards. I tried leaks Astra did not:
- M5: generic leaks on the two peer lookups (
Task<IReadOnlyDictionary<Guid, IReadOnlyList<EggLot>>>,
Task<KeyValuePair<Guid, EggGrade>?>). Red, with the exact type path. - M7: a repository interface as a property of a contract record. Red, through
IRepository<EggLot, Guid>. - M6: adapter parameters
IEnumerable<IEggLotRepository>andFunc<IEggInventoryMovementRepository>.
Red as contract bypasses.
- M5: generic leaks on the two peer lookups (
- Ledger.
- The contract lists 26 types, which matches the record's breakdown.
- All five exception rows match the seeder code: tables,
IgnoreQueryFiltersbefore tenant
resolution, and child-first deletes. - All four rewritten reasons are accurate. No
FlockManagement → EggOperationsedge exists. - CI's matrix and compatibility tests are green.
- Rule duties. No migration, config key, singleton, hosted service or user-visible change. [C] #514 slice 12: Commerce contract, consuming the stock seam #854 and
[C] #514 slice 16: Platform composition, jobs, CLI and seeder conversion #858 carry amendments. CI is green on every check at this head.
Nits (P3, all CONFIRMED by reading)
-
IDailyEntryRepository.FindByNaturalKeyAsync(IDailyEntryRepository.cs:28,DailyEntryRepository.cs:30)
now has no caller insrc/. Its only implementer outside the repository is the new test fake, which
throws. Delete it. -
docs/decisions/855-inventory-contract.md:96still says the feed and water record handlers keep
IDailyEntryRepository. This PR moved them toIDailyEntryLookup. -
AGENTS.md:84says "adapters callIEggOperationsModule, and peers callIEggGradeLookupand
IDailyEntryLookup". ButSaleEndpoints, an adapter, callsIEggGradeLookup. The decision record
explains this; the canonical rule does not. -
IEggOperationsModule.LockSubmittedEntriesAsync(IEggOperationsModule.cs:40) is on the adapter
surface. It does a flock-scoped bulk write with noFlockScopeGuard, no actor and no audit. Its comment
does not say it is for the leader-gated sweep only, and an endpoint that called it would trip no guard.
(Approved design; a comment is enough.) -
IDailyEntryLookup.FindIdForFlockScopedWriteAsyncis now a peer-facing port.- It ignores both query filters and applies
AccountIdfrom its argument. - It applies no flock scope at all.
- Its safety rests on the caller having run
IFlockScopeGuard, and only a comment says so.
This shape existed before, on the repository.
- It ignores both query filters and applies
-
#853gets closed by this PR, but it carries no remainder note saying the stock seam moved to [C] #514 slice 12: Commerce contract, consuming the stock seam #854.
AGENTS asks for one: "record the remainder on the issue". The [C] #514 slice 12: Commerce contract, consuming the stock seam #854 amendment holds the substance, so
nothing is lost. -
docs/architecture.md:89-91names the sweep as where the lock transition lives. The loop body now lives
inLockDueDailyEntriesHandler.
Verification run
| Run | Result |
|---|---|
| Solution build at head (before and after mutations) | 0 warnings, 0 errors |
EggLotLockOrderTests, unchanged head (baseline) |
5/5 pass |
EggLotLockOrderTests, restored head |
5/5 pass |
Application EggOperationsModuleTests, ModuleContract*, AdapterReachRealTree*, CompatibilityException*, restored head |
55/55 pass |
CI on 1c974acb (gh pr checks) |
All checks pass; publish skipped as expected on a PR |
I did not run the full integration suite. Astra ran it at 3f2584fe, and CI ran it at this head.
Is any finding a product defect? No. F1 is a test and documentation defect; the production SQL is
correct today.
Verdict: fix first. The fix is test and doc only (at minimum, narrow the AGENTS.md lock-order claim); no
product defect.
Opus review of #1028. The lock-order seeds used random lot Ids in insertion order, so a dropped ORDER BY failed only by chance and a dropped Id tie-breaker on the sale-void read never failed. The seeds now insert the later lot first with fewer eggs, and on different dates give it the smaller Id (PostgreSQL decides the Id order), so heap, allocation-index and primary-key order disagree with the canonical order. The daily-entry test seeds its entry and lots directly to pick their Ids. Reversals, dropped tie-breakers and dropped ORDER BY clauses on the three reads now fail every run, except the sale-void read's tie-breaker: the confirm has already rewritten its lots in canonical order. EggLotLockSqlTests pins the ORDER BY text of every FOR UPDATE over EggLots, which covers it. Also: drop the dead FindByNaturalKeyAsync, comment the sweep-only and guard-first ports, and bring AGENTS.md, architecture.md and the #855 record up to date with the new ports.
|
Response to the Opus review (at F1 (P2): the lock-order pin caught a reversed order but not a dropped one. Fixed.
Every red row failed with
P3 nits, all seven fixed:
Local evidence is in
No further review round will be triggered on this PR. |
2 participants
Closes #853
Egg Operations now sits behind a contract: daily entries, grades, lots and the egg ledger. It is the fifth module after Finance (#849), Farm (#851), Flock Management (#852) and General Inventory (#855). No route, response, status, query, audit row or schema changes.
A real coverage gap on
main: nothing pinned the egg lot lock orderEvery
FOR UPDATEread overEggLotsorders by(ProductionDate, Id), so a confirm, a sale void and a daily-entry void that touch the same lots queue instead of deadlocking. No test noticed when that order changed. Onmainat54a5f298, with bothGetByIdsLockedAsyncandGetByDailyEntryLockedAsyncreversed toORDER BY "ProductionDate" DESC, "Id" DESC, all 1881 integration tests passed, and so did the 42 tenant-bypass tests (the allow-list keys raw SQL by signature, not by SQL text).The first commit adds
EggLotLockOrderTests, before any structure moves. Each test holds the canonically later lot in a second connection and waits until the request is parked on it (pg_blocking_pids). It then tries the earlier lot withFOR UPDATE NOWAIT, and that try must fail, because the request already holds that lot. Each reversed order turns its test red, with the messagethe request must already hold the earlier lot while it waits on the later one:GetByIdsLockedAsyncVoidSale_HoldsTheOlderSourceLotWhileWaitingOnTheNewerGetByDailyEntryLockedAsyncVoidDailyEntry_HoldsTheEarlierLotWhileWaitingOnTheLaterGetAvailableFifoLockedAsyncConfirmSale_HoldsTheOlderLotWhileWaitingOnTheNewer(andVoidSale_…, because its setup's confirm then draws from one lot only)Each mutation was reverted and the binaries rebuilt. The pin passed 5 of 5 runs on the unchanged code.
Checkpoint decisions (owner-approved)
IEggOperationsModule. Peers callIEggGradeLookup(grade names) andIDailyEntryLookup(the day's entry id). Injecting the module would construct nine handlers per request to use none.IEggOperationsModulecovers entries (record, submit, adjust, void, get, list), grades (list, get, create, update, activate, deactivate), stock by grade, lots, lot movements, recording a lot movement, and the sweep's lock pass.EggOperationsModuleonly forwards to the existing handlers and four repositories. Each lookup forwards to one repository method. No handler body changes.ConfirmSaleHandler,VoidSaleHandler,CreateProductHandler,UpdateProductHandlerandAddOrderItemHandlerstill call the Egg Operations repositories. The decision record lists the properties [C] #514 slice 12: Commerce contract, consuming the stock seam #854's seam must keep: one confirm-timeFOR UPDATE, two planning passes over the same locked rows usingFlockId,AllocateandRestoreon the locked instances, Sale movements referencing Commerce's allocation id, Void movements referencing the order, and no save. They are also posted on [C] #514 slice 12: Commerce contract, consuming the stock seam #854.LockDueDailyEntriesHandler. The sweep still picks the account, the cutoff and the batch size, and logs the result. One log line moved. The "LockDailyEntry failed" warning now prints after the save instead of before it. That branch cannot run: the query selects only Submitted entries, andLockrefuses only non-Submitted ones.IDailyEntryLookup. [C] #514 slice 13: General Inventory contract and the flock-eligibility read #855 left this port to this slice. The query and tracking are unchanged, because the lookup calls the sameFindByNaturalKeyForFlockScopedWriteAsync.db.DailyEntries.AnyAsyncwith the identical predicate. It gives the same answer and no longer tracks the entry.FK_BirdMovements_DailyEntries_DailyEntryIdstill saidIBirdMovementRepository, which is stale since [C] #514 slice 10: Flock Management contract, eligibility and mortality ports #852; it now saysIMortalityLedgerand that the FK is schema-only. The feed and water FK reasons and the GeneralInventory → EggOperations edge reason now nameIDailyEntryLookup.DailyEntry.Submitsince Show "created by / when" and "last changed by / when" inline on record pages #494, pinned byAuditProvenanceTests.DailyEntrySubmit_WritesAnAuditEvent. The invariant held here is that every handler writes the rows it wrote before.Rationale:
docs/decisions/853-egg-operations-contract.md. AGENTS.md's #849 paragraph now names Egg Operations and the lock-order rule.Reach
main54a5f29, placeholder contract)Platform -> EggOperations)DailyEntryLockSweep.LockDueEntriesAsyncimplementationscompatibilityExceptionsrows (Platform,deleteWhen#858)No edges were added or removed. The adapter pruning list (
Loosenable) is empty. The regeneratedcoupling-matrix.mdchanges only the A (24) → A (25) cell.Change map
Verification
Test counts were measured here, not copied from elsewhere. All suites pass, and no existing test file was edited: the only test-file changes are two new files and the ledger data.
main54a5f29EggOperationsModuleTests)EggLotLockOrderTests)dotnet build Cluckwork.slnis clean with warnings as errors. The branch base is54a5f298.mainhas since gained #1025, which adds two test files and overlaps nothing here; CI runs on the merge ref.EF model digest. The SHA-256 of the design-time
Model.ToDebugString(LongDefault)is45F550E426163CEBEFF36E509CFC6E53B36D26D3D89C72E0F728855BCC8FB9AE, onmain54a5f29 and on this head. The two 1486-line dumps are byte-identical.Invariants. Counts below are from the full integration run at
3f2584fe; every test passed and none was edited.SubmitDailyEntryTests(13, incl.ParallelSubmits_ExactlyOneWins_NoDuplicateLots),DailyEntryAdjustTests(13, incl.ParallelAdjusts_SameBaseVersion_ExactlyOneWins,ParallelAdjustAndVoid_SameBaseVersion_ExactlyOneWins),EggLotWriteOffTests(17, incl.TwoRacingWriteOffs_OneWins_OneRefused_LedgerBalances),EggGradeManagementTests(6),EggGradeFloorRaceTests(1)(ProductionDate, Id)EggLotLockOrderTests(3, new),EggLotConcurrencyTests(2),VoidSaleTests(7, incl.VoidRacingConfirm_OnSameLots_NeverLosesOrDuplicatesStock)EggLedgerTests(6, incl.Ledger_RebuildsTheCachedBalance_AcrossTheFullLifecycle),EggLotWriteOffTestsDailyEntryAdjustTests(Adjust_ShrinkBelowSold_Is422_AndNothingChanges,Void_WithSoldStock_Is422),EggLotWriteOffTests(reconcile limits)QualityEggTests(12),WithdrawalRestrictedLotTests(1)MortalityLedgerAtomicityTests(2),AuditTests(8, incl.MidTransactionFailure_WritesNothing_EventualVoidWritesOne),EggLedgerTests.FailedOperations_WriteNoMovements,SubmitDailyEntryTests.Submit_PartialGrades_Fails_NoLotsCreatedAuditProvenanceTests(48, incl.DailyEntrySubmit_WritesAnAuditEvent),AuditTests(8),EggGradeManagementTests.Grade_Create_WritesAuditEvent,EggLotWriteOffTests.WriteOff_WritesAuditRowFlockScopeTests(16),TenantIsolationTests(2),TwoFarmIsolationMatrixTests(1),ReadEndpointTests(13),RoleMatrixTests(18),AdminGatingTests(7),EggGradeLowStockFloorTests(9),SalesProductTests(25)DailyEntryAdjustTests.LockSweep_*(2),SimulationDeepSeedDrainTests(2)FeedUsageTests(13),WaterUsageTests(7),FeedUsageLockOrderTests(2)DemoSeedTests(2),SimulationSeederTests(39)Mutations. Each was run against a clean build and reverted afterwards, and every one turned red:
FarmId↔HouseIdin the entry copyEggOperationsModuleTests.GetDailyEntry_…,ListDailyEntries_…CrackedGradeId↔DirtyGradeIdin the entry copyQuantityProduced↔QuantityAvailablein the lot copyListLots_ForwardsTheFiltersAndCopiesEveryFieldListLotMovements_CopiesEveryField_UnknownLotIsNullWithoutAMovementReadGrades_CopyEveryField_AndForwardTheFarmIDailyEntryLookupswaps farm and houseDailyEntryLookup_ForwardsTheNaturalKeyAndReturnsTheIdIDailyEntryLookupalways returns nullFeedUsageTests.Usage_WithExistingDailyEntry_StampsTheLink,WaterUsageTests.Record_WithExistingDailyEntry_StampsTheLink_AndUpdateKeepsItIEggGradeLookupreturns no namesSalesProductTests.OrderItems_ReadCurrentGradeName_EvenWhenInactive(both cases)LockDueDailyEntriesHandlerwithout its saveDailyEntryAdjustTests.LockSweep_LocksOldSubmittedEntries_…,LockSweep_StillLocksEntries_ForASuspendedFarmSimulationSeederTests.SimulationSeed_ProductionHistory_IsIdempotent_NoDuplicateEntriesAfterTwoHostStartsIEggLotRepository eggLotsadded back toStockEndpoints.ListLotsAdapterReachRealTreeTests:contract bypass … ListLots -> EggOperations through …IEggLotRepositoryEggLotonIEggOperationsModuleModuleContractRealAssemblyTestsEggLotRepositoryremoved fromimplementationsCompatibilityExceptionRealTreeTests: undeclaredEggLotRepository.*readsCompatibilityExceptionRealTreeTests: undeclaredSeedFlockHistoryAsyncreadAdapterReachRealTreeTests: undeclaredLockDueEntriesAsync -> EggOperationsIn the first attempt, two mutations (the sweep without its save, and the empty grade lookup) did not compile because a parameter became unread, so the tests ran against stale binaries and passed. I reran both with mutations that compile, and both turned red. The mutation script now refuses to run tests after a build error.
Cleanup.
pstack:deslopran on the branch diff before opening. It removed:Building with
IDE0005enabled then found three redundantusing Cluckwork.Domain.Common;lines in new Application files, where the global using already covers them. Those were removed too. A/noslopreview of the full diff found nothing more to remove.Nothing user-visible changes, so this PR has no screenshots.