Repository navigation
refactor(finance): put Finance behind an IFinanceModule contract - #1010
Conversation
…m guards A ledger owner may list its contract types under `contract`. The adapter reach walk then fails on any adapter crossing into that owner through a type outside the list, and on a listed type that is undeclared or owned by another module. SeamSurfaceScanner.ScanContracts adds an entity-or-aggregate rule on top of the #847 persistence rules. No owner declares a contract yet, so the real tree is unchanged.
ExpenseEndpoints and SimulationDataSeeder now reach Finance through IFinanceModule, its four commands and three result records, instead of the handlers, repositories and aggregates. FinanceModule delegates to the existing handlers, so routes, status codes, validation and audit rows are unchanged. The ledger declares the contract, and the regenerated matrix counts FinanceModule's SeedDefaults read on the Finance -> Farm edge.
|
Review of record: Codex PR #1010 reviewReviewed FindingsP2 · CONFIRMED · Tuple aliases hide forbidden adapter parametersLocation: The contract check accepts a non-contract Finance repository in an adapter parameter when the parameter uses a tuple alias. Concrete mutation in using ExpenseRepos = (
Cluckwork.Application.Features.Expenses.IFinanceModule Finance,
Cluckwork.Application.Features.Expenses.IExpenseRepository Expenses);Add Evidence: The mutated API builds with zero warnings/errors, the real adapter guard passes, and all 250 architecture tests pass, including matrix regeneration. See P2 · CONFIRMED · Aliased service factories evade the contract checkLocation: An adapter can construct a non-contract Finance handler through an aliased Concrete mutation: import _ = ReviewActivator.CreateInstance<
Cluckwork.Application.Features.Expenses.CreateExpense.CreateExpenseHandler>(reviewServices);An endpoint can use that handler directly instead of Evidence: The mutated API builds cleanly and all 250 architecture tests pass. Replacing the alias with the qualified P2 · CONFIRMED · Public fields hide aggregates inside contract DTOsLocation: The contract scanner accepts an aggregate exposed through a public field at depth because its recursive walk examines only properties. Concrete mutation: give public sealed class ReviewEnvelope
{
public ReviewPayload? Payload { get; init; }
}
public sealed class ReviewPayload
{
public Cluckwork.Domain.Expenses.Expense? Entity;
}If Finance fills that field, an adapter receives the mutable aggregate through Evidence: The compiled mutation passes both real assembly guards and all 250 architecture tests. Changing only P2 · CONFIRMED · Contract DTOs can expose Finance repository interfacesLocation: The contract rules allow a DTO to return Concrete mutation: public sealed record ExpenseListPage(
IReadOnlyList<ExpenseDetails> Items, long TotalMinorUnits)
{
public IExpenseRepository? Repository { get; init; }
}Finance can populate this with its injected repository, letting an adapter read aggregates or call repository mutations through a permitted contract result. The repository interface itself matches no forbidden rule, and the recursive walk does not inspect its methods or inherited interfaces. The stronger root-interface inspection never runs for this nested interface. Evidence: The compiled mutation passes all 250 architecture tests, including the new contract guard and #847. See Behavior and scope
Verified counts and checksThe branch's actual scanners run over both source trees independently reproduced the PR's numbers. Full output is in
The added Farm read symbol is
NitsThe PR body says "eight expense endpoints"; the file maps seven routes: three category routes and four expense routes. Verdict: Request changes for the four confirmed P2 contract-guard defects; no current product behavior regression found. |
Round 1 review of #1010 found four mutations that built and left every architecture test green: - A tuple alias hid an adapter parameter's element types. Alias expansion now reads NamespaceOrType, so tuple aliases are walked. - An aliased ActivatorUtilities receiver evaded the service-call selector. The receiver alias is now expanded before matching. - The seam walk followed public properties only. It now also follows public instance fields. - A contract result could expose a repository interface. The contract walk now follows the method signatures of any nested interface, so a repository that returns an entity fails.
Round 1 response: Codex
|
| # | Finding | Change | Evidence with the fix |
|---|---|---|---|
| 1 | Tuple alias hides an adapter parameter (AdapterReachScanner.cs:160) |
Alias expansion reads UsingDirectiveSyntax.NamespaceOrType instead of Name, so a tuple alias's element types are walked. Fixture: AdapterReachTests.ContractedOwner_TupleAliasParameterIsWalkedForBypasses. |
Your ExpenseRepos mutation on ListCategories reds AdapterReachRealTreeTests with contract bypass ...ListCategories -> Finance through ...IExpenseRepository. |
| 2 | Aliased ActivatorUtilities receiver (:385) |
ServiceTypes expands a receiver alias before it compares the receiver with ActivatorUtilities. Fixture: AdapterReachTests.AliasedActivatorUtilitiesReceiver_IsReach. |
Your ReviewActivator.CreateInstance<CreateExpenseHandler> mutation reds the same test with contract bypass ... through ...CreateExpenseHandler. |
| 3 | Public fields hide an aggregate (SeamSurfaceScanner.cs:280) |
The recursive walk follows public instance fields as well as properties. Fixture: the field/property control pair SeamSurfaceTests.Contract_AggregateBehindANestedFieldOrProperty_IsAViolation, a theory with both cases. |
Your Envelope.Payload.Entity field mutation reds ModuleContractRealAssemblyTests via ExpenseListPage.Envelope -> ReviewEnvelope.Payload -> ReviewPayload.Entity. |
| 4 | Contract result exposes IExpenseRepository (:44/:280) |
In contract scans, the walk follows the method signatures, inherited ones included, of any interface a contract type exposes. A repository that returns an entity therefore fails. Fixtures: Contract_RepositoryInAResult_IsAViolation (a non-generic IFlockStore : IRepository<Flock, Guid>, the shape of your case) and Contract_NestedInterfaceSignature_IsWalked. |
Your Repository property mutation reds ModuleContractRealAssemblyTests via ExpenseListPage.Repository -> IExpenseRepository.ListAsync -> ... -> Expense. |
Scope notes.
- The field walk (3) applies to [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847's scan too. The real Application assembly stays green under it.
- The nested-interface walk (4) runs for contract scans only. Applied to [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847's scan, it double-reported two existing fixtures through their base interfaces, so [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847's behaviour is unchanged.
- One limit remains, recorded in
docs/decisions/849-module-contract.md. An interface that returns only DTOs is not detected as a repository, because nothing structural marks it as one.
Nit. The PR body now says seven expense routes (three category, four expense).
Verification at c5b1b5d. dotnet build Cluckwork.sln reports 0 warnings. Domain 495, Application 563, AppHost 10 and Integration 1873 all pass. The 6 new fixture tests account for the Application change from 557 to 563.
|
Review of record: Codex PR #1010, round twoReviewed fix Original findings retested
Evidence: FindingsP2 · CONFIRMED · Alias normalization still misses namespace prefixes and
|
…ards Round 2 review of #1010 found three more guard holes: - Alias handling matched exact spellings. ExpandAlias now qualifies every written name in one place: a leftmost alias written Alias.X or Alias::X becomes its target, global:: stays fully qualified, and a qualifier the walk cannot resolve fails the guard. - The seam walk skipped static and inherited static members. Properties and fields now use public instance and static members, flattened. - The nested interface walk kept its own reduced signature. Root and nested interfaces now share WalkSignature, which includes generic constraints, and the walk follows inherited interface types.
Round 2 response: Codex
|
| # | Finding | Change | Evidence with the fix |
|---|---|---|---|
| 1 | Alias normalization misses namespace prefixes and alias:: (AdapterReachScanner.cs:383, ModuleLedgerScanner.cs:613) |
One normalization point, AdapterReachScanner.ExpandAlias, used by both the parameter walk and the service-receiver match. A leftmost alias written Alias.X or Alias::X becomes its target. global:: stays fully qualified. A qualifier the walk cannot resolve fails the guard (AliasErrors) instead of dropping to unresolved. The exact-spelling receiver case from round 1 is removed. Fixtures: NamespaceAliasPrefixOnTheActivatorReceiver_IsReach, AliasQualifiedParameter_IsReach, UnresolvableAliasQualifier_FailsClosed. |
ReviewDI.ActivatorUtilities.CreateInstance<CreateExpenseHandler> reds AdapterReachRealTreeTests with contract bypass ...ListCategories -> Finance through ...CreateExpenseHandler. ReviewDomain::Expense? reviewExpense on ToResponse reds it with contract bypass ...ToResponse -> Finance through Cluckwork.Domain.Expenses.Expense. |
| 2 | Public static fields skipped (SeamSurfaceScanner.cs:286) |
Nested properties and fields use one PublicMembers flag set: public, instance and static, flattened, so inherited statics are included. Fixtures: a static field, and an inherited static field on a derived class. |
public static Expense? Entity; on ExpenseListPage reds ModuleContractRealAssemblyTests via ExpenseListPage -> ExpenseListPage.Entity. |
| 3 | Nested interfaces omit generic constraints and inherited type arguments (:296-298) |
The reduced second signature is gone. Root CheckMethod and the nested-interface walk both call one WalkSignature, which covers generic arguments and their constraints, parameters and return. The nested walk also walks each inherited interface type through Walk, with the same visited-set cycle protection. Fixtures: IFlockRegistry.Register<T>() where T : IFlockStore and IFlockTag : IMarker<Flock>. |
void Register<T>() where T : IExpenseRepository reds ModuleContractRealAssemblyTests via IReviewRegistry.Register -> T -> IExpenseRepository -> IRepository<Expense, Guid> -> Expense. IReviewTag : IReviewMarker<Expense> reds it via ExpenseListPage.Tag -> IReviewMarker<Expense> -> Expense. |
Nit. The documented limit in docs/decisions/849-module-contract.md now says "exposes only DTOs and values".
Regression check. Round 1's IExpenseRepository property mutation still reds the same test. Its path now goes through the inherited IRepository<Expense, Guid>.
Verification at 64d40ac. dotnet build Cluckwork.sln reports 0 warnings and 0 errors. Domain 495, Application 563 → 570 (the 7 new fixtures), AppHost 10 and Integration 1873 all pass. No production file changed this round.
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 holes). No round 3 will be triggered.
Change map: what this PR changes, before and afterPosted by the coordinator so this PR's structural change can be read at a glance. The database, routes and responses are unchanged; only who calls whom changes. New guard (tests only): After this PR and #1012 both merge:
|
Why
This is the Finance pilot for #514 (Track C slice 7) and the first module behind a contract. It uses the narrowed criterion (option 1) from the 2026-09-26 audit. Finance handlers and repositories now sit behind
IFinanceModuleinside the existingCluckwork.Applicationassembly, with no new project.ExpenseEndpointskeeps its three non-Finance injections: the Farm currency read, the Flock name read andIAuditEventRepository.The existing guards could not tell the old shape from the new one. The #846 ratchet records owners, not types, and #847 allows concrete aggregates. So this PR adds a ledger
contractlist and two checks that make the rule enforceable. Later contract slices (#851 to #855) copy this shape.Scope
IFinanceModule,FinanceModuleand the result recordsExpenseDetails,ExpenseCategoryDetailsandExpenseListPage, all inCluckwork.Application.Features.Expenses.FinanceModuledelegates to the four existing handlers and two repositories.ExpenseEndpointsinjectsIFinanceModulein place of the handlers,IExpenseRepositoryandIExpenseCategoryRepository.IValidator<TCommand>stays, because the commands are contract types.SimulationDataSeederinjectsIFinanceModulein place ofCreateExpenseCategoryHandlerandCreateExpenseHandler. Itsdb.Expensesanddb.ExpenseCategoriesreads stay. [C] #514 slice 8: close or date every compatibility exception #850 tracks them.module-ledger.jsongainsowners.Finance.contract.AdapterReachScannerfails on a crossing through a non-contract type, andSeamSurfaceScanner.ScanContractsrejects entities and aggregates at any depth. The ledger's Finance -> Farm cell namesFinanceModule, and the regenerated matrix showsR (3)there.docs/decisions/849-module-contract.mdand a one-paragraph rule in AGENTS.md.Out of scope: Farm, Flock Management and Insights ports, a new assembly, namespace moves, schema changes and any behaviour change. Nothing user-visible changes, so this PR has no screenshots.
Finance reach, before → after
origin/mainExpense,ExpenseCategory)A)The last row does not move, and it should not. Every endpoint still reaches Finance, now through its contract. The first row is the count that falls. I measured it by running this branch's scanner and ledger over
origin/main'ssrc/.Blast radius
The change touches the seven expense routes (three category, four expense) and the simulation seeder's two expense writes. Every write still runs the same handler in the same DI scope, and the reads run the same repository queries in the same order. No EF configuration, entity or migration file changes.
Verification
origin/main05d7e7a → this branch: Domain 495 → 495, Application 544 → 570, AppHost 10 → 10, Integration 1873 → 1873. Total 2922 → 2948, all green both times. The 26 new tests cover the guards andFinanceModule's read mapping. No existing test was edited.dotnet build Cluckwork.slnreports 0 warnings and 0 errors.origin/main'ssrc/, the contract check reports all 12 bypasses.Task<Expense?> LeakAsync(Guid)toIFinanceModuleredsModuleContractRealAssemblyTests. The [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847 guard alone stays green on that leak.FarmIdandExpenseCategoryIdinFinanceModule.ToDetailsredsFinanceModuleTests.imagechecks fail on Trivy over the base image's OpenSSL (ci: move image vulnerability scanning out of CI to a weekly scan of published images #1006). That failure is unrelated to this change./expensesspecs on the month-boundary bug, E2E expense specs fail at farm month boundaries #1009, which also failed unrelated PRs.Closes #849