Skip to content

refactor(finance): put Finance behind an IFinanceModule contract - #1010

Merged
mforce merged 5 commits into
mainfrom
refactor/849-finance-module
Oct 2, 2026
Merged

mforce merged 5 commits into
mainfrom
refactor/849-finance-module

Conversation

@mforce

@mforce mforce commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

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 IFinanceModule inside the existing Cluckwork.Application assembly, with no new project. ExpenseEndpoints keeps its three non-Finance injections: the Farm currency read, the Flock name read and IAuditEventRepository.

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 contract list and two checks that make the rule enforceable. Later contract slices (#851 to #855) copy this shape.

Scope

  • IFinanceModule, FinanceModule and the result records ExpenseDetails, ExpenseCategoryDetails and ExpenseListPage, all in Cluckwork.Application.Features.Expenses. FinanceModule delegates to the four existing handlers and two repositories.
  • ExpenseEndpoints injects IFinanceModule in place of the handlers, IExpenseRepository and IExpenseCategoryRepository. IValidator<TCommand> stays, because the commands are contract types.
  • SimulationDataSeeder injects IFinanceModule in place of CreateExpenseCategoryHandler and CreateExpenseHandler. Its db.Expenses and db.ExpenseCategories reads stay. [C] #514 slice 8: close or date every compatibility exception #850 tracks them.
  • module-ledger.json gains owners.Finance.contract. AdapterReachScanner fails on a crossing through a non-contract type, and SeamSurfaceScanner.ScanContracts rejects entities and aggregates at any depth. The ledger's Finance -> Farm cell names FinanceModule, and the regenerated matrix shows R (3) there.
  • docs/decisions/849-module-contract.md and 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

Measure origin/main this branch
Adapter crossings into Finance through a non-contract type 12 (8 types: 4 handlers, 2 repositories, Expense, ExpenseCategory) 0
All adapter crossings into Finance 16 14
Adapter rows reaching Finance (matrix A) 9 9

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's src/.

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

  • Test counts at origin/main 05d7e7a → 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 and FinanceModule's read mapping. No existing test was edited.
  • dotnet build Cluckwork.sln reports 0 warnings and 0 errors.
  • Each mutation below turned its guard red:
    • Run against origin/main's src/, the contract check reports all 12 bypasses.
    • Adding Task<Expense?> LeakAsync(Guid) to IFinanceModule reds ModuleContractRealAssemblyTests. The [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847 guard alone stays green on that leak.
    • Removing the bypass failures, the undeclared-type check or the owner check each reds its fixture test.
    • Swapping FarmId and ExpenseCategoryId in FinanceModule.ToDetails reds FinanceModuleTests.
  • The image checks 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.
  • Playwright smoke passed on all three shards at 64d40ac. Earlier heads failed four /expenses specs on the month-boundary bug, E2E expense specs fail at farm month boundaries #1009, which also failed unrelated PRs.
  • Every other check passes.

Closes #849

mforce added 3 commits October 1, 2026 05:51
…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.
@mforce

mforce commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Review of record: Codex gpt-6-astra, round 1, at head ef323b91. Posted by the coordinator. The log files named below are local to the review host.

PR #1010 review

Reviewed ef323b9155ce1d8c2fbbf7ee4234cfa44dc659cf against 05d7e7a7, using the detached checkout and #849's superseding audit. No current product behavior defect found. Four P2 defects undermine the new contract guards. Every finding below was reproduced; none is merely plausible.

Findings

P2 · CONFIRMED · Tuple aliases hide forbidden adapter parameters

Location: tests/Cluckwork.Application.Tests/Architecture/AdapterReachScanner.cs:160, consumed by the new contract check at line 222.

The contract check accepts a non-contract Finance repository in an adapter parameter when the parameter uses a tuple alias.

Concrete mutation in ExpenseEndpoints.cs:

using ExpenseRepos = (
    Cluckwork.Application.Features.Expenses.IFinanceModule Finance,
    Cluckwork.Application.Features.Expenses.IExpenseRepository Expenses);

Add ExpenseRepos reviewRepos to ListCategories alongside its existing parameters. The method can now reach the repository through reviewRepos.Expenses, outside the declared contract. UsingDirectiveSyntax.Name does not represent the tuple type, so alias expansion skips it; the unresolved alias never becomes a failure.

Evidence: The mutated API builds with zero warnings/errors, the real adapter guard passes, and all 250 architecture tests pass, including matrix regeneration. See 1010-astra-mutation-tuple_alias_parameter-build.log and 1010-astra-mutation-tuple_alias_all_architecture.log. A direct repository alias and an explicit generic repository parameter both correctly fail. Walk the alias's complete type syntax, or reject unsupported aliases that reach an adapter.

P2 · CONFIRMED · Aliased service factories evade the contract check

Location: tests/Cluckwork.Application.Tests/Architecture/AdapterReachScanner.cs:385, consumed by the new contract check at line 222.

An adapter can construct a non-contract Finance handler through an aliased ActivatorUtilities call without triggering the contract guard.

Concrete mutation: import ReviewActivator = Microsoft.Extensions.DependencyInjection.ActivatorUtilities, add IServiceProvider reviewServices to ListCategories, then execute:

_ = ReviewActivator.CreateInstance<
    Cluckwork.Application.Features.Expenses.CreateExpense.CreateExpenseHandler>(reviewServices);

An endpoint can use that handler directly instead of IFinanceModule. The selector compares the receiver's literal spelling against ActivatorUtilities and its qualified name; it never expands receiver aliases. This is an advertised service-resolution path, not the documented exclusion for arbitrary object creation.

Evidence: The mutated API builds cleanly and all 250 architecture tests pass. Replacing the alias with the qualified ActivatorUtilities name makes the adapter guard fail. See 1010-astra-mutation-aliased_activator-build.log, 1010-astra-mutation-aliased_activator_all_architecture.log, and 1010-astra-mutation-static_activator.log. Resolve receiver aliases before selecting these service calls.

P2 · CONFIRMED · Public fields hide aggregates inside contract DTOs

Location: tests/Cluckwork.Application.Tests/Architecture/SeamSurfaceScanner.cs:280, reached by the new ScanContracts entry at line 55.

The contract scanner accepts an aggregate exposed through a public field at depth because its recursive walk examines only properties.

Concrete mutation: give ExpenseListPage an Envelope property and add these types in its existing namespace:

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 page.Envelope.Payload.Entity, violating the new "no entity or aggregate at any depth" rule.

Evidence: The compiled mutation passes both real assembly guards and all 250 architecture tests. Changing only Entity from a field to a property makes ModuleContractRealAssemblyTests fail and identify Expense. See 1010-astra-mutation-nested_field_all_architecture.log and 1010-astra-mutation-nested_property.log. Include public data fields in the contract walk and retain this property/field control pair as a causal fixture.

P2 · CONFIRMED · Contract DTOs can expose Finance repository interfaces

Location: tests/Cluckwork.Application.Tests/Architecture/SeamSurfaceScanner.cs:44 and :280.

The contract rules allow a DTO to return IExpenseRepository, contrary to #849's requirement that contract types never carry repositories.

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 1010-astra-mutation-repository_property_all_architecture.log. Reject repository interfaces on contract results and ensure nested interfaces cannot hide forbidden signature types.

Behavior and scope

  • All seven HTTP route mappings, validation calls, status/error mapping and authorization remain unchanged. Facade writes return the original handler tasks without catching or translating results. List queries remain list, sum, account, provenance, flock names in that order. DTO copies preserve the response fields, including Version.
  • SeedDefaults.FarmId is the same static readonly GUID before and after, 0000000f-0000-0000-0000-000000000001, in every tenant context. Both reads use the same repository; its existing AccountId query filter still supplies tenant isolation. It is not a tenant-derived ID that changed when moved.
  • CreateExpenseHandler retains category/flock checks, the transaction, Farm's FOR SHARE currency read, insertion and audit in their original order. The seeder retains its account argument, ActAs, result handling and existence checks. No domain mutation, EF configuration or migration changed.
  • The narrowed pilot is appropriately scoped. A small extension to the existing guards is justified because owner-level reach and [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847 alone cannot enforce the new boundary. The facade is straightforward for later slices to copy. Fix the guard holes before presenting this as their enforcement pattern; no broader contract framework is needed.
  • Body-level static calls and standalone typeof(IExpenseRepository) also stay green. Those are documented selector exclusions in decision [C] #514 slice 7: Finance pilot — first module behind a contract #849, so they are reported here as limits, not additional findings.

Verified counts and checks

The branch's actual scanners run over both source trees independently reproduced the PR's numbers. Full output is in 1010-astra-counts.log.

Measure Base Head
Finance crossings through non-contract types 12 0
All Finance adapter crossings 16 14
Finance adapter rows 9 9
Finance-to-Farm read symbols 2 3

The added Farm read symbol is FinanceModule. The committed matrix matches regeneration; it was not hand-edited during review.

  • Clean solution builds before and after mutations: zero warnings/errors. Initial targeted Application run: 253/253 passed; final restored-source Application suite: 557/557 passed. Final logs: 1010-astra-final-build.log and 1010-astra-final-application.log.
  • Real Postgres integration run through sg docker: 71/71 passed, covering ExpensesTests, CurrencyLockRaceTests, FlockScopeTests, TenantIsolationTests and SimulationSeederTests. This includes the parallel-adjust version race, expense audit creation and currency serialization. Logs: 1010-astra-build.log, 1010-astra-baseline.log, 1010-astra-integration.log.
  • Direct aliases, generic parameters, typed lambdas, nested types, generic service resolutions assigned to var, direct typeof service resolutions and unaliased static service factories correctly turn the adapter guard red.
  • Adding nonexistent Cluckwork.Application.Features.Expenses.DoesNotExist to the ledger fails both the real adapter guard and contract assembly guard. Evidence: 1010-astra-mutation-missing_contract_type.log.
  • Every mutation and temporary test was reverted. No commits, pushes, GitHub comments or edits to another checkout were made. Known ci: move image vulnerability scanning out of CI to a weekly scan of published images #1006 and E2E expense specs fail at farm month boundaries #1009 failures remain outside this review.

Nits

The 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.
@mforce

mforce commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Round 1 response: Codex gpt-6-astra review at ef323b91

All four P2 findings are fixed in c5b1b5d. For each one, I first added a fixture test and confirmed it failed on the unfixed scanner (5 of 5 new fixtures red at ef323b91). Then I applied the fix. With the fix in place, I applied your exact mutation to the real tree, saw the named guard go red, and reverted it.

# 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.

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.

@mforce

mforce commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Review of record: Codex gpt-6-astra, round 2, at head c5b1b5d7. Posted by the coordinator. The log files named below are local to the review host.

PR #1010, round two

Reviewed fix c5b1b5d741971b4bc3e94642bdb3ba6f0d6cc36c against ef323b91, including the worker's response. No product defect found. The fix changes only architecture tests/scanners and documentation. All four original reproductions are now rejected, but three categories of guard defects remain. All findings below are CONFIRMED.

Original findings retested

Round-one mutation New result
Tuple alias containing IExpenseRepository on ListCategories RED: names ListCategories, Finance, IExpenseRepository, and the source line.
ReviewActivator.CreateInstance<CreateExpenseHandler> RED: names the non-contract handler and adapter.
ExpenseListPage.Envelope.Payload.Entity public instance field RED: reports Expense and the complete field/property path.
ExpenseListPage.Repository exposing IExpenseRepository RED: reports Expense through repository method signatures.

Evidence: 1010-r2-astra-r1_tuple_alias.log, 1010-r2-astra-r1_aliased_activator.log, 1010-r2-astra-r1_nested_field.log, and 1010-r2-astra-r1_repository_property.log. These are actual mutations of the new source, not the worker's reported results. The reflection cases were rebuilt before testing. The diagnostics are useful, although record-generated methods produce duplicate paths.

Findings

P2 · CONFIRMED · Alias normalization still misses namespace prefixes and ::

Location: tests/Cluckwork.Application.Tests/Architecture/AdapterReachScanner.cs:383; parameter-name normalization also uses ModuleLedgerScanner.cs:613.

The fix expands an alias only when its name equals the entire service receiver, while parameter normalization discards custom :: qualifiers, allowing two ordinary alias forms to hide Finance internals.

Concrete service mutation:

using ReviewDI = Microsoft.Extensions.DependencyInjection;
// In ListCategories, with an IServiceProvider parameter:
_ = ReviewDI.ActivatorUtilities.CreateInstance<
    Cluckwork.Application.Features.Expenses.CreateExpense.CreateExpenseHandler>(reviewServices);

The adapter can construct and invoke the handler without the facade. The lookup searches for an alias literally named ReviewDI.ActivatorUtilities, so it never expands ReviewDI.

A second compiled mutation adds using ReviewDomain = Cluckwork.Domain.Expenses; and an optional ReviewDomain::Expense? reviewExpense = null parameter to the existing ToResponse helper. That allows a non-contract aggregate parameter. DottedText strips ReviewDomain::, leaving an unresolved Expense, which does not fail the guard. This mutation does not alter endpoint binding.

Evidence: Both API builds succeed with zero warnings/errors, and each mutation passes all 256 architecture tests, including the real adapter guard and matrix check. Logs: 1010-r2-astra-namespace_alias_activator-build.log, 1010-r2-astra-namespace_alias_all_architecture.log, 1010-r2-astra-alias_qualified-build.log, and 1010-r2-astra-alias_qualified_all_architecture.log.

Normalize qualified names consistently: preserve custom alias qualifiers, distinguish global::, and expand the receiver's alias prefix before matching service APIs. Adding another exact-spelling case would leave the same underlying problem.

P2 · CONFIRMED · Static aggregate fields still pass the contract guard

Location: tests/Cluckwork.Application.Tests/Architecture/SeamSurfaceScanner.cs:286.

The new field walk explicitly selects instance fields, so a listed contract DTO can expose a public static aggregate field and remain green.

Concrete mutation:

public sealed record ExpenseListPage(
    IReadOnlyList<ExpenseDetails> Items, long TotalMinorUnits)
{
    public static Expense? Entity;
}

Finance can populate this field, and an adapter can access the aggregate through ExpenseListPage.Entity. This contradicts the stated prohibition on entities in contract types and the updated promise to walk public fields. It is not the documented DTO-only interface limitation.

Evidence: The compiled mutation passes both real assembly guards and all 256 architecture tests in 1010-r2-astra-static_field_all_architecture.log. Changing just the field into a public static property makes ModuleContractRealAssemblyTests fail with ExpenseListPage.Entity exposes ...Expense; see 1010-r2-astra-static_property_control.log. Include public static fields consistently with the public static properties already checked, including the relevant inherited members.

P2 · CONFIRMED · Nested interfaces omit generic constraints and inherited type arguments

Location: tests/Cluckwork.Application.Tests/Architecture/SeamSurfaceScanner.cs:296–298.

The added interface traversal walks method parameters and return types, but omits method generic parameters and the inherited interface types themselves, letting forbidden contract types escape through both.

Concrete mutation: add IReviewRegistry? Registry { get; init; } to ExpenseListPage, then declare:

public interface IReviewRegistry
{
    void Register<T>() where T : IExpenseRepository;
}

An adapter now consumes an interface whose public constraint requires Finance's internal repository. The root CheckMethod handles generic constraints, but the new nested-interface loop does not. Changing the method to T Resolve<T>() where T : IExpenseRepository correctly fails because the return type happens to lead the walk to the constraint.

A second mutation gives the page an IReviewTag property and declares:

public interface IReviewMarker<T>;
public interface IReviewTag : IReviewMarker<Expense>;

The contract now explicitly references the concrete aggregate through an inherited generic interface, yet the walker inspects only that interface's methods. There are none, so it misses the Expense type argument. Direct generic arguments and inherited methods that return an entity are correctly rejected; the omitted interface type is the distinction.

Evidence: Both compiled mutations pass all 256 architecture tests. See 1010-r2-astra-nested_constraint_all_architecture.log and 1010-r2-astra-inherited_marker_all_architecture.log. The return-type control fails in 1010-r2-astra-nested_constraint_control.log. Walk inherited interface types and generic method constraints with the existing cycle protection, rather than maintaining a reduced second definition of a method signature.

Other checks and the stated limit

  • Nested tuples inside generic aliases are rejected. using static ActivatorUtilities is rejected when it resolves a non-contract handler. A same-scope alias of another alias fails compilation with CS0246, so I did not count that invalid C# as a scanner bypass.
  • Public instance fields inherited from base classes are rejected when they expose an entity. Interfaces inherited through IReader<Expense> and interfaces exposed inside IReadOnlyList<T> are rejected. Recursive interfaces exposing only DTOs terminate and pass. Evidence: 1010-r2-astra-shapes.log; its temporary fixture is saved as 1010-r2-astra-shapes.cs.
  • I found no new false positive in legitimate code. The clean head passes all 259 targeted tests. Both [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847 real-assembly checks remain green. Thus the broader instance-field walk changes no current production seam's verdict.
  • The documented DTO-only repository limit is acceptable as an explicit review obligation. An unmarked interface whose entire public signature uses DTOs/values is structurally indistinguishable from a DTO-based service. Naming it Repository is not reliable evidence. Current IExpenseRepository and inherited IRepository<Expense, Guid> signatures do expose aggregates and are now caught in the original reproduction. Do not describe this as complete enforcement of the semantic prohibition on repositories. This limit does not excuse the findings above, where forbidden types are explicitly present and observable.
  • No production files changed in the fix. The round-one product-behavior review therefore still applies; I did not rerun database or browser tests in this round. The unrelated image and month-boundary failures remain out of scope.
  • Final restored-source validation: dotnet build Cluckwork.sln --no-restore succeeds with zero warnings/errors; the complete Application suite passes 563/563. Logs: 1010-r2-astra-final-build.log and 1010-r2-astra-final-application.log.
  • All mutations and temporary tests were reverted. No commits, pushes, GitHub comments, or edits to another checkout were made.

Nits

In the documented limit, prefer "exposes only DTOs/values" to "only returns DTOs": a method accepting an entity is detected even if its return type is a DTO.

Verdict: Request changes for the three confirmed guard-defect categories; all four original reproductions are fixed, and no product defect was found.

…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.
@mforce

mforce commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Round 2 response: Codex gpt-6-astra review at c5b1b5d7

All three P2 findings and the nit are fixed in one commit, 64d40ac. I proved each the same way as round 1. First, 7 new fixture tests, all red on the unfixed scanner at c5b1b5d7. Then the fix. Then each of your mutations applied to the real tree, built clean, seen red, and reverted.

# 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.

@mforce

mforce commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Change map: what this PR changes, before and after

Posted 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.

BEFORE                                     AFTER

ExpenseEndpoints                           ExpenseEndpoints
 ├─► IExpenseCategoryRepository             ├─► IFinanceModule ◄── new front door
 ├─► IExpenseRepository                     │      │
 ├─► CreateExpenseCategoryHandler           │      ▼
 ├─► UpdateExpenseCategoryHandler           │   FinanceModule (just forwards)
 ├─► CreateExpenseHandler                   │    ├─► IExpenseCategoryRepository
 ├─► AdjustExpenseHandler                   │    ├─► IExpenseRepository
 │   (and got back Expense /                │    ├─► 4 handlers (unchanged)
 │    ExpenseCategory DB objects)           │    └─► returns plain records:
 │                                          │        ExpenseDetails,
 │                                          │        ExpenseCategoryDetails,
 │                                          │        ExpenseListPage
 ├─► IFlockRepository     (Flock name)      ├─► IFlockRepository     ┐ unchanged,
 ├─► IAccountRepository   (currency)        ├─► IAccountRepository   │ not Finance
 └─► IAuditEventRepository (history)        └─► IAuditEventRepository┘ (option 1)

SimulationDataSeeder                       SimulationDataSeeder
 ├─► CreateExpenseCategoryHandler           ├─► IFinanceModule
 ├─► CreateExpenseHandler                   └─► db.Expenses counts (left for #850)
 └─► db.Expenses counts

New guard (tests only):

module-ledger.json
  owners.Finance.contract = [IFinanceModule, ExpenseDetails, ...]
        │
        ├─► AdapterReachScanner: an endpoint/CLI/job touching any OTHER
        │                        Finance type → CI fails
        └─► SeamSurfaceScanner:  a contract type exposing a DB object
                                 (at any depth) → CI fails

After this PR and #1012 both merge: ExpenseEndpoints

ExpenseEndpoints
 ├─► IFinanceModule    (#1010)  ← expenses and categories
 ├─► IInsightsModule   (#1012)  ← "created by / last changed by"
 ├─► IFlockRepository           ← flock name   (future Flock slice, #852)
 └─► IAccountRepository         ← currency     (future Farm slice, #851)

The two PRs overlap in three files: ExpenseEndpoints.cs, module-ledger.json and the generated coupling-matrix.md. Whichever merges second rebases and regenerates the matrix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C] #514 slice 7: Finance pilot — first module behind a contract

1 participant