Repository navigation
refactor(accounts): move the cross-farm account reads behind IFarmDirectory - #1043
Conversation
…ectory list-accounts, the farm-code lookup and the daily-entry lock sweep each read Accounts with the tenant filter off, directly through AppDbContext. They now call Farm's new IFarmDirectory, which AccountRepository implements with the same three queries, moved verbatim. A scratch SQL trace of all three call sites is identical before and after. That deletes three #850 compatibility rows. The directory is on Farm's contract, so the ledger guards would let any endpoint take it. FarmDirectoryCallerTests therefore allows only the operator verbs, the jobs and the directory's own files to name it or AccountRepository. ListAccounts_PrintsFarmsInSlugOrder pins the ordering list-accounts promised but nothing checked. Refs #858
…directory # Conflicts: # tests/Cluckwork.Application.Tests/Architecture/Data/coupling-matrix.md
|
Independent security and code-quality review (Claude Opus 5.5: security-review, thermo-nuclear and ponytail) at head PR #1043 security review: Opus 5.5, independent
Bottom lineThere is no product defect and no security defect in this PR.
The findings are about how strong the new guard is, plus comment and doc drift.
FindingsF1: P2 (guard soundness). The caller guard checks naming, not reach, and the comments overclaim. CONFIRMED
F2: P3. The allow-list is keyed by path, not by what the code declares. CONFIRMED
F3: P3. Two cross-farm slug reads with opposite contracts on one class. CONFIRMED (no live defect)
F4: P3 (design). The cross-farm capability lives on the tenant-scoped repository. Suggestion
F5: P3. The ordering pin depends on heap layout. PLAUSIBLE
F6: P3. Doc and comment drift. CONFIRMED
Context: pre-existing, outside this slice, not a finding against the PR
Verbatim and invariant checks (all CONFIRMED by reading head)
Mutation resultsGuard set for the unit mutants:
Every mutant was reverted; security-review skill outputThe skill's own git context was empty: it read my session's worktree, not the review checkout. I applied its method to the PR diff directly, and without subagents, as the brief requires. Candidates and confidence:
Result: no HIGH or MEDIUM vulnerabilities with confidence ≥ 8. pstack:thermo-nuclear-code-quality-review (P3, this slice only)No structural regression, and no file approaches 1k lines (
ponytail:ponytail-review (P3, this slice only)
net: -7 lines possible. VerdictMerge. There is no product or security defect. F1's comment correction and the forwarder deletion SHOULD follow once #857 D lands; F2–F6 are P3. |
|
Review of record (Codex Astra, xhigh) at head PR #1043 security reviewReviewed head No product behavior defect or currently reachable cross-farm HTTP disclosure was found in the unmodified PR. There is one confirmed defect in its security regression guard. The query move preserves behavior, but the guard can miss an ordinary direct reference in code compiled into the application. FindingF1. P2, CONFIRMED. Parse the caller guard with the application's framework symbols. Location: The new Failure scenario: an existing Farm-reaching endpoint such as Evidence: I made exactly that temporary mutation. The caller, adapter, peer, tenant-bypass, module-ledger, compatibility-exception and coupling-matrix classes all passed. A scratch integration test built the API, seeded two farms, authenticated as the first farm's Owner, called Remedy: reuse Evidence files: mutant diff, seven guard results, HTTP proof source, HTTP proof result, same mutant rejected with shared parse options. Reproduction script. Query and scope parity
The queries are behaviorally identical, rather than literally identical source text, because two projections now construct records. I compared the supplied Caller boundary and reproduced mutations
I also tried a forwarding bypass absent from the PR's table. A new That forwarding example requires adding new code to an explicitly trusted file. It demonstrates the existing limitation of a name-based boundary and #846's non-transitive scan, rather than a leaking path already present in this PR. Review must still inspect new methods in allowed files. The guard is useful direct-reference protection after F1, not a runtime authorization boundary or a proof that arbitrary request code cannot reach the directory. I do not request a general call-graph scanner. Ledger and allowlists
#1042 merge overlapThey do not merge cleanly in either order. The only conflict is documentation. I ran Resolve the long rule paragraph by retaining this PR's Farm-directory/caller-guard wording and #1042's Insights base-port exclusion and temporary Code qualityThe strict maintainability pass found no new state machinery, speculative layers, branching growth or source file crossing 1,000 lines. The directory has three real consumers and moves ownership without duplicating the queries. Reusing the existing repository and retaining the original scope boundaries is appropriate. F1 should reuse the existing parser configuration rather than add another parser or symbol registry.
NitsNone requiring changes. The forwarding limitation above should be remembered when describing the guard's guarantee. VerificationAll review mutations and the scratch HTTP test were reverted before clean-head validation. Nine targeted Application classes passed, 17 tests total: the caller, adapter, peer, tenant-bypass, module-ledger, compatibility-exception, coupling-matrix, module-contract and seam guards.
The API and affected test projects rebuilt successfully. Every test invocation selected one class, integration commands used The assigned detached checkout was clean and has been removed with the requested Verdict: REQUEST CHANGES for the confirmed P2 caller-guard defect; the production query move preserves behavior. |
…s symbols FarmDirectoryCallerTests parsed source with no preprocessor symbols, so a reference under `#if NET10_0` was invisible to it while the compiler built it (Astra's m10 leaked every farm through GetSettings past every guard). It now reuses ModuleLedgerScanner.ParseOptions, with a fixture case for a watched name inside an active framework conditional. The guard and IFarmDirectory comments now say what the guard holds: it keeps the name out of request code, but does not follow a forwarder such as AccountSlugLookup.ResolveAsync, which #858 P8 deletes after #857 D. The 851 record gains an amendment note, and the slug-order test compares the whole printed list with the database's own ORDER BY, reading only farm rows from stdout. Refs #858
|
Reply to the Codex Astra review, fixed in F1 (P2): fixed.
Other guards that parse real
The M7 (forwarder in an allowed file): agreed, it is a limit of a name-based guard. The comments now say so, and the one forwarder that exists today, #1042 overlap: noted in the PR body. Whichever PR lands second keeps both After the fix round: Application 704 of 704; |
|
Reply to the Claude Opus review, fixed in
|
…directory # Conflicts: # src/AGENTS.md
…directory # Conflicts: # tests/Cluckwork.Application.Tests/Architecture/Data/coupling-matrix.md
Nothing changes for users or operators.
list-accounts, the farm-code lookup behindseed --farm-code,suspend-account,reactivate-accountandrename-account, and the daily-entry lock sweep send the same SQL and print the same output.For maintainers, the three cross-farm reads of
Accountsthat sat in a CLI verb, a CLI helper and a job now live in Farm, behind a new contract interface,IFarmDirectory. Three #850 compatibility rows go with them. A new guard keeps request paths away from the directory, because each of its reads ignores the tenant filter.This is P3 of #858's approved plan (plan comment). It was built on
mainatfd8960e4(#1039's query-shape rule), thenmainat75e2b1be(#1037, #857 B) was merged in. That merge conflicted only in the generatedcoupling-matrix.md, which I regenerated: it differs frommainonly in the Farm adapter cell this PR changes. The Application suite is 703 of 703 after the merge.Refs #858
Fix round (
c022ab28)Tests, comments and one decision-record note; no production change.
FarmDirectoryCallerTestsparsed source with no preprocessor symbols, so#if NET10_0code was invisible to it. It now reusesModuleLedgerScanner.ParseOptions, with no second symbol list.IFarmDirectorycomments now say what the guard actually holds: it keeps the name out of request code, but does not follow a forwarder in an allowed place. Today the forwarder isAccountSlugLookup.ResolveAsync.GetRequiredService<IFarmDirectory>().FindIdBySlugAsyncinto the four verbs once [C] #514 slice 15: Access contract — security hold point, runs last #857 D has moved them.AccountRenameService's comment no longer cites the moved call site.docs/decisions/851-farm-contract.mdgains an amendment note pointing at this PR, so it no longer contradictssrc/AGENTS.md. P8 records the final state.ORDER BY "Slug", keeping only three-column farm rows from stdout (which also carries the host's JSON log lines). A droppedORDER BYnow fails on any inversion, not only on the seeded pair. M2 is red again.GuardScanner.cs:157,:380,:816(tenant bypass);InsightsReadOnlyTests.cs:65;EggLotLockSqlTests.cs:25;TransactionDelegateShapeTests.cs:109;ExportSnapshotSourceTests.cs:41.FindIdBySlugAsync, so both slug lookups share one contract.FarmDirectory(AppDbContext)type, soAccountRepositorystops being a watched name.IAccountRepository.FindBySlugAsyncis a cross-farm read on Farm's seam that no caller guard covers.src/AGENTS.mdparagraph. Whichever lands second keeps both edits: this PR's Farm-directory sentence, and refactor(repositories): delete the generic repository base and its unused members #1042's Insights base-port andProgram.cswording.What changed
IFarmDirectory(Application.Features.Accounts) has three members, one per former call site. Each query moved verbatim:ListTimeZonesAsyncDailyEntryLockSweep.RunAsyncListAsyncListAccountsCliCommand.RunAsyncORDER BY "Slug"FindIdBySlugAsyncAccountSlugLookup.ResolveAsyncThe results are two records,
FarmTimeZoneandFarmListing, so no entity crosses the contract. The anonymous projections became record projections; the SQL is unchanged (trace below).AccountRepositoryimplements it. It is already a listed Farm implementation, so the ledger needs no newimplementationsentry. It is registered as scoped. The sweep is a singleton, so it keeps resolving the directory inside its existing enumeration scope and never takes it in its constructor.Callers.
list-accountskeeps its sanitization and output format.AccountSlugLookupkeeps its signature andNormalize. OnlyResolveAsync's body changed, which keeps the hunk [C] #514 slice 15: Access contract — security hold point, runs last #857 D also touches small.DurableJobWorker,PostgresLeaderLeaseandAddCluckworkJobsare untouched (Background worker has no single-runner guarantee — double-runs if scaled >1 instance #271).Ledger.
contractgainsIFarmDirectory,FarmTimeZoneandFarmListing.compatibilityExceptionsrows withdeleteWhen: #858are deleted (35 → 32).Farm:AccountSlugLookup.ResolveAsync,ListAccountsCliCommand.RunAsyncandDailyEntryLockSweep.RunAsync. The matrix cell Platform/adapters → Farm goes fromA (17)toA (20).Tenant-bypass allow-list. The three
IgnoreQueryFilterssites moved, so their rows moved: three newAccountRepositoryrows, and four stale ones deleted. The fourth,SuspendAccountCliCommand.RunAsync, was flagged only as a same-file caller ofAccountSlugLookup, which is no longer a forwarding wrapper. The allow-list is keyed by symbol and file, so no token hash changes.src/AGENTS.md. The Farm paragraph namesIFarmDirectoryand the new guard.The new guard, and why it is needed
On
mainan endpoint cannot list every farm without adding anIgnoreQueryFilterssite, whichTenantBypassRealTreeTestsmakes someone justify. Putting the directory on Farm's contract would remove that friction: the ledger guards let any adapter take a contract type, and the directory's own sites are already allow-listed. SoFarmDirectoryCallerTestswalks every file undersrc. It fails when a file outsideApi/Cli/andInfrastructure/Jobs/, other than the directory's declaration, implementation and registration, namesIFarmDirectoryorAccountRepository. It is name-based likeInsightsReadOnlyTests, so an alias or reflection evades it; it says so. P5 extends its allow-list to the seeders, and P7 to the new registration file.A pin that did not exist
The plan promised to keep
list-accounts' ordering, but no test checked it.ListAccounts_PrintsFarmsInSlugOrderrenames two seeded farms so their heap order is the reverse of their slug order, then asserts the printed order. DroppingORDER BYturns it red (M2).SQL trace
A scratch integration test (not committed) recorded every statement the three call sites send, on
mainfd8960e4and on this head, against a fresh database with one extra seeded farm:RunAsync, through a command interceptor;AccountSlugLookup.ResolveAsyncfor a real code and an unknown one, through a command interceptor;list-accountssubprocess, with EF's command log raised to Information.diff trace-main.txt trace-head.txtprints nothing. Head output:Change map
Files #857 also touches:
SuspendAccountCliCommand.cs. [C] #514 slice 15: Access contract — security hold point, runs last #857 D moves the verb ontoIAccessOperations. This PR changes only theusinglines and theAccountSlugLookupclass at the end of the file. If D lands first, I rebase this over it; if this lands first, D's rebase meets oneusinghunk.module-ledger.json,coupling-matrix.mdandtenant-bypass-allowlist.json. Whichever PR merges second regenerates the matrix.src/AGENTS.md. This PR edits the Farm sentence, which [C] #514 slice 15: Access contract — security hold point, runs last #857 does not.Mutation table
Each mutant was an exact single-match replacement in this head, run against the named classes, then reverted. The script and logs are under
858-logs/p3/on the worker host. Integration mutants ran with #1039's query-shape initializer on.ListTimeZonesAsynckeeps only active farms (.Where(a => a.IsActive))DailyEntryAdjustTests.LockSweep_StillLocksEntries_ForASuspendedFarmAssert.Equal() Failure: Values differExpected: LockedActual: SubmittedListAsyncdrops.OrderBy(a => a.Slug)ListAccountsCommandTests.ListAccounts_PrintsFarmsInSlugOrder(new)expected order-e7e46d9c4a70-aa before order-e7e46d9c4a70-zzFindIdBySlugAsyncdrops itsWhereand answers the first rowSeedCommand_Demo_WithUnknownFarmCode_ExitsNonZeroAndNamesListAccounts;UnknownSlug_ExitsOne_AndChangesNothingforsuspend-accountandreactivate-accountAssert.Contains() Failure: Sub-string not found;Expected: 1Actual: 0ListTimeZonesAsyncdropsIgnoreQueryFilters()DailyEntryAdjustTests.LockSweep_*testsExpected: "Locked"Actual: "Submitted"AccountEndpointsgains a helper takingIFarmDirectoryFarmDirectoryCallerTests(new), andAdapterReachRealTreeTestsbecause the helper is a new memberThese files name IFarmDirectory or AccountRepository outside the operator verbs and jobs. … src/Cluckwork.Api/Endpoints/Accounts/AccountEndpoints.csAccountEndpoints.GetSettings, which already reaches Farm, also takesIFarmDirectoryFarmDirectoryCallerTests; adapter, peer and tenant-bypass guards stay greenAccountEndpointsgains a helper taking the concreteAccountRepositoryFarmDirectoryCallerTests(Infrastructureis the free Platform hub to the ledger)GetSettingstakesIFarmDirectoryand returns every farm, inside#if NET10_0c022ab28:FarmDirectoryCallerTestsnamesAccountEndpoints.cs. At991b1164seven guard classes were green and an Owner'sGET /account/settingsreturned every farm (Astra's HTTP proof)These files name IFarmDirectory or AccountRepository outside the operator verbs and jobs. … src/Cluckwork.Api/Endpoints/Accounts/AccountEndpoints.csModuleLedgerScanner.ParseOptionsagainANameInsideAnActiveFrameworkConditional_IsSeen:Assert.True() FailureExpected: TrueActual: FalseListAsyncdrops.OrderBy(a => a.Slug), against the hardened ordering testAssert.Equal() Failure: Collections differExpected: ["default-farm", "order-d6b7975ddbfa-aa", "order-d6b7975ddbfa-zz"]Actual: ["default-farm", "order-d6b7975ddbfa-zz", "order-d6b7975ddbfa-aa"]ListAccountsCommandTests+SchemaDocsTests12 of 12M5b and M6 are why the new guard exists: without it, either change passes every other guard.
Test counts
mainfd8960e457a1946675e2b1be(991b1164)Cluckwork.Application.Tests(full)FarmDirectoryCallerTests)ListAccountsCommandTests,AccountLifecycleCommandTests,DailyEntryAdjustTests.LockSweep*,OneShotVerbMinimalConfigTests,CliDispatcherTests,DurableJobWorkerLeaderGateTests,PostgresLeaderLeaseTests, the threeSeedCommandTestsfarm-code cases,QueryShapeWarningTests,SchemaDocsTestsListAccounts_PrintsFarmsInSlugOrder)The 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. Thesrcdiff moves three queries and deletes their inline copies. Each new comment states a reason the code cannot show:The comments that described the old inline
IgnoreQueryFilterscall sites were rewritten to point atIFarmDirectory. The three callers lose theirMicrosoft.EntityFrameworkCoreusing. The sweep keepsInfrastructure.PersistenceforTenantContext.Try to refute
mainand at head.FarmDirectoryCallerTestsis the only thing that says so, and it is name-based.AppDbContextbefore.