Repository navigation
refactor(repositories): delete the generic repository base and its unused members - #1042
Conversation
IInsightsModule inherits IReportQueries and IExportQueries. Listing either in the ledger would let any adapter or peer inject it past the ledger guards and skip the facade, so the rule paragraph now says to keep both off. It also notes that the Insights facade is registered in Program.cs until #858 P7, not beside its handlers. Review findings F1 and F2 on #1040. Refs #858
…used members IRepository<T,TId> forced every module repository to expose GetByIdAsync, AddAsync, Update and Remove. The compiler, run over the whole solution, shows which ones callers use: AddAsync on all 15 ports, GetByIdAsync on 11, and Update only on IFlockRepository. Each port now declares just those, and the implementations and test fakes lose the members nothing called. That removes 12 unreviewed Remove paths and 11 Update paths on aggregates whose rules say void, never delete, and the three append-only ledgers no longer need throwing stubs: their ports simply have no Update or Remove. TrackedMutationReadTests found its repositories by their Update and Remove methods, so it now also keys on AddAsync. It walks 18 repository files instead of 16 (the catalog and egg-movement repositories join), and four append-only ledgers with no single-entity read are excluded by name. The seam fixture keeps its inherited-generic case with a test-local IStore<T,TId>, and the two assembly anchors use IUnitOfWork. Refs #858
|
Independent review (Claude Opus 5.5: correctness, thermo-nuclear and ponytail) at head PR #1042 review (Opus): delete
|
| Guard | Verdict | Evidence |
|---|---|---|
SeamSurfaceRealAssemblyTests anchor → typeof(IUnitOfWork).Assembly |
Kept | 2/2 green. The ≥30-interface floor still holds, which shows the anchor names the Application assembly. |
ModuleContractRealAssemblyTests anchor |
Kept | 1/1 green. |
SeamSurfaceTests repository-in-result fixture (test-local IStore<T,TId>) |
Kept | 39/39 green. The scanner reaches IFlockStore only through resolved.GetInterfaces() (SeamSurfaceScanner.cs:318), because Type.GetMethods() on an interface omits inherited members. The fixture still exercises that inherited-interface branch, and the shape is structural, not name-based. |
AdapterReachTests:447 generic-argument test |
Kept | 97/97 green. The test is a syntax walk, so the container type was never bound to anything. The rename is cosmetic. |
TrackedMutationReadTests.AllMutableRepositoryReads_AreTracked |
Kept, wider than before | See the walk below. |
Ledger, matrix and peer guards (ModuleLedgerRealTreeTests, CouplingMatrixRealTreeTests, PeerContractRealTreeTests, InsightsReadOnlyTests) |
Unaffected | All green. |
The tracked-read walk. I replayed the guard's own regexes in Python over git show <ref>:<file> for both refs (/tmp/opus1042_walk.py):
maindiscovers 16 files.- Head discovers 18 files: the same 16 plus
CatalogRepositories.cs(Product) andEggInventoryMovementRepository.cs. - Reads. Every read checked on
mainis still checked at head, except the 4GetByIdAsyncmembers this PR deletes (bird movement, feed usage, inventory lot, inventory movement).ProductRepository.GetByIdAsyncis newly checked.
The PR's "superset" claim is CONFIRMED.
Mutants
Each mutant replaced exactly one match, ran the single test method, and was reverted with git checkout. The tree was clean afterwards.
| # | Mutant | Result | Meaning |
|---|---|---|---|
| A2 (PR) | ProductRepository.GetByIdAsync → AsNoTracking() |
red: CatalogRepositories.cs: GetByIdAsync |
Reproduced. The new discovery covers it. |
| A3 (PR) | FeedUsageRepository gains a tracked GetByIdAsync |
red: FeedUsageRepository.cs is listed as an append-only ledger but now has a single-entity read (GetByIdAsync)… |
Reproduced. |
| M1 (own) | FeedUsageRepository gains a live public void Update(FeedUsage entity) => db.FeedUsages.Update(entity); and still has no read |
green | The list says "append-only" but never checks it. See F1. |
| M2 (own) | EggUnitConversionRepository.GetByIdAsync → AsNoTracking(). This read feeds UpdateEggUnitConversionHandler's write. |
green | A gap that predates this PR: the repository has no AddAsync, Update or Remove, so neither ref discovers it. See O1. |
Findings
F1: P3. The append-only exclusion verifies "no read" but not "append-only". CONFIRMED.
- Where:
tests/Cluckwork.Api.IntegrationTests/TrackedMutationReadTests.cs:108-114(the list) and:142-148(its branch). - Defect: A listed file is excused when it has no single-entity read. Nothing checks that it still has no
UpdateorRemove, which is the premise the list's name and comment state ("add rows but never read one back to change it"). - Scenario: Someone makes feed usage editable and adds
Update(FeedUsage)to the port and the repository. They feed it a stub built from the request instead of a read. That is the detached-write shape feat(tenancy): write-side tenant guard + single-assignment TenantContext (#546) #561 describes, and the guard stays green. - Evidence: Mutant M1 stays green.
- Not a regression, and low risk:
- On
main, the equivalent edit was also green, because those files had a trackedGetByIdAsyncthat satisfied the backstop. - Write guard trusts OriginalValue as DB provenance; detached Update/Remove can bypass the tenant theft check #562's
AccountIdconcurrency token, tested byDetachedTenantWriteTests, still refuses a cross-tenant detached write at the database.
- On
- Fix (about 2 lines): In the ledger branch, also assert that the file has no
public void Update|Remove. For example, assert thatentityTypescame only fromAddAsyncmatches. The list name then verifies itself.
F2: P3. A comment names the deleted type. CONFIRMED.
- Where:
src/Cluckwork.Application/Features/Sales/ISalesOrderAllocationRepository.cs:5. - Defect: The comment says
// Deliberately not IRepository: allocations are written as a batch…, and that type no longer exists. The diff made the comment stale but does not touch the file. - Fix: Reword the lead without the type name, for example "Deliberately has no single-entity Get, Update or Remove: …". Keep the rest.
F3: P3. The tech spec still lists IRepository as a port. CONFIRMED.
- Where:
specs/technical/tech_spec.md:46. - Defect: The line reads "port interfaces (
IRepository,IIdentityProvider,IClock,IUnitOfWork)". - Fix: Name per-feature repository ports instead, for example
IFlockRepository. - Also stale, but leave it:
docs/decisions/849-module-contract.md:72("as everyIRepository<T, TId>does"). It is a decision record and records history.
F4: P3. The guard's own comments still assume Update/Remove. CONFIRMED.
- Where:
TrackedMutationReadTests.cs:80-84. - Defect: The comment says the ReadOnly reads "are never fed to Update/Remove".
IDailyEntryRepositoryandISalesOrderRepositoryno longer have either member, so the reason has lost its subject. - Fix: Reword to "never fed to a write".
- Related:
:92-94("Most repositories lost their unused Update and Remove in [C] #514 slice 16: Platform composition, jobs, CLI and seeder conversion #858…") describes history, not the invariant. One clause saying thatAddAsyncnames the aggregate a repository owns would carry the same rule.
F5: P3. The squash mixes in an unrelated docs commit. PLAUSIBLE.
- Where: commit
e8e46067(src/AGENTS.md:10). - Defect: This commit holds the test(arch): declare the Insights module contract in the ledger #1040 Insights review fixes. On squash-merge it lands under the title
refactor(repositories): …, so the Insights rule change is not visible from the release note. - Mitigation: The PR body discloses the commit, and I checked its two claims:
Program.cs:78registersIInsightsModule, andIInsightsModule : IReportQueries, IExportQueries. - Options: Accept it as disclosed, or split it out. This is the owner's call.
Observations (not findings against this PR)
- O1: tracked-read coverage gap that predates this PR. CONFIRMED by M2.
- Gap: The guard keys on
AddAsync,UpdateandRemove. It therefore never seesEggUnitConversionRepository.GetByIdAsync, which feedsUpdateEggUnitConversionHandler.cs:16. If that read becameAsNoTracking, the handler would report success and silently persist nothing.ProductEggGradeMappingis also unseen, becauseAddMappingAsyncdoes not match the pattern. - Comment overreach: The rewritten header comment ("every repository that writes an entity") claims more than its parenthetical delivers.
- Follow-up option: Key on the
DbSet<T>entity types inAppDbContextinstead (see the thermo section). Measured cost: 4 untrackedAccountRepositoryreads that are not named ReadOnly would need exclusions or renames (GetCurrentAsync,GetCurrentSharedLockedAsync,GetCurrentLockedAsync,FindBySlugAsync). That is out of scope for a deletion slice.
- Gap: The guard keys on
- O2: no narrowed port is left with a single member. The smallest are
IFeedUsageRepository(AddAsync,ListAsync) andIInventoryMovementRepository(AddAsync,ListByItemAsync). Folding either into its module facade would read worse: it would put an entity (FeedUsage,InventoryMovement) on a contract, whichModuleContractRealAssemblyTestsrejects. The repo's single-member ports (IMortalityLedger,IDailyEntryLookup,IEggUnitConversionLookup) are deliberate peer ports from [C] #514 slice 10: Flock Management contract, eligibility and mortality ports #852, [C] #514 slice 11: Egg Operations contract and the opaque stock seam #853 and [C] #514 slice 12: Commerce contract, consuming the stock seam #854, and this PR did not create them. - O3:
IFlockRepository.Updateis redundant with change tracking. All 4 callers load through the trackedGetByIdAsync. DeletingUpdatewould still change SQL:DbSet.Updatemarks every property modified, so the UPDATE would go from all columns to changed columns only. Graph-attach semantics would change too. Keeping it is right for a slice that promises no SQL change. If anyone deletes it later, that belongs in its own PR with an SQL-trace parity check.
Code quality: pstack:thermo-nuclear-code-quality-review (P3, slice-scoped)
Verdict: a structural improvement with no regression. The PR removes a concept: a generic base that gave 15 ports four members whether or not anything used them. The net change is −69 lines. No file nears 1k lines (TrackedMutationReadTests.cs is 202). Production code gains no new branches.
Re-declaring the two members is the right shape, not duplication to extract. Each of the 11 surviving ports now spells out GetByIdAsync and AddAsync. Pulling those two lines back into a shared generic base would rebuild the abstraction this PR deletes, and would hand them back to the 4 ports that need only AddAsync. The per-port signatures are the honest contract.
Is the exclusion list a magic list? No. It is the boring, correct choice. The guard has one new branch, the ledger continue. The obvious alternative is to skip quietly any file that has AddAsync but no read. That would exempt exactly the files the anti-rot backstop exists to watch, and a pattern that later stopped matching would pass without notice. The list is the price of keeping the backstop. Each entry is checked mechanically in both directions:
- a listed file that gains a read fails (A3);
- a file dropped from the list fails through the backstop (PR mutant A4);
- a stale entry fails
Assert.All(..., Contains)(PR mutant A5).
The one weakness is that the name claims "append-only" without checking it (F1).
Code-judo move considered and deferred. The guard's real question is whether a persisted aggregate's single-entity read is tracked. It answers through a proxy: "this file has a mutation-shaped member". Keying directly on the DbSet<T> types in AppDbContext would make three pieces disappear: the mutates regex, the ledger list and its branch, and the backstop's special case. It would also close O1. It is not free, though. It needs a matching list of 4 untracked AccountRepository reads, and it widens the guard's scope. That suits a follow-up, not a deletion PR.
Comments. The new port comments on IBirdMovementRepository and IInventoryMovementRepository state a domain rule (corrections are Adjustment rows), so they earn their place. The test-local IStore comment says why the copy exists, which is also fine. F2 and F4 are the comment hygiene items.
Code quality: ponytail:ponytail-review (P3, slice-scoped)
tests/Cluckwork.Application.Tests/Architecture/SeamSurfaceTests.cs:L157-165: shrink.IStore<T,TId>mirrors all 4 members and both constraints, but the fixture only needs one inherited member that exposesT, for examplepublic interface IStore<T> { Task<T?> GetByIdAsync(Guid id); }. That saves about 6 lines. This is optional: the faithful copy keeps both parameter and return positions in play, and that is a defensible reason to keep it.tests/Cluckwork.Api.IntegrationTests/TrackedMutationReadTests.cs:L92-94: shrink. Drop the [C] #514 slice 16: Platform composition, jobs, CLI and seeder conversion #858 history sentence and keep the rule. About −2 lines.src/Cluckwork.Application/Features/Flocks/IFlockRepository.cs:L174 (diff): yagni, deferred.Updateis redundant with the tracked read (O3). Do not cut it in this slice, because doing so changes SQL.- Everything else is deletions, or contract lines that callers need. The append-only list is load-bearing, not speculative.
net: -8 lines possible. Lean otherwise.
Tests run (head 3117bdbd, one class at a time)
- Build:
dotnet build Cluckwork.sln: 0 warnings, 0 errors. - Application.Tests:
SeamSurfaceRealAssemblyTests: 2/2SeamSurfaceTests: 39/39ModuleContractRealAssemblyTests: 1/1AdapterReachTests: 97/97ModuleLedgerRealTreeTests: 2/2CouplingMatrixRealTreeTests: 2/2PeerContractRealTreeTests: 1/1InsightsReadOnlyTests: 23/23- Fake-carrying classes:
UpdateCustomerHandlerTests5,EggOperationsModuleTests9,FinanceModuleTests3,FlockLookupTests10,MortalityLedgerTests3,InventoryModuleTests6,CommerceModuleTests5. All green.
- Integration:
TrackedMutationReadTests: 2/2, run undersg docker. - Not run: the full Application suite and the full integration suite, because memory is limited. When I checked, CI's
Tests (integration)was still pending and every other check had passed.
Verdict
Approve. There is no product defect, every guard keeps its purpose, and the tracked-read walk is now a strict superset of main's. F1 to F4 are optional P3 polish: a 2-line self-check plus three stale comments and doc lines. F5 is a scope call for the owner.
|
Review of record (Codex Astra, xhigh) at head Reviewed PR #1042 at One P2 guard defect needs correction. No product defect found. The production deletion is consistent with its callers, and the restored head builds cleanly with 213 selected tests passing. Finding F1: P2, CONFIRMED. The failure scenario is a later change adding a destructive operation to one of these four ports and implementations. A caller could pass a row from the existing untracked list, or an attached stub, without introducing a guarded single-entity load. The new exclusion leaves the tracked-read backstop green despite the repository no longer being append-only. This is a guard defect, not an existing deletion path or a demonstrated tenant-isolation defect. I reproduced it with the unlisted N1 mutant: add Require an excluded file to have no recognized Deletion and guard coverage.
Mutation results. All mutations were reverted.
A2-head and A3-head ran the compiled head guard against mutated source with #1040 documentation fold-in. Nits, separate from F1.
Code quality. The strict maintainability pass found no additional structural regression. Production changes remove behavior from the ports and implementations without adding wrappers, dependencies or replacement abstractions. No changed C# file crosses 1,000 lines; the largest is the unchanged-length 853-line adapter fixture file. The test-local generic interface preserves measured coverage.
Final verification at the restored head: solution build 0 warnings, 0 errors; 180 Application tests and 33 integration tests passed, across 15 sequential class runs. Integration coverage was Verdict: REQUEST CHANGES for F1's append-only exclusion check; no current product defect found. |
…ate or Remove TrackedMutationReadTests excused the four named append-only ledgers once they had no single-entity read, but never checked that they were still append-only. A ledger that gained a real Update or Remove (Astra's N1) stayed green. The exclusion now also refuses a listed file that declares either, so its reads go back through the ordinary checks. Also fixes the stale IRepository mentions (Opus F2, F3) and the guard's comments that still assumed Update/Remove (Opus F4). Refs #858
|
Reply to the Codex Astra review, fixed in F1 (P2): fixed. In the append-only branch, Your N1 mutant is now red. I applied your diff (port, implementation, test fake) and reran the guard:
An Nit ( |
|
Reply to the Claude Opus review, fixed in
|
Nothing changes for users. No SQL statement, route or response changes: every deleted member had no caller.
For maintainers, the generic
IRepository<T,TId>is gone. It made all 15 module repository ports exposeGetByIdAsync,AddAsync,UpdateandRemove, whether or not anything used them. Each port now declares only what its callers use, so a handler can no longerRemovea sales order, a payment or an egg lot through its port. The append-only ledgers drop their throwing stubs: their ports simply have noUpdateorRemove.This is P2 of #858's approved plan (plan comment). It also carries the two #1040 review fixes as a separate docs commit.
Refs #858
Fix round (
39151656, onmain75e2b1bemerged in as0fbc3ca6)Tests and comments only; no production change.
UpdateorRemove. The writer regex gained a namededitgroup, so the check reuses the existing discovery data. The empty-read and stale-entry checks are unchanged. N1 and M1 below are red.ISalesOrderAllocationRepositorycomment andspecs/technical/tech_spec.mdno longer nameIRepository. The guard's comments say "never fed to a write" and state theAddAsyncrule instead of [C] #514 slice 16: Platform composition, jobs, CLI and seeder conversion #858 history.EggUnitConversionRepository.GetByIdAsyncfeeds a write, but the repository declares noAddAsync,UpdateorRemove, so the guard never sees it. The same goes forProductEggGradeMapping, whose writer isAddMappingAsync. This predates the PR and does not fit in one line of the guard, so it stays a follow-up.TrackedMutationReadTests,SchemaDocsTestsandQueryShapeWarningTests9 of 9.Commits
e8e46067docs(arch). These are the test(arch): declare the Insights module contract in the ledger #1040 review findings, docs only:src/AGENTS.mdnow saysIReportQueriesandIExportQueriesmust stay off Insights' contract list, and why.Program.csuntil P7.3117bdbdrefactor(repositories). The deletion, described below.How the members were chosen
The compiler did the search. I deleted
IRepository.cs, built the whole solution, and read everyCS1061error. Each error names a port member a caller still uses.IFlockRepositoryGetByIdAsync,AddAsync,Update(4 lifecycle handlers call it)ICustomerRepository,IDailyEntryRepository,IEggGradeRepository,IEggLotRepository,IExpenseCategoryRepository,IExpenseRepository,IInventoryItemRepository,IPaymentRepository,ISalesOrderRepository,IWaterUsageRepositoryGetByIdAsync,AddAsyncIBirdMovementRepository,IFeedUsageRepository,IInventoryLotRepository,IInventoryMovementRepositoryAddAsyncA codemod (
p2-prune.py, on the worker host) then deleted each implementation and test-fake member that its port no longer declares:Remove: 12 real deletes plus 3 throwing stubs.Update: 11 real plus 3 stubs.GetByIdAsync.The whole solution then built with 0 errors under warnings-as-errors, which proves nothing called a deleted member on a concrete type either.
Kept on purpose, per the design review:
IFlockRepository.Update.RemoveonUserRoleAssignmentRepositoryandFarmLogoRepository. Neither ever derived fromIRepository, so this PR does not touch them.The guard that had to change
TrackedMutationReadTests.AllMutableRepositoryReads_AreTrackedkeeps every repository read that feeds a write path tracked (#561). It found those repositories by theirpublic void Update/Removemethods. Those were mostly the dead members this PR deletes. Without a change, the guard fails closed:Expected to discover many mutable repositories, found 3: FarmLogoRepository.cs, FlockRepository.cs, UserRoleAssignmentRepository.cs.AddAsync. It walks 18 repository files instead of 16.CatalogRepositories.csandEggInventoryMovementRepository.csjoin, soProductRepository.GetByIdAsyncis now guarded too. Every read the guard checked onmainis still checked, apart from the four deadGetByIdAsyncthis PR deletes.ReadOnlyexclusion: bird movements, egg inventory movements, feed usage and inventory movements. They add rows but have no single-entity read. The exclusion fails if a listed ledger gains such a read, or if a listed file stops being discovered.Two other test edits keep their coverage:
SeamSurfaceTests. Its "repository inside a contract result" fixture now inherits a test-localIStore<T,TId>with the same four members.AdapterReachTests:447. Its syntax-only fixture string saysIStore<Flock, Guid>, so nobody reads it as the deleted production type. The test is about generic-argument resolution, and the name is irrelevant to it.The two real-assembly guards anchor on
typeof(IUnitOfWork).Assembly.Change map
Files #857 also touches: none of #1037's files.
SaleAllocationPolicyTests.csis on #857 B's required-green list. This PR deletes two forwarding lines (Update,Remove) from itsCountingEggLotRepositorydecorator and changes no assertion. If #857 measures "security suites unedited" against a baseline, take it after this PR or count those two lines as mechanical.Mutation table
Each mutant was an exact single-match replacement, run, then reverted. "main" runs used a detached checkout of
922b208d.mainCustomerRepository.GetByIdAsyncreadsAsNoTracking()(Customer lost itsUpdate/Removehere)Update)These repositories can mutate, but the read that feeds the write path opts out of change tracking. …CustomerRepository.cs: GetByIdAsyncProductRepository.GetByIdAsyncreadsAsNoTracking()CatalogRepositories.cs: GetByIdAsync(newly guarded)FeedUsageRepositorygains aGetByIdAsyncFeedUsageRepository.cs is listed as an append-only ledger but now has a single-entity read (GetByIdAsync). Remove it from the list.FeedUsageRepository.csdropped from the append-only listFeedUsageRepository.cs mutates [FeedUsage] but no tracked single-entity read was found for it. The guard's pattern no longer matches this file — fix the pattern, do not ignore it.RetiredLedgerRepository.csadded to the listAssert.Contains() Failure: Item not found in collection … Not found: "RetiredLedgerRepository.cs"SeamSurfaceScannerstops walking a result's inherited interfacesContract_RepositoryInAResult_IsAViolation:Assert.Single() Failure: The collection was empty. Same tests at head andmain, so theIStorefixture keeps the coverage.UpdateCustomerHandlercallscustomers.Remove(customer)error CS7036(noRemoveonICustomerRepository; the compiler finds only an unrelated extension)IFeedUsageRepositoryandFeedUsageRepositorygain a realRemove(FeedUsage)(and the test fake implements it)39151656; green at3117bdbd(Astra's run)FeedUsageRepository.cs is listed as an append-only ledger but now declares Remove. Remove it from the list so its reads are checked like any other mutable repository.FeedUsageRepositorygains a realUpdate(FeedUsage)39151656; green at3117bdbd(Opus's run)… now declares Update. …A1 on
mainwas not run; the guard discoveredCustomerRepository.csthere through itsUpdate.Merged
mainatfd8960e4(#1039)02293613mergesmain(not a rebase). #1039 changedInventoryItemRepository,InventoryLotRepository,SalesOrderRepositoryandAccountRepositoryfromFirstOrDefaultAsynctoSingleOrDefaultAsync. The merge was clean, and all five of #1039'sSingleOrDefaultAsynclines are kept. None of them sat in a member this PR deletes: theInventoryLotRepositorychange is inGetByIdLockedAsync, not the deadGetByIdAsync. After the merge I reran everything below with #1039's query-shape initializer on (a module initializer, so every integration test in that assembly throws on the listed EF warnings).Test counts
main922b208d3117bdbd02293613Cluckwork.Application.Tests(full)TrackedMutationReadTests,SaleAllocationPolicyTests,DetachedTenantWriteTests,FlockManagementTests,BirdMovementTests,FeedUsageTests,SchemaDocsTestsQueryShapeWarningTestsThe full integration suite was not run locally, to spare memory while #857 runs in parallel. CI runs it.
Deslop record
/deslopovergit diff origin/mainremoved nothing more. The diff is deletions, one-line re-declarations, the guard's discovery change, and comments that state a reason:IStorecomment;dotnet formatturned out not to flag IDE0005 here (a planted unusedusingpassed). The 14 removedusing Cluckwork.Application.Common;lines were checked by grep: none of those files names aCommontype any more.Try to refute
mainit checked 16 files' single-entity reads. At head it checks every one of those, except the four deadGetByIdAsyncthis PR deletes, plusProductRepository.GetByIdAsync.IFlockRepository.Updateis the onlyUpdatestill called, and it is kept. Handlers that change an entity load it through the trackedGetByIdAsyncand save throughIUnitOfWork, as before.