Repository navigation
refactor(sales): put Commerce behind a contract and reach egg stock through IEggStock - #1030
Conversation
Confirm and void reach egg lots through Egg Operations' IEggStock port. LockForSaleAsync runs the same single FOR UPDATE; the reservation plans over the locked lots (the FIFO planner moves to Egg Operations) and draws one planned quantity with its Sale movement. RestoreAsync locks, restores and writes the Void movements. The port never saves. Commerce's grade reads go through IEggGradeLookup.GetAsync, which runs the same GetByIdAsync query. EggLotLockOrderTests gains a two-grade confirm case: before it, locking per grade in line order left every test green. Refs #854
… amounts Farm's CurrencyBoundRowProbe no longer reads Commerce, Finance and General Inventory tables itself. Each repository that owns a probed table implements Farm's ICurrencyBoundRowSource with the probe's query for that table, and the probe asks them in registration order, which is the order it read them in before. Both of the probe's compatibility-exception rows (deleteWhen #854) are gone. Refs #854
Product, customer, sale and payment endpoints and both seeders reach Commerce only through ICommerceModule, which forwards to the existing handlers and repositories and copies reads into *Details records. Farm's settings handler and Access's stepper-unit handler read packed-unit conversions through the new IEggUnitConversionLookup port. The ledger lists the 34 contract types and six repository implementations. The eight seeder reads of Commerce tables are compatibility exceptions for #858. Refs #854
…g commit CommerceModuleTests pins every Details copy field by field. Swapping two same-typed fields in the customer, order-line or payment copy turns it red. ConfirmSaleAtomicityTests faults a confirm after a lot allocation, after its Sale movement, after the allocation rows and after the order is confirmed, and finds nothing persisted. Running confirm without its transaction while saving after each draw turns all four red. Refs #854
|
Independent review: Claude Opus 5.5 (xhigh), at head PR #1030 review: Commerce contract and the
|
| Check | Result |
|---|---|
Build (the integration and application test projects, which build all of src/, warnings as errors, #985 style gate) |
clean |
Clean head: EggLotLockOrderTests, ConfirmSaleAtomicityTests, VoidSaleTests, EggLotConcurrencyTests, SaleAllocationPolicyTests |
32/32 green |
Clean head: Application Architecture (ledger, matrix regeneration, adapter reach, contract walk, compatibility exceptions), CommerceModuleTests, SaleAllocationPlannerTests, EggOperationsModuleTests |
359/359 green |
| Two-grade lock test, repeated on the clean head | 5/5 green; deterministic, because LockRows sits above Sort and the holder never commits before the assertion |
Mutation A (PR's): LockForSaleAsync locks each grade in line order; the reservation is kept sorted canonically, so only the lock order differs |
two-grade case red, other 5 lock-order cases green |
| Mutation B (PR's): save after each draw, transaction kept | atomicity 4/4 green (matches the PR) |
| Mutation C (mine, against the F1 prototype): no handler transaction, one save at the end | prototype 4/4 red; original test green per the PR |
Mutation D (mine): ConfirmSaleHandler re-injects IEggLotRepository and plans over EggLot |
Architecture 338/338 green (F2) |
AllocationSourceMissing claimed unreachable |
holds: FK_SalesOrderAllocations_EggLots_EggLotId ... ON DELETE RESTRICT, no caller of EggLotRepository.Remove, void is AdminOnly, and Owner and Manager are unrestricted in FlockScope |
Not reproduced: the 54-file SQL trace. The script is not committed. My reading agrees with it: the lock SQL is untouched, the probe queries are verbatim, and no tracked entity changes order.
All mutations are reverted. The worktree was clean before removal.
Product defects: none. Findings: 4 × P3, all docs or tests.
Verdict: merge. F1–F4 are cheap follow-ups, and F3 and F2 are one-line text fixes that could ride along.
|
Review of record: Codex PR #1030 review by Astra Reviewed One confirmed P2 contract defect. No product defect was demonstrated in the current routes or seeders.
The confirm/void comparison found no other behavioral change:
Independent mutation results, each compiled and reverted:
The four-point atomicity test is meaningful as a check for partial persistence under those faults. It is not an independent guard for the explicit transaction or the no-save rule. All four injected faults occur before the normal final save, so the unmodified test does not exercise rollback of already-issued business writes. The combined mutation supplies those writes and proves the assertions detect them. The PR describes this limitation accurately; I do not report it as a separate defect. Detecting an added seam save depends on the reviewed code and the separately rerun SQL trace. The currency probe preserves the seven predicates, their order and short-circuit behavior. A real DI resolution returned SalesOrder, Payment, Expense, Product, InventoryLot, FeedUsage and InventoryItem repositories in that order. These explicit registrations provide deterministic enumeration. The empty-farm trace runs all seven reads; the expense-farm trace stops after the third. Both The contract and ledger changes match the code: six Commerce implementations, eight new seeder exceptions assigned to #858, and the Commerce-to-EggOperations edge reduced from eight symbols to five because the planner types moved. No I reran the PR's throwaway trace code on both commits in the assigned checkout. All four confirm/void scenarios matched byte-for-byte, as did the 24 route scenarios' SQL and normalized bodies. Across the complete output, 52 files were literally identical. The other two differed only in generated farm IDs in currency-probe response headers; their SQL and normalized headers matched. The recorder omits SQL parameter values and normalizes response IDs/timestamps, so this proves the recorded query shapes and normalized examples, not every possible input. Verification: the final solution build passed with zero warnings/errors, including #985's style gate. All 132 selected existing integration cases, 29 selected Application cases, three additional diagnostic cases, and the three trace cases on each commit passed. Application guards passed again after mutation restoration. No full integration suite was run. I did not independently rerun the SPA suite or EF-model digest. The checkout is clean and restored to the reviewed head; no commits, pushes or GitHub comments were made. Evidence: Nits: none. Verdict: request changes for the reservation lifetime contract; no current product regression demonstrated. |
…cked it IEggStock refuses to run without a transaction, and a reservation remembers the transaction that locked its lots and refuses to plan or draw once another transaction, or none, is current. It reads the transaction id through a new Platform port, ICurrentTransaction, so no persistence type reaches Application. EggStockTransactionTests keeps a reservation past its commit and requires both refusals. ConfirmSaleAtomicityTests now checks, at each fault, that the confirm is still in a transaction and that no draw was saved yet, so each rule fails on its own. IEggStockReservation.Plan's comment no longer calls it pure. Refs #854
…ts contract PeerContractRealTreeTests walks Commerce's namespaces as adapter roots with the existing adapter scanner and fails on any Egg Operations type outside owners.EggOperations.contract. It is the first peer check #1023 asks for. Re-adding IEggLotRepository to ConfirmSaleHandler turns it red while every other architecture test stays green. Refs #854, #1023
…round The Farm -> Commerce and Access -> Commerce reasons, and three Commerce foreign-key reasons, still named repositories the handlers no longer take. The decision record and AGENTS.md describe the transaction binding, the peer check and the stricter atomicity test. Refs #854
|
Review round 1, fixed at
Behaviour is unchanged. I re-ran the SQL-trace script on Tests run locally on
Not done, on purpose. I left Opus's nits as they are: duplicate grade-read bodies, |
|
Review of record: Codex PR #1030, round 2, Astra review Reviewed The original P2 escape is fixed. One further P2 contract defect is confirmed below. No current product defect or legitimate-confirm regression was demonstrated.
The original escape now fails for the intended reason. I reran the exact round-one diagnostic, which previously planned and persisted a draw after commit. It now throws The additional binding probes established:
The normal confirm transaction is not automatically replayed. The revised atomicity tests now catch each single regression. An added The peer guard works within its stated scope. Independent mutations adding No persistence type crosses the actual Confirm and void handler bodies are unchanged by this round. The new transaction checks issue no SQL. I reran the three trace cases on this head and compared all 54 outputs against my independently recorded Verification: 51 targeted integration cases passed, including transaction, atomicity, lock-order, allocation-policy, confirm/void race and retry suites. Ten targeted Application guards passed. Five additional binding diagnostics passed, one deliberately demonstrating the savepoint defect. The old escape diagnostic failed as expected at the new refusal; the other two round-one diagnostics and all three trace cases passed. After restoring mutations, the ten Application guards and seven transaction/atomicity cases passed again. The final solution build had zero warnings and errors. No full integration suite was run. Evidence: Nits: none. Verdict: request changes for savepoint invalidation; the original escape is fixed and no current product regression was demonstrated. |
|
Review round 2, documented at The savepoint edge is real. Rolling back to a savepoint taken before
Both say that nothing in Checks on The review loop was stopped deliberately by the owner after two consecutive rounds that found no product defect (rounds 1 and 2 found only seam-contract and test gaps). No round 3 will be triggered. |
|
Independent review: Claude Fable 5.1 (xhigh), at head PR #1030 review: Commerce contract and the
|
| Check | Result |
|---|---|
| Build: Integration and Application test projects, warnings as errors (#985 style gate) | clean, 0 warnings |
Clean head: EggStockTransactionTests (3), ConfirmSaleAtomicityTests (4), EggLotLockOrderTests (6) |
13/13 green |
Clean head: PeerContractRealTreeTests, SaleAllocationPlannerTests, CommerceModuleTests |
13/13 green |
| Mutation A (new): planner stamps every draw with the first line's id | 95/95 green (F1) |
Mutation B (new): IEggLotRepository re-injected into ConfirmSaleHandler behind a using alias (using Lots = …IEggLotRepository; Lots lots) |
PeerContractRealTreeTests red: ConfirmSaleHandler.ctor through Cluckwork.Application.Features.EggLots.IEggLotRepository at …/ConfirmSaleHandler.cs:19. The scanner expands aliases. |
Mutation C (new): EnsureLocked weakened from transaction.Id != lockedIn to transaction.Id is null |
EggStockTransactionTests 1 red / 2 green: Reservation_InsideALaterTransaction_RefusesToPlanOrDraw. The test pins the transaction's identity, not its presence. (A first attempt left lockedIn unread and failed the build on CS9113; the run was repeated with the parameter consumed.) |
Mutation D (new): confirm's audit.WriteAsync moved after ExecuteInTransactionAsync |
ConfirmSaleAtomicityTests 1 red / 3 green: AfterOrderConfirmation, at the test's own "runs outside a transaction" check. #494's same-transaction rule is enforced. |
All four mutations were reverted; git status --short printed nothing before the worktree was removed.
Not reproduced: the 54-file SQL trace (script not committed; Astra re-ran it twice), the two-grade lock mutation (run by the PR, Opus and Astra, all red), the EF model digest.
Summary
- Product defects: none. The seam is a faithful extraction; every behaviour difference I looked for reduced to the same SQL, the same tracked instances and the same change-tracker order.
- Findings: 2 × P3. F1 is a real coverage hole exposed by this PR's own rename (CONFIRMED by a 95-test-green mutation); F2 is a one-word doc drift.
- Earlier reviews. I agree with Opus and both Astra rounds on the handler equivalence. The reservation-lifetime fixes hold under a weaker-check mutation they did not run.
Verdict: merge. F1 is a two-line test addition that could ride along or follow.
SaleAllocationLineTests confirms a two-line order and requires each allocation row to name the line whose grade it drew; nothing pinned it, and the admin export carries it. AGENTS.md and the decision record now say the peer check reads parameter types and service resolutions, not return types or method bodies. Refs #854
|
Fable review, both items fixed at
Checks: the solution builds clean. The new test passes, as do the documentation guards (20) and |
…h-verifier No conflicts. The regenerated coupling matrix equals the auto-merged one.
Closes #854
Commerce now sits behind a contract: products, conversions, customers, sales orders, allocations and payments. It is the sixth module after Finance (#849), Farm (#851), Flock Management (#852), General Inventory (#855) and Egg Operations (#853). Confirm and void reach egg lots only through a new Egg Operations port,
IEggStock, and Commerce no longer seesEggLot. No route, response, status, query, lock, audit row or schema changes. A trace of every SQL statement and transaction event proves that for 30 request scenarios (below).A real coverage gap on
main: nothing pinned the lock order across gradesIEggLotRepositorywarns that locking each grade's lots in order-line order deadlocks against a void, so a confirm takes one farm-wideFOR UPDATEover every grade on the order. Onmainthat shape was pinned by a comment only:EggLotLockOrderTestsuses one grade. With the confirm changed to lock per grade, 156 tests across 15 classes stayed green on the design spike at #1028's head, every lock-order case included.This PR adds
EggLotLockOrderTests.ConfirmSale_TwoGrades_HoldsTheOtherGradesEarlierLotWhileWaitingOnTheLater. The order's first line is a grade whose only lot is canonically later. The test holds that lot and requires the request to already hold the other grade's earlier lot. Under per-grade locking it fails 3 of 3 runs withthe request must already hold the earlier lot while it waits on the later one; the other five cases stay green. The order is built through the API, one request per line, so the lines keep their order. A precondition asserts that order, and it caught an earlier seed that saved both lines in one batch and got them back in either order.Checkpoint decisions (owner-approved)
ICommerceModulefor adapters, one narrow port for peers ([C] #514 slice 10: Flock Management contract, eligibility and mortality ports #852's rule). The product, customer, sale and payment endpoints and both seeders callICommerceModule. Farm'sUpdateFarmSettingsHandlerand Access'sSetStepperUnitHandlerread packed-unit conversions throughIEggUnitConversionLookup, which forwards to the sameGetByUnitAsync.CommerceModuleonly forwards to the existing handlers and five repositories; no handler body changes except confirm and void below.IEggStock.LockForSaleAsyncruns the same singleFOR UPDATEand returns anIEggStockReservation. ItsPlanis the FIFO planner, moved from Commerce, over the locked lots; it is pure, so the Configurable Worker sales allocation scope by flock assignment #612 assigned-flocks-first retry plans twice over the same rows with no second lock.DrawAsynccallsEggLot.Allocateon the locked instance (a refusal throws) and writes the Sale movement.RestoreAsyncis the void's lock,AllocationSourceMissingcheck,Restoreand Void movement. The port never saves, likeIMortalityLedger. Commerce keeps the order lock, the Configurable Worker sales allocation scope by flock assignment #612 policy and refusals, the allocation rows and their ids, and the audit write. Movement reference types are passed in, so Egg Operations names no Commerce type.SalesOrderAllocationis created beforeAllocateruns, because the Sale movement needs its id. Nothing reaches the database before the single save, and EF orders that batch itself; the SQL trace is byte-identical.CreateProductHandler,UpdateProductHandler,AddOrderItemHandlerand confirm's two refusal names read throughIEggGradeLookup.GetAsync, which runs the same trackedGetByIdAsync. The grade copy moved intoEggGradeLookupso the module and the lookup share one.deleteWhen #854rows are deleted. Farm ownsICurrencyBoundRowSource. The seven repositories that own a probed table implement it with exactly the probe's query for that table, registered in the order the probe read them.CurrencyBoundRowProbemoved into Farm and asks the sources in that order, stopping at the first yes. A read on each module's contract was rejected: Farm would resolveICommerceModuleand its handlers on everyIFarmModuleuse, and gain edges that close cycles with Finance → Farm and General Inventory → Farm.Rationale:
docs/decisions/854-commerce-contract.md. AGENTS.md's #849 paragraph now names Commerce and theIEggStockrule;docs/architecture.md's egg loop names the port.Reach
maind6dbd47, placeholder contract)implementationscompatibilityExceptionsrows (Platform,deleteWhen#858)CurrencyBoundRowProberows (deleteWhen#854)compatibilityExceptionsrows in totalCommerce -> EggOperationsedgeowners.EggOperations.contractIEggStock,IEggStockReservation,SaleDemandLine,PlannedEggLotDraw,SaleAllocationPlan)owners.Commerce.contractThe regenerated
coupling-matrix.mdchanges only theCommerce -> EggOperationscell,W (8)→W (5). No edge was added or removed.Change map
Verification
Counts were measured on this machine, before and after.
maind6dbd47CommerceModuleTests, +1GradeLookup_GetCopiesEveryField)ConfirmSaleAtomicityTests)dotnet build Cluckwork.slnis clean with warnings as errors, which also enforces the #985 style gate.Test edits. One existing test file changed its input:
SaleAllocationPlannerTestsbuildsSaleDemandLines instead of aSalesOrder, because the planner moved to Egg Operations and no longer takes order items. Its 7 assertions are byte-identical, and reversing FIFO order in the planner still turnsWholeOrderFits_ProducesOneDrawPerLot_InFifoOrderred.EggLotLockOrderTestsandEggOperationsModuleTestsonly gained a test each (git diff --numstat: 40 and 7 lines added, 0 removed). Every other test file is unedited.EggLotLockSqlTestsstill finds all threeFOR UPDATEstatements, because the SQL never leftEggLotRepository.EF model digest. The SHA-256 of the design-time
Model.ToDebugString(LongDefault)is45F550E426163CEBEFF36E509CFC6E53B36D26D3D89C72E0F728855BCC8FB9AEonmaind6dbd47 and on this head. The two dumps are byte-identical.SQL trace (re-run script, not committed). A throwaway test adds a
DbCommandInterceptorand aDbTransactionInterceptorthroughConfigureDbContextand records every statement and transaction event of a request. Response bodies are recorded too, with ids and timestamps normalised. Repeated runs on the base were identical, so the trace has no noise. This head againstmain:canChangeCurrencyunpaid=true; record, list and void payments; balances; cancelAll 54 files match. The script also catches a wrong port: locking per grade adds a second lock statement, and saving inside
DrawAsyncsplits the save into separate batches.Invariants. Every suite passed in the full run at 4036702; none was edited except as stated above.
EggLotConcurrencyTests(2),SalesDiscountReasonTests.ParallelConfirms_OnlyOneWins_AndVersionMovesOnce,SalesDiscountCeilingTests.TwoManagersConfirmingOneOverCeilingOrderConcurrently_AllocateItExactlyOnceVoidSaleTests.ParallelVoids_ExactlyOneWins_StockRestoredOnce,VoidRacingConfirm_OnSameLots_NeverLosesOrDuplicatesStock(ProductionDate, Id)EggLotLockOrderTests(5 existing cases + the new two-grade case),EggLotLockSqlTestsSaleAllocationPolicyTests(13, incl.ConfirmSale_RestrictedWorkerAssignedRetry_IssuesExactlyOneFifoQuery, whose countingIEggLotRepositorydecorator still sees the port's call)EggLedgerTests.FailedOperations_WriteNoMovements,SaleAllocationPolicyTests.*_AppliesNothing,SaleAllocationPlannerTests.InsufficientStock_ReportsTheShortGradeAndRemainder_AndProducesNoDrawsConfirmSaleAtomicityTests(4, new)VoidSaleTests.Void_RestoresExactSourceLots_AndPreservesFifo,EggLedgerTests.MultiLotSale_WritesOneMovementPerLot_NewestFirst_LotsPage,SimulationSeederTests.SimulationSeed_ConfirmedOrders_DepleteEggLotsFifoFromASharedOldLotPool,SaleAllocationPlannerTestsVoidSaleTests(allocation rows released, never deleted)PaymentsTests.VoidOrder_WithPayments_RefusedUntilPaymentsVoided,AuditTests.MidTransactionFailure_WritesNothing_EventualVoidWritesOneVersionracesCustomerAndOrderTests.ParallelAddItems_TotalMatchesPersistedItems,Customer_Update_StaleBaseVersion_ExactlyOneWinnerOverHttp,PaymentsTests.ParallelPayments_CannotOvershootTheTotal,ParallelVoids_SameBaseVersion_ExactlyOneWinsSalesProductTestsTenantIsolationTests.ConfirmSale_ForOtherAccountsOrder_Returns404,CustomerAndOrderTests.*Foreign*,EggLedgerTests.LotMovements_AreTenantIsolated_ForeignLotLooksNonexistentTransientDbResilienceTests.TransientFailure_InsideAWrappedWrite_IsNotReplayed_ButTheClientsOwnRetrySucceeds; the port opens no transaction or execution strategyFarmSettingsTests(54, with a refusal for each probed table that can exist alone),CurrencyLockRaceTests,CurrencyLockSerializationTests,FarmModuleTestsAuditProvenanceTests,AuditTests,SalesOrderAuditPayloadTestsDemoSeedTests,SimulationSeederTestsThe stock, sales, seed and currency set above ran as one filter of 384 tests at each step and passed every time.
Mutations. Each was applied to a clean build that compiled, run, then reverted and rebuilt:
IEggStocklocks each grade separately, in line orderEggLotLockOrderTests.ConfirmSale_TwoGrades_…(the request must already hold the earlier lot…); the other 5 lock-order cases stay greenSaleAllocationPlannerSaleAllocationPlannerTests.WholeOrderFits_ProducesOneDrawPerLot_InFifoOrder(first draw from the newer lot)Planignores the assigned-flock filterSaleAllocationPolicyTests.DefaultPolicy_RestrictedWorker_AssignedInsufficient_FarmWideSufficient_…andConfirmSale_RestrictedWorkerAssignedRetry_IssuesExactlyOneFifoQuery(OKwhere422is expected)IEggStockReservationexposes the lockedEggLotsModuleContractRealAssemblyTestsSalesOrderonICommerceModuleModuleContractRealAssemblyTests(LeakAsync exposes Cluckwork.Domain.Sales.SalesOrder)ISalesOrderRepository ordersadded back toSaleEndpoints.GetSalesOrderAdapterReachRealTreeTests(contract bypass … GetSalesOrder -> Commerce through …ISalesOrderRepository)PaymentRepositoryremoved fromimplementationsCompatibilityExceptionRealTreeTests, everyPaymentRepositoryread undeclared, its currency-source read includedProductRepositorynot registered as a currency sourceFarmSettingsTests.CurrencyChange_AfterAPricedProductExists_IsRefusedEmail↔Addressin the customer copyCommerceModuleTests.CustomerReads_…Quantity↔QuantityBasein the order-line copyCommerceModuleTests.OrderReads_…ReferenceNumber↔Notein the payment copyCommerceModuleTests.PaymentReads_…IEggStock runs only inside a transaction); with the port's checks also off, at the test'sthe confirm runs outside a transaction(round 1)a drawn lot was already saved before the fault(round 1)EggStockTransactionTests(round 1)ConfirmSaleHandlertakesIEggLotRepositoryagainPeerContractRealTreeTests; the other 338 architecture tests stay green (round 1)Since review round 1,
ConfirmSaleAtomicityTestsalso checks at each fault that the confirm is in a transaction and has saved no draw, so each rule fails on its own. See the round-1 comment for the rest of that round.Not tested, and why.
EggLot.AllocationSourceMissingmoved verbatim intoRestoreAsyncwith no test. It cannot be reached:FK_SalesOrderAllocations_EggLots_EggLotIdstops a drawn lot being deleted, and void is AdminOnly, so the flock scope never hides one.Cleanup.
pstack:deslopran on the branch diff before opening. It reflowed one confirm comment that the rewrite had spliced onto a single long line, and removed a comment inEggStock.DrawAsyncthat repeated the interface's reason for throwing. Building withIDE0005enabled then found three unused usings this branch introduced (ProductEndpoints,CustomerEndpoints,CommerceModule); they are gone. The ten otherIDE0005hits in touched files were already unused onmain, so they are left alone.Nothing user-visible changes, so this PR has no screenshots.