Skip to content

test(arch): declare the Insights module contract in the ledger - #1040

Merged
mforce merged 1 commit into
mainfrom
refactor/858-p1-insights-contract
Oct 3, 2026
Merged

mforce merged 1 commit into
mainfrom
refactor/858-p1-insights-contract

Conversation

@mforce

@mforce mforce commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

Nothing changes for users. This PR touches no source file.

For maintainers, Insights now has a ledger contract. Insights already had a facade, IInsightsModule (#856), but the ledger did not declare it. InsightsReadOnlyTests already kept endpoints off IReportQueries, IExportQueries and IAuditEventRepository (:58), and already rejected an aggregate inside an Insights result (:42). Nothing stopped a CLI verb, job, seeder or another module from injecting those ports, which resolve straight to ReportQueries, ExportQueries and AuditEventRepository and skip the facade. Now the shared ledger guards reject those bypasses for every adapter and peer as well.

This is P1 of #858's approved plan (plan comment).

Refs #858

What changed

  • module-ledger.json. owners.Insights.contract lists IInsightsModule and its ten result records: AuditEventRead, EntityProvenance, ExportDataset, ProductionReport, ProductionDay, GradeTotal, SalesSummary, ExpenseSummary, ExpenseCategoryTotal and ProfitReport.
    • The design named only the three types adapters take today. Listing every result record follows the [C] #514 slice 7: Finance pilot — first module behind a contract #849 rule ("the interface, its commands and its *Details result records"). It also means a future helper that takes, say, a SalesSummary parameter needs no ledger edit.
    • IReportQueries, IExportQueries and IAuditEventRepository are left out on purpose. IInsightsModule inherits the first two, so their members are part of the contract surface, but listing either type would let callers inject it and skip the facade. IAuditEventRepository is an implementation port.
  • src/AGENTS.md. Adds Insights to "contracted today".
  • docs/decisions/1023-peer-contract-guard.md. The peer guard's "not checked" list now names only Access.

The coupling matrix does not change, because it does not render contracts.

Change map

BEFORE
 Endpoints (Reports, Export, Audit, and five ToResponse helpers)
     --> IInsightsModule, EntityProvenance, ExportDataset      [nothing enforced]
 Any adapter or peer --> IReportQueries | IExportQueries | IAuditEventRepository   [allowed]

AFTER
 Endpoints --> IInsightsModule + 10 result records             [AdapterReachRealTreeTests]
 Any adapter --> IReportQueries | IExportQueries | IAuditEventRepository  --> red
 Any peer module --> the same three ports                      --> red (PeerContractRealTreeTests)
 ledger: owners.Insights gains "contract"; edges, adapters and matrix unchanged

Files #857 also touches:

Mutation table

Each mutant was applied by exact single-match replacement. It was then run against the 22 real-tree, real-model and real-assembly architecture tests, and reverted. Mutants 2 to 4 were also run with origin/main's ledger, to show this PR is what turns them red. The script and logs are under 858-logs/ on the worker host.

# Mutant Head main ledger Failing assertion, quoted
1 Remove EntityProvenance from the contract red: AdapterReachRealTreeTests, CouplingMatrixRealTreeTests n/a contract bypass Cluckwork.Api.Endpoints.DailyEntries.DailyEntryEndpoints.ToResponse -> Insights through Cluckwork.Application.Features.Audit.EntityProvenance at src/Cluckwork.Api/Endpoints/DailyEntries/DailyEntryEndpoints.cs:108; Insights declares a contract, so call one of its contract types (plus the same for the Egg Grade, Expense, Flock and Sale ToResponse helpers)
2 ReportEndpoints.Sales takes IReportQueries instead of IInsightsModule red: AdapterReachRealTreeTests, CouplingMatrixRealTreeTests green contract bypass Cluckwork.Api.Endpoints.Reports.ReportEndpoints.Sales -> Insights through Cluckwork.Application.Features.Reports.IReportQueries at src/Cluckwork.Api/Endpoints/Reports/ReportEndpoints.cs:90
3 CreateExpenseHandler gains a helper taking IAuditEventRepository, with a declared Finance -> Insights edge so the edge ratchet stays quiet red: PeerContractRealTreeTests, CouplingMatrixRealTreeTests only the matrix (new edge) peer contract guard failed: contract bypass Cluckwork.Application.Features.Expenses.CreateExpense.CreateExpenseHandler.Peek -> Insights through Cluckwork.Application.Features.Audit.IAuditEventRepository at src/Cluckwork.Application/Features/Expenses/CreateExpense/CreateExpenseHandler.cs:18
4 AuditEventRead gains an AuditEvent? Leak member (a Platform aggregate, which #847 allows on ordinary seams) red: ModuleContractRealAssemblyTests green module contract guard failed: Cluckwork.Application.Features.Insights.AuditEventRead.op_Equality exposes Cluckwork.Domain.Auditing.AuditEvent via AuditEventRead -> AuditEventRead.Leak
All reverted 22 of 22 green

Rejected mutant shapes, for the record:

Test counts

Suite main 02e8a633 head
Cluckwork.Application.Tests (full) 702 702
SchemaDocsTests image-pin guards (walk every tracked file) 2 2

No integration suite is needed, because no runtime code changed. CI runs everything.

Deslop record

/deslop over git diff origin/main found nothing to remove. The diff is the ledger block and two edited sentences of prose. No comments were added.

Try to refute

  • No runtime change. git diff origin/main --stat -- src ':!src/AGENTS.md' is empty.
  • The list is complete and minimal. Every type an adapter takes today is listed, and nothing listed is an implementation port.
  • The guard's own limit still holds. It reads parameter types and service resolutions, never return types or method bodies. So an endpoint that only receives a ProductionReport from IInsightsModule was never a bypass, before or after.

Description corrected after merge, from the review nits by Codex Astra and Claude Opus.

Insights had a facade, IInsightsModule, but no ledger contract, so the
adapter and peer guards let any caller inject IReportQueries,
IExportQueries or IAuditEventRepository directly. The contract lists the
interface and its ten result records, following the 849 rule.

No source changes. Every real-tree guard stays green, and the coupling
matrix is unchanged.

Refs #858
@mforce
mforce merged commit 922b208 into main Oct 3, 2026
19 checks passed
@mforce
mforce deleted the refactor/858-p1-insights-contract branch October 3, 2026 17:09
@mforce

mforce commented Oct 3, 2026

Copy link
Copy Markdown
Owner Author

Independent review (Claude Opus 5.5: correctness, thermo-nuclear and ponytail) at head 8e7e2bc5. Posted by the coordinator after merge; the P3 findings go to #858 P2.

PR #1040 review: declare the Insights module contract (#858 P1)

Reviewer: Claude Opus 5.5, independent of Codex Astra. Head 8e7e2bc5, base main 02e8a633.
Checkout /home/mforce/.cluckwork-slices/review-1040-opus (detached, git diff 02e8a633...HEAD = 3 files, +16/-3).
Mutation script, per-mutant logs and summary: /home/mforce/.cluckwork-slices/reviews/1040-opus-mutants/.

No product defect. The PR changes no src file. The ledger list is correct and complete, and the guards it turns on catch what the PR says they catch. There are two P3 findings: one is a missing guard note about the boundary, the other is doc wording.

1. Correctness

Is the list exactly the interface plus every result record?

Yes. IInsightsModule (src/Cluckwork.Application/Features/Insights/IInsightsModule.cs:7) inherits IReportQueries and IExportQueries and declares two members of its own. Its full member surface returns these types:

Member (origin) Returns
GetProductionAsync (IReportQueries) ProductionReport → ProductionDay, GradeTotal
GetSalesAsync SalesSummary
GetExpensesAsync ExpenseSummary → ExpenseCategoryTotal
GetProfitAsync ProfitReport
Datasets, GetDataset, BeginConsistentReadAsync (IExportQueries) BCL types, ExportDataset
ListAuditEventsAsync AuditEventRead
GetProvenanceAsync EntityProvenance

That makes 10 records. Every one is top-level, non-generic and owned by Insights by namespace. The public types in the four Insights Application namespaces are exactly those 10 records, IInsightsModule, and the three excluded ports (IReportQueries, IExportQueries, IAuditEventRepository). Nothing is extra and nothing is missing. No parameter type needs listing, because every parameter is a primitive.

The design checkpoint named only 3 types: the interface, EntityProvenance and ExportDataset. The PR lists all 10 records. This matches the house pattern: Farm lists FarmBrandingHashes and Finance lists ExpenseListPage, and no adapter names either one (grep over src/Cluckwork.Api: 0 hits each). Insights therefore needs no special case.

implementations is correctly absent. Insights owns no tables, and CompatibilityExceptionScanner.cs:312-330 rejects an implementation that "reads none of {module}'s tables".

Can an adapter or peer still reach Insights' internals?

The checked paths are closed. Parameter injection, service resolution, a peer constructor, and the concrete InsightsModule facade are all red at head and green on main's ledger (table below). The following paths remain open. #1023's "What this does NOT cover" documents all of them, so none is new:

  • Method bodies. A typed local or a cast such as IReportQueries r = insights; is not walked. Because IInsightsModule : IReportQueries, that conversion compiles without naming anything a guard reads.
  • The Platform hub. Cluckwork.Api.Hosting registers all three ports as public DI services (CluckworkFeatureServiceCollectionExtensions.cs:92-128). Any Platform helper may take them, and an adapter may take the helper.
  • Untyped lambdas, var, and reflection.

Mutants

Each mutant was an exact single-match replacement. Each ran under --no-build, because the walks read src/ at runtime, against 9 real-tree tests: AdapterReach, PeerContract, CouplingMatrix ×N, ModuleLedger and CompatibilityException. Each ran once with the head ledger and once with main's ledger copied into bin/, then was reverted. Baseline and final runs were both 9/9 green, and git status was clean afterwards.

# Mutant Head ledger main ledger Verdict
PR #2 ReportEndpoints.Sales takes IReportQueries red: AdapterReach + Matrix, contract bypass … ReportEndpoints.Sales -> Insights through …IReportQueries at …ReportEndpoints.cs:90 green reproduced
PR #3 CreateExpenseHandler ctor takes IAuditEventRepository (no edge added) red: PeerContract, contract bypass …CreateExpenseHandler.ctor -> Insights through …IAuditEventRepository at …CreateExpenseHandler.cs:16 (plus edge/matrix) PeerContract green; only edge/matrix red reproduced: the PR is what makes the peer check red
own M5b ExportEndpoints.ExportDataset resolves services.GetRequiredService<IExportQueries>(). This adapter already declares Insights reach, so the #846 owner ratchet stays quiet red: AdapterReach + Matrix, …ExportDataset -> Insights through …IExportQueries at …ExportEndpoints.cs:38 green the service-resolution path is enforced too
own M6 ReportEndpoints.Sales takes the concrete Cluckwork.Infrastructure.Insights.InsightsModule red: through Cluckwork.Infrastructure.Insights.InsightsModule green Infrastructure.Insights resolves to Insights, not to the Platform hub
own M5 ListAccountsCliCommand resolves IExportQueries red red, as undeclared adapter reach from #846 not isolating; replaced by M5b
own F1 Ledger also lists IReportQueries, plus PR #2's mutant green, 10/10 (including ModuleContractRealAssemblyTests) — see F1

Full Application suite at head: 702 passed, 0 failed (/tmp/review-1040-opus-test.log), the same count the PR reports.

Findings

F1 — P3 — the base interfaces' exclusion is load-bearing, but nothing records or guards it

  • Location: tests/Cluckwork.Application.Tests/Architecture/Data/module-ledger.json:167-179, which is the Insights contract. The rule paragraph is src/AGENTS.md:27.
  • Defect: IInsightsModule inherits IReportQueries and IExportQueries. Their members are part of the contract surface: ModuleContractRealAssemblyTests walks base interfaces (SeamSurfaceScanner.cs:99-105). But the two types must stay off the ledger list, or adapters may inject them and skip the facade. The PR body is the only place that says so, and its wording is slightly wrong: it calls them "implementation ports, not the contract", yet their members are the contract. Only IAuditEventRepository is purely an implementation port. JSON allows no comment, and neither src/AGENTS.md nor a decision record mentions the exclusion.
  • Scenario: A later editor sees IInsightsModule listed and its base interfaces missing, and "completes" the list. Every guard stays green, and endpoints can again inject IReportQueries. That resolves straight to ReportQueries, which is exactly the bypass this PR closes.
  • Evidence: Mutant F1 above: the ledger plus IReportQueries together with PR mutant Fix egg-farm spec gaps: hen-day math, egg unit conversion, regrade path #2 gives 10/10 green.
  • Remedy (slice-sized): Add one clause to the [C] #514 slice 7: Finance pilot — first module behind a contract #849 paragraph in src/AGENTS.md: "Insights' IInsightsModule inherits IReportQueries and IExportQueries; keep both off the list, or adapters inject them directly." The structural fix is to stop inheriting and declare the members on IInsightsModule, which makes the surface and the nameable list the same thing. That is a src change and belongs to a later [C] #514 slice 16: Platform composition, jobs, CLI and seeder conversion #858 step, not P1.
  • CONFIRMED.

F2 — P3 — "registered beside them" is now false for one of the seven named modules

  • Location: src/AGENTS.md:27: "implemented by a <Module>Module that delegates to the existing handlers and repositories and is registered beside them … Finance, Farm, …, Commerce and Insights are contracted today."
  • Defect: InsightsModule is registered inline at src/Cluckwork.Api/Program.cs:78. Its three ports are registered in src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs:92-128, which is where IFinanceModule, IFlockModule and IInventoryModule are registered (lines 318-341).
  • Scenario: A reader follows the rule to find Insights' registration and looks in the wrong file. This has no runtime effect.
  • Evidence: grep shown above. The design's P7 already plans to move the inline Insights registration out of Program.cs.
  • Remedy: None needed if P7 lands. Otherwise, add "(Insights: Program.cs, until [C] #514 slice 16: Platform composition, jobs, CLI and seeder conversion #858 P7)".
  • CONFIRMED (the fact). Low value.

2. Code quality

pstack:thermo-nuclear-code-quality-review

There is no structural regression. The diff is data and prose: one ledger owner expanded to the same multi-line contract shape as the six other contracted owners, one clause appended to a sentence, and one sentence edited. No file crosses a size threshold. The ledger grows from 1801 to 1814 lines; it is a pre-existing data file, not code. src/AGENTS.md:27 is a 6077-character single line, but this PR does not cause that.

The only boundary concern is F1. The contract has two notions that differ for Insights alone: the surface the contract walk checks, which includes the inherited ports, and the types adapters may name, which excludes them. The code-judo move is to drop the inheritance from IInsightsModule and declare its 9 members directly. The two notions would then match, the exclusion would need no explanation, and InsightsModule's pure delegation would read like every other module's. It is out of P1's no-src scope, so it is rated P3 and recommended for a later #858 step.

I considered one more move and rejected it: deleting the facade and listing the three ports as the contract. InsightsModule is a 9-member identity wrapper, but #849 defines a contract as one I<Module>Module. Deleting it would make Insights the one module without a facade, and that adds a special case instead of removing one.

ponytail:ponytail-review

module-ledger.json:L169-178: considered yagni on the 8 records that no adapter names today. Rejected: they follow #849's "interface, its commands and its result records" pattern, which Farm (FarmBrandingHashes) and Finance (ExpenseListPage) already follow. Cutting them would make Insights the exception and save nothing at runtime.

Lean already. Ship.

3. Docs

Product defect

None.

Verdict: approve. The ledger contract is correct and enforced as claimed; two optional P3 doc notes (F1 worth one clause).

@mforce

mforce commented Oct 3, 2026

Copy link
Copy Markdown
Owner Author

Review of record (Codex Astra, xhigh) at head 8e7e2bc5. Posted by the coordinator after merge; the nit goes to #858 P2.

PR #1040 review

Reviewed 8e7e2bc598b64a5a5dc102ba2daa012d2883561a against 02e8a6333bbc18cf80ba20e4e64dae39dea52ab9, directly and without delegation. The supplied diff matches those commits: three files, +16/-3, with no production-code change.

Findings

No code defects or blocking findings. No product defect was found. The change declares the existing Insights API without changing runtime behavior.

Contract list

tests/Cluckwork.Application.Tests/Architecture/Data/module-ledger.json:167 is exactly right: IInsightsModule plus ten result records, with no missing or extra entry.

Results Connection to the facade
AuditEventRead, EntityProvenance Audit-list elements and provenance dictionary values
ExportDataset GetDataset result
ProductionReport, ProductionDay, GradeTotal Production result and its two nested collections
SalesSummary Sales result
ExpenseSummary, ExpenseCategoryTotal Expense result and its nested collection
ProfitReport Profit result

My earlier three-type probe checked today's adapter parameters. It did not establish the complete result inventory required by #849. The eight additional records are AuditEventRead and the seven report records. Keeping them is correct.

The inherited IReportQueries and IExportQueries signatures do not require permitting those ports as independent injection types. They and IAuditEventRepository are correctly absent. No listed record exposes an entity or aggregate. I also inspected all 20 export mappings because ExportDataset.Rows contains object arrays that reflection cannot validate at runtime; current mappings emit scalar values, not entities.

Detailed declarations and ownership evidence are in the contract and quality audit.

Mutations and tests

Full Application suite: 702 passed on untouched head and 702 passed after restoring the principal mutations. Every ledger mutation was rebuilt; the runner checked the build-output ledger against the source.

The selected architecture filter ran 18 real-tree/model/assembly checks, including adapter, peer, contract, compatibility and matrix guards. These are my 18 checks, not a reproduction of the author's exact 22-test selection.

Mutant Head Main's ledger
PR #1: remove EntityProvenance 2 failures; adapter and matrix guards name the five response helpers Not run
PR #2: sales endpoint takes IReportQueries 2 failures; adapter and matrix guards 18 passed
PR #4: AuditEventRead gains AuditEvent? Leak 1 failure; contract guard follows the leaked aggregate 18 passed
New: add IReportQueries to the contract All 702 Application tests passed Not applicable

The new mutant demonstrates a documented limit, not a defect in this PR. A DTO-only port passes structural validation, so review must prevent an overly broad allow-list. The actual list passes that review.

Two additional probes confirmed that the pre-existing Insights endpoint and DTO guards reject mutants #2 and #4 with main's ledger. After restoring those probes and rebuilding, both assertions and the contract guard passed, 3/3. The green main results above apply only to the selected architecture checks.

See commands, scripts and logs. No integration suite ran. All mutations were restored, the checkout was clean, and the requested review worktree was removed.

Wording

src/AGENTS.md:27 correctly adds Insights to the contracted modules. docs/decisions/1023-peer-contract-guard.md:58 correctly leaves Access as the sole uncontracted module at this head. Neither changed statement overclaims coverage. The documented limits concerning parameter/service-resolution checks, Platform, and statically discoverable result types still apply.

Nits

  • P3, CONFIRMED. PR description, opening explanation; evidence at tests/Cluckwork.Application.Tests/Architecture/InsightsReadOnlyTests.cs:58 and :42. The description implies endpoints using the three ports previously passed CI. The existing endpoint guard already rejects those references, and the existing DTO guard already rejects the aggregate-leak mutant. A maintainer reading the prose as a claim about the whole suite would misunderstand what this PR adds. Both failures were reproduced with main's ledger. Say that the shared ledger guards now reject the bypasses, extending enforcement to the other adapters and peers. The mutation table's explicitly scoped results remain valid.

Code quality

Applied ponytail:ponytail-review and pstack:thermo-nuclear-code-quality-review. The existing registry is the right place for this declaration. No new machinery or structural regression; no useful deletion. The ledger grows from 1,801 to 1,814 lines, so no file crosses the 1,000-line threshold.

Ponytail review: Lean already. Ship.

Verdict: APPROVE, with one minor PR-description nit.

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.

1 participant