Repository navigation
test(arch): declare the Insights module contract in the ledger - #1040
Conversation
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
|
Independent review (Claude Opus 5.5: correctness, thermo-nuclear and ponytail) at head PR #1040 review: declare the Insights module contract (#858 P1)Reviewer: Claude Opus 5.5, independent of Codex Astra. Head No product defect. The PR changes no 1. CorrectnessIs the list exactly the interface plus every result record?Yes.
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, The design checkpoint named only 3 types: the interface,
Can an adapter or peer still reach Insights' internals?The checked paths are closed. Parameter injection, service resolution, a peer constructor, and the concrete
MutantsEach mutant was an exact single-match replacement. Each ran under
Full Application suite at head: 702 passed, 0 failed ( FindingsF1 — P3 — the base interfaces' exclusion is load-bearing, but nothing records or guards it
F2 — P3 — "registered beside them" is now false for one of the seven named modules
2. Code qualitypstack:thermo-nuclear-code-quality-reviewThere is no structural regression. The diff is data and prose: one ledger owner expanded to the same multi-line 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 I considered one more move and rejected it: deleting the facade and listing the three ports as the contract. ponytail:ponytail-review
Lean already. Ship. 3. Docs
Product defectNone. Verdict: approve. The ledger contract is correct and enforced as claimed; two optional P3 doc notes (F1 worth one clause). |
|
Review of record (Codex Astra, xhigh) at head PR #1040 reviewReviewed FindingsNo code defects or blocking findings. No product defect was found. The change declares the existing Insights API without changing runtime behavior. Contract list
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 The inherited Detailed declarations and ownership evidence are in the contract and quality audit. Mutations and testsFull 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.
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
Nits
Code qualityApplied Ponytail review: Lean already. Ship. Verdict: APPROVE, with one minor PR-description nit. |
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.InsightsReadOnlyTestsalready kept endpoints offIReportQueries,IExportQueriesandIAuditEventRepository(: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 toReportQueries,ExportQueriesandAuditEventRepositoryand 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.contractlistsIInsightsModuleand its ten result records:AuditEventRead,EntityProvenance,ExportDataset,ProductionReport,ProductionDay,GradeTotal,SalesSummary,ExpenseSummary,ExpenseCategoryTotalandProfitReport.*Detailsresult records"). It also means a future helper that takes, say, aSalesSummaryparameter needs no ledger edit.IReportQueries,IExportQueriesandIAuditEventRepositoryare left out on purpose.IInsightsModuleinherits 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.IAuditEventRepositoryis 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
Files #857 also touches:
module-ledger.json. P1 edits only the Insights owner line, far from the Access rows refactor(users): put user administration behind IAccessModule and peer reads behind IAccessLookup #1037 changes. Whichever PR merges second takes the other's hunk. The matrix is unaffected.src/AGENTS.md. [C] #514 slice 15: Access contract — security hold point, runs last #857 E will add Access to the same "contracted today" sentence.1023-peer-contract-guard.md. [C] #514 slice 15: Access contract — security hold point, runs last #857 E should delete the remaining "Today that is Access" sentence when it declares Access.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 under858-logs/on the worker host.mainledgerEntityProvenancefrom the contractAdapterReachRealTreeTests,CouplingMatrixRealTreeTestscontract 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 SaleToResponsehelpers)ReportEndpoints.SalestakesIReportQueriesinstead ofIInsightsModuleAdapterReachRealTreeTests,CouplingMatrixRealTreeTestscontract bypass Cluckwork.Api.Endpoints.Reports.ReportEndpoints.Sales -> Insights through Cluckwork.Application.Features.Reports.IReportQueries at src/Cluckwork.Api/Endpoints/Reports/ReportEndpoints.cs:90CreateExpenseHandlergains a helper takingIAuditEventRepository, with a declaredFinance -> Insightsedge so the edge ratchet stays quietPeerContractRealTreeTests,CouplingMatrixRealTreeTestspeer 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:18AuditEventReadgains anAuditEvent? Leakmember (a Platform aggregate, which #847 allows on ordinary seams)ModuleContractRealAssemblyTestsmodule contract guard failed: Cluckwork.Application.Features.Insights.AuditEventRead.op_Equality exposes Cluckwork.Domain.Auditing.AuditEvent via AuditEventRead -> AuditEventRead.LeakRejected mutant shapes, for the record:
CS9113).Flockleak also trips the peer and edge guards onmain, so it does not isolate the contract check.Entity<Guid>is already caught onmainby [B] #514 slice 5: seam-surface guard — no persistence type crosses a seam #847's seam guard.Test counts
main02e8a633Cluckwork.Application.Tests(full)SchemaDocsTestsimage-pin guards (walk every tracked file)No integration suite is needed, because no runtime code changed. CI runs everything.
Deslop record
/deslopovergit diff origin/mainfound nothing to remove. The diff is the ledger block and two edited sentences of prose. No comments were added.Try to refute
git diff origin/main --stat -- src ':!src/AGENTS.md'is empty.ProductionReportfromIInsightsModulewas never a bypass, before or after.Description corrected after merge, from the review nits by Codex Astra and Claude Opus.