Repository navigation
refactor(flocks): put flock lifecycle behind an IFlockModule contract - #1021
Conversation
|
Review of record: Codex Review of PR #1021Reviewed head FindingP2 · CONFIRMED · Guard defect, not a product defect The new derived-type walk searches only the base type's assembly, so it misses an Application subclass of Domain's Concrete failure scenario: add this public type in public sealed class ReviewFlockResult(Cluckwork.Domain.Flocks.Flock flock, Result result)
: Result(result.IsSuccess, result.Error)
{
public Cluckwork.Domain.Flocks.Flock Flock { get; } = flock;
}
Evidence: I made that mutation and ran Logs: The new walk correctly closes the same-assembly Product correctnessNo product defect found.
Item 4: pre-existing feed-usage stale readThe lookup does not worsen this issue. Both calls still use the same tracked repository query. Creating a new I ran a temporary real-Postgres probe: read through the lookup, archive and commit in another scope, then read again in the original scope. The second lookup returned a different record object with stale The port makes a later fix easier to contain: callers already consume immutable records, so a dedicated untracked read can replace the port's implementation without changing those callers. It must remain separate from the tracked repository reads used by lifecycle mutations. This pre-existing defect is not a finding against this PR. Independent verification
The 123 integration tests comprise BirdMovement 5, SubmitDailyEntry 13, DailyEntryAdjust 13, DailyEntryGrade 9, FlockManagement 8, FlockScope 16, FlockScopeMiddleware 13, FeedUsage 13, WaterUsage 7, Expenses 5, SeedAndFlock 3, MortalityLedgerAtomicity 2, FlockNameResolution 3, SimulationDuplicateFlockName 1, RetryBoundary 11 and BootstrapRetryBoundary 1. This includes the parallel submit, parallel adjustment, adjust-versus-void, lifecycle, scope and rollback cases. I did not rerun the entire integration project. The PR's CI checks were green; image publishing was skipped as expected for the PR.
The Submit early-save failure observed The ledger declares the three ports and their carried types, preserves the peer edge kinds and symbols, and lists the two repository implementations. The six #850 rows match their readers and tables: five Platform seeder rows ending at #858 and the Access assignment join ending at #857. The regenerated matrix is unchanged. The decision records accurately describe the transaction, tracking, seeder and schema-FK boundaries, apart from the derived-walk coverage qualification in the finding. The accepted peer-module guard gap is not reported as a defect. The clean solution build also passed the #985 style gate. NitsNone. Verdict: Request changes for one confirmed P2 contract-guard gap; no product defect found. |
|
Codex Astra round 1 (at P2 (CONFIRMED). The derived-type walk searched only the base type's assembly (
Mutation evidence. The reviewer's mutation is an Application
Pre-existing feed-usage stale read. Not changed here, as agreed. It gets its own PR after this one merges. |
|
Review of record: Codex PR #1021, round-two reviewReviewed the five-file fix from Original findingResolved. Repeating the original Application This is a contract assertion failure, not a build failure. A public nested generic subclass with an explicitly typed Findings1. P2 · CONFIRMED · Generic result erasure still bypasses the guard
The derived-type enumeration obtains generic definitions rather than the closed types actually returned, allowing Concrete failure scenario: replace only public async Task<Result> DepleteAsync(Guid id, CancellationToken ct)
{
var result = await depleteFlock.HandleAsync(id, ct);
var flock = await flocks.GetByIdAsync(id, ct);
return result.IsFailure || flock is null ? result : Result.Success(flock);
}This uses the existing Evidence: the contract, adapter and module-ledger checks all passed, 5/5. A temporary real-Postgres integration test resolved Logs: This is a guard defect, not a product defect in the unmodified PR. Prevent entity-bearing generic results from being erased to the non-generic contract, or explicitly accept and document this runtime-type limitation. Enumerating more assemblies does not solve it. 2. P2 · CONFIRMED · The real scan omits an assembly that can supply contract results
Passing only Application and Domain still permits a DI-supplied Infrastructure subclass of Concrete failure scenario, reproduced without making Application reference Infrastructure:
The contract still returns Evidence: contract, adapter, module-ledger and #847 real-assembly checks passed, 7/7. A temporary real-Postgres test resolved the module from DI, depleted a flock, asserted that the result's assembly was Logs: This is a guard defect, not a product defect in the unmodified PR. Include the production assemblies that can supply contract results, enforce an implementation boundary that excludes them, or explicitly accept this reachable gap. The revised decision record names the assembly list, but does not state the resulting unenforced case. This does not involve the accepted peer-module repository-injection gap. Product correctness and scopeNo product defect found. The fix changes tests and documentation only. The two findings are remaining guard limitations exposed by the requested second-round attacks; the original cross-assembly case is fixed. Neither mutation is present in the delivered product code. The #847 scan still receives no contract-case assembly set, so its Item 4: pre-existing feed-usage stale readUnchanged and outside this fix-only review. No production lookup, repository or feed-usage code changed. It remains excluded from findings against this PR. Independent verification
Every mutation and temporary runtime test was reverted. No commits, pushes or GitHub comments were made. NitsNone. Verdict: Original P2 resolved; request changes or explicit acceptance for two confirmed remaining P2 guard gaps, with no product defect found. |
|
Codex Astra round 2 (at 1. A closed generic 2. An Infrastructure subclass of Mutation evidence. The reviewer's mutation is a public
The review loop was stopped deliberately by the owner after two consecutive rounds that found no product defect (rounds 1 and 2 found only guard gaps). No round 3 will be triggered. |
Closes #852
Epic #514, Track C slice 10. This puts Flock Management behind a contract, the third module after Finance (#849) and Farm (#851). Nothing a user sees changes. The only behaviour change is in the simulation seeder, described below.
What changed
IFlockModuleis the adapter surface. It carries the six lifecycle commands, the picker search, birds-removed counts and the movement ledger page.FlockEndpoints,DemoDataSeederandSimulationDataSeedercall it.IFlockLookupis the read port. It providesGetAsync,GetForFlockScopedWriteAsync,GetDisplayNamesAsyncandResolveByNameAsync, and returns aFlockDetailscopy that carriesCanRecordProductionOn(date). The other endpoints and every peer handler call it.IMortalityLedgeris the write port.AppendAsyncadds one daily-entry movement to the caller's unit of work and never saves. A positive count records Mortality and a negative count records an Adjustment, which is the rule Submit, Adjust and Void already used. It also takes over the note truncation fromAdjustDailyEntryHandler.MovementNote, which is deleted.AssignFlockHandler.Flock.CanRecordProductionOngains a static overload over status and depletion date, so the aggregate and the copy share one rule.SeamSurfaceScanner.ScanContractsnow also walks every public type that derives from a walked class. Without that, aFlockproperty added toFlockNameResolution.NotFoundpassed the contract walk. I checked this by mutation.docs/decisions/852-flock-contract.mdis new. AGENTS.md gains one [C] #514 slice 10: Flock Management contract, eligibility and mortality ports #852 paragraph, and the [C] #514 slice 7: Finance pilot — first module behind a contract #849 record now reflects the change.Checkpoint decisions (owner-approved)
FlockModulebuilds six handlers on every resolve. Narrow ports keep a peer request from constructing them, which is the reason [C] #514 slice 9: Farm contract, with tenant identity staying in Platform #851 leftFarmClockalone. The decision record says when later slices should split the same way.IFlockRepositoryagain stays green.SimulationDataSeeder.EnsureFlockAsyncnow resolves by name. OnAmbiguousit fails the seed and names every candidate id, where it used to take the first match. It is the method's only caller.FK_BirdMovements_DailyEntries_DailyEntryId, from a bareGuid?). The decision record says [C] #514 slice 17: assembly split — conditional on Track A/B evidence #859 has to break it.Ledger and reach
mainIFlockRepository/IBirdMovementRepositorySimulationDataSeeder.EnsureFlockAsyncno longer readsdb.Flocksdirectly, so the seven reads my pre-merge probe predicted are now six.Change map
End state of files later slices also change:
IFlockLookupplusIMortalityLedgerand nothing else from Flock Management.DailyEntryEndpointsholdsIFlockLookup. The Egg Operations to Flock Management edge staysW.IFlockLookuponly.InventoryEndpoints.ListFeedUsageandWaterUsageEndpoints.ListWaterUsageholdIFlockLookup.AssignFlockHandlerholdsIFlockLookup.UserRoleAssignmentRepository.ListByNameByUserAsyncis a [C] #514 slice 8: close or date every compatibility exception #850 row withdeleteWhen: #857.deleteWhen: #858.Transaction atomicity evidence
MortalityLedgerAtomicityTestsruns the handlers outside HTTP, where the idempotency middleware's request transaction does not wrap Submit's singleSaveChangesAsync. It faults the audit write, which comes after the mortality append, and asserts that the movement count, entry status andVersionare unchanged.MortalityLedgercallsSaveChangesAsyncafterAddAsyncExecuteInTransactionAsyncruns without a transactionVoid (and Adjust, which uses the same
ExecuteInTransactionAsyncpath) stays atomic under an early save because its own transaction rolls the save back. The second mutation shows the Void test detects a partial commit once nothing holds that transaction. I reverted every mutation.Other mutations (all red, all reverted)
Flockproperty toFlockNameResolution.NotFoundturnsModuleContractRealAssemblyTestsred. It was green before the scanner change.Task<Flock?>default method toIFlockLookupturnsModuleContractRealAssemblyTestsred.SeamSurfaceTests.Contract_AggregateInADerivedCase_IsAViolationred.DepletedOnandArchivedOninFlockLookup.ToDetailsturns 2FlockLookupTestsred.IgnoreQueryFilters()inFlockRepository.ListByNameAsyncturns all 3FlockNameResolutionTestsred.db.Flocks.FirstOrDefaultAsync(name)in the seeder turnsSimulationDuplicateFlockNameTestsred. The re-run then fails later, at the count check.IFlockRepositoryparameter toReportEndpoints.ProductionturnsAdapterReachRealTreeTestsred with a contract bypass.Test baselines (measured, not quoted)
main6e3f584All green.
dotnet build Cluckwork.slnreports 0 warnings and 0 errors. No existing test file was edited;SeamSurfaceTests.csonly gained a fixture and a theory.Race, lifecycle, scope and rollback suites, run per class at the head, all passing:
ParallelSubmits_ExactlyOneMortalityMovement)ParallelAdjustAndVoid_SameBaseVersion_ExactlyOneWins)The ImagePin guards pass (2). No migration, entity configuration or
AppDbContextfile changed, so the EF model is identical.Not in this PR
db.Flocks/db.BirdMovementsreads ([C] #514 slice 16: Platform composition, jobs, CLI and seeder conversion #858) and the assignment join ([C] #514 slice 15: Access contract — security hold point, runs last #857), registered as [C] #514 slice 8: close or date every compatibility exception #850 exceptions.