Skip to content

test(arch): make the scoped guards module-safe before the package-by-module move - #1088

Merged
mforce merged 7 commits into
mainfrom
refactor/1087-s0-module-safe-guards
Oct 5, 2026
Merged

mforce merged 7 commits into
mainfrom
refactor/1087-s0-module-safe-guards

Conversation

@mforce

@mforce mforce commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Refs #1087

S0 of the package-by-module design: it makes the architecture guards safe for the module moves before any file moves. No product file moves and no behaviour change. Everything is under tests/ and tools/, plus one sentence in src/AGENTS.md.

Review order

  1. efc6f3cf: the five scoped guards (small diffs, each one widens a walk).
  2. d8ed4d87 + ba6b2803: contract derivation from Contracts namespaces, and the contract scan now walks constructors.
  3. 04d4a227: the new folder/namespace rule.
  4. 68837a92: tools/module-move/move.py, the codemod the slices will use. Skim it; the trial below is its check.
  5. 673bc853: the three test-type renames (build: enforce file-scoped namespaces and using placement at build time #985).

Change map

Area Change
SeamSurfaceRealAssemblyTests walks the whole Cluckwork.Application assembly instead of Features + Common (today the same set; src/AGENTS.md #847 sentence updated)
RealModuleLedger.Adapters Cluckwork.Api.Modules added to AdapterRoots and to PersistenceForbiddenNamespaces
InsightsReadOnlyTests reads the whole Api project (floor 80 files); only InsightsModuleServiceCollectionExtensions.cs may name the read ports
TrackedMutationReadTests selects Repositories/ and Modules/<Owner>/Repositories/ and their subfolders, by folder and by namespace; the discovery equality, exclusions and floor are unchanged
ServingGuardCoverageTests walks Cluckwork.Api.Modules beside Cluckwork.Api.Hosting; a theory pins both roots
RealModuleLedger.DeriveContracts contract = [ModuleContract] mark ∪ top-level type in Cluckwork.{Domain,Application}.Modules.<Owner>.Contracts. Nested types are excluded. A type that has both is a registry error, so each slice deletes the mark when it moves the type. A fixture in the test assembly covers the new branch, since main has no Contracts namespace yet
SeamSurfaceScanner (contract mode) walks public constructor parameters. Without this, a handler moved into Contracts/ passed ModuleContractRealAssemblyTests, because its repositories appear only in its constructor (see N4 below). All 160 real contracts still pass
NamespaceFolderAgreementTests (new) every file in Domain, Application, Infrastructure and Api declares exactly its folder as its namespace. Exceptions: #1086's rules files (Domain/Common/Architecture/Modules → Cluckwork.Domain.Common.Architecture) and namespace-free files that declare no type, except Program in Api/Program.cs. Floor: 500 files. main already complies
tools/module-move/move.py (new) split (extracts declarations into <Type>.cs beside the file), move (git mv by a finite per-file table; the namespace comes from the folder; rewrites full and, in code only, partial qualified names, usings including extension-method callers, registry strings and paths), marks (deletes marks in Contracts/), ide0005 + prune (removes only the unused usings the tool introduced, read from SARIF). No restore input, lock file or CI change
#985 renames Farm → SeededFarm (two private test records); Contracts → NestedContracts (seam fixture class)

Counts (measured on origin/main 02c299a and on this head)

Run main head
Application.Tests, Architecture + TenantBypass + Documentation + ImagePin 461 474
ServingGuardCoverageTests 5 7
TrackedMutationReadTests, AccessFixtureTests, AccessLookupTests, SchemaDocsTests 2, 4, 6, 4 2, 4, 6, 4
Domain.Tests — 495

The whole solution builds with 0 warnings. The integration and e2e legs run only in CI.

End-to-end trial (scratch copy, not committed)

The trial ran on S1's real files, at this head:

  1. split: IReportQueries.cs split into its 7 records; SubmitDailyEntryHandler.cs split into SubmitDailyEntryResponse. Each extracted declaration is a verbatim slice of the original, and the remainder equals the original minus those slices.
  2. move: the 7 records → Application/Modules/Insights/Contracts/ (Cluckwork.Application.Modules.Insights.Contracts); IReportQueries → Modules/Insights/Reports/; the response → Modules/EggOperations/Contracts/; the handler → Modules/EggOperations/DailyEntries/SubmitDailyEntry/. The Insights and Egg Operations roots were set through the table's replace. It compiled first time, with no hand fixes, and introduced no unused using.
  3. marks: the 8 marks deleted. These contracts now come only from the folder.
Proof (base mapped through the type/path table vs head) Result
dotnet build Cluckwork.sln, analyzers on 0 warnings, 0 CW diagnostics
EF has-pending-model-changes no changes
Ordered DI descriptors: Production/Development × serving/one-shot × Redis absent/configured (8) identical
Contract set per owner (160) identical
Module-edge report, adapter reach, peer reach, table owners identical, ignoring file/line positions; the edge walk scans 8 more files (the split)
Generated OpenAPI document (Development) identical
Guard filter above 473 green; the one extra test was the scratch dump

The trial also exercised the root change that nsmove.py missed: AdapterRoots and PersistenceForbiddenNamespaces already include Cluckwork.Api.Modules here.

Mutations (scratch copy of the trial; each case reset, built and run)

"main guard" means the same mutation with that guard put back as it is on main.

# Mutation S0 guard main guard
G1 interface under Application.Modules returns IQueryable<object> red SeamSurfaceRealAssemblyTests green (skipped)
G2a endpoint under Api.Modules takes IFlockRepository red AdapterReachRealTreeTests (undeclared reach) green
G2b endpoint under Api.Modules takes AppDbContext red AdapterReachRealTreeTests (forbidden persistence) green; also green with only the root added, so the ban list needs its own entry
G3 endpoint under Api.Modules names IReportQueries red InsightsReadOnlyTests green
G4a untracked Task<Flock?> read in Infrastructure/Modules/FlockManagement/Repositories red TrackedMutationReadTests green
G4c untracked Task<Flock?> read in a subfolder: Repositories/Review/ and Modules/FlockManagement/Repositories/Review/ (16c7d86a) red TrackedMutationReadTests, both roots; restored green 2/2 main: red for Repositories/Review/ (its file walk read subfolders). The exact-folder selector at ba6b2803 stayed green for both, the gap Astra found
G4b codemod moves FlockRepository into Modules/FlockManagement/Repositories green (still discovered) red (its exclusions go stale)
G5a codemod moves the Farm registration to Cluckwork.Api.Modules.Farm green 7/7 red GuardWalk_ReachesDescendantNamespaces
G5b G5a plus an Ensure* method in the moved registration red EveryServingBootGuardMethod_HasACoveringRow that test green (method unseen)
N1 Contracts/UpdateCustomerCommand.cs moved to its feature folder, .Contracts namespace kept red NamespaceFolderAgreementTests only; every other guard green, which is the gap this rule closes —
N2 file stays in Contracts/, namespace changed to the feature red NamespaceFolderAgreementTests, AdapterReachRealTreeTests (CustomerEndpoints reaches a non-contract type), CouplingMatrix —
N3 file and namespace moved out of Contracts red AdapterReachRealTreeTests, CouplingMatrix (no peer takes the command, so PeerContractRealTreeTests stays green) —
N4 SubmitDailyEntryHandler moved into Contracts, namespace included red ModuleContractRealAssemblyTests green without the constructor walk (N4b)
N5 Cluckwork.Application.Modules.Nowhere.Contracts.NowhereProbe build red: CW1003 unowned namespace. With analyzers off: registry error "names owner 'Nowhere', which has no ModuleOwner row" —
N6 !IsNested filter dropped red ContractDerivationTests —
N7 moved contract keeps its [ModuleContract] mark red: registry error across the ledger guards —

For N1–N3, UpdateCustomerCommand was first moved into Modules/Commerce/Contracts/ by the codemod, because every contract the trial moved is a return type, and the adapter guards read only parameters.

Follow-on slice requirements

  • Each move slice must compare the exact normalized contract set per owner against its base, not only run the guards: PeerContractRealTreeTests stays green when an unused contract leaves Contracts (Astra, review of ba6b2803). The trial above did this comparison.

Notes for later slices

  • The codemod's first matrix run found two gaps, both fixed here. Extension-method callers (services.AddFarmModule()) never name the moved class, so the tool now adds their namespace too. Path strings in test data are rewritten too, so the folder rule's examples use a fictitious module.
  • Design §1's N3 row expects PeerContractRealTreeTests red; it is red only when a peer takes the moved type.
  • The design's commit order (split → move → fix → re-key → marks) matches the tool's commands: split, ide0005/prune, move, marks.

mforce added 6 commits October 5, 2026 18:21
Farm (two private records) and Contracts (a nested fixture class) become
segment names under #1087's Modules tree. Renamed first, per #985's
shadowing note.

Refs #1087
Each guard selected code by a namespace or folder that #1087 moves out of:
- SeamSurfaceRealAssemblyTests walks the whole Application assembly;
- AdapterRoots and PersistenceForbiddenNamespaces add Cluckwork.Api.Modules;
- InsightsReadOnlyTests reads the whole Api project, exempting the Insights
  registration file by name;
- TrackedMutationReadTests selects Repositories/ and Modules/<Owner>/Repositories/;
- ServingGuardCoverageTests walks Cluckwork.Api.Modules beside Hosting.

Refs #1087
A top-level type in Cluckwork.{Domain,Application}.Modules.<Owner>.Contracts
is <Owner>'s contract. Nested types stay outside it. A type both marked and in
a Contracts namespace is a registry error, so each module slice deletes the
mark when it moves the type. A fixture in the test assembly exercises the
branch, since main has no Contracts namespace yet.

Refs #1087
Reflection reads a contract from Type.Namespace, not the file's folder. This
rule makes them agree, so a file moved out of Contracts/ that keeps its
namespace fails. The #1086 rules files and namespace-free files are the
documented exceptions.

Refs #1087
tools/module-move/move.py splits declarations out of mixed files, moves
files by a finite per-file table with the namespace taken from the folder,
rewrites qualified names, usings, registry strings and paths, and prunes
only the unused usings it introduced (IDE0005 runs read from SARIF). The
compiler is the check. Exercised end to end on the S0 trial; no product
file moves in this PR.

Refs #1087
ModuleContractRealAssemblyTests read only methods, properties and events, so
a handler moved into a Contracts folder passed: its repositories sit in its
constructor. The contract scan now walks public constructor parameters.

Refs #1087
@mforce mforce added this to the Modular monolith milestone Oct 5, 2026
@mforce

mforce commented Oct 5, 2026

Copy link
Copy Markdown
Owner Author

Reviewed ba6b2803 against 02c299a7, the recorded main base. One P3 finding; no P0/P1/P2 findings. The rest of S0 matches the approved design. Both thermo-nuclear-code-quality-review and ponytail-review are complete.

[P3] Preserve recursive repository discovery. TrackedMutationReadTests.cs:156 now accepts only the exact Repositories or Modules/<Owner>/Repositories directory/namespace. The old source walk included subdirectories. Adding a normal feature subfolder now drops its source and reflected methods together, so discovery equality and the floor remain green. I added Repositories/Review/ReviewRepository.cs, with its matching namespace and an untracked Task<Flock?> read: head passes both tests; main's guard fails. Keep descendants in both selectors, for example ["Repositories", ..] or ["Modules", _, "Repositories", ..], and preserve a nested-folder mutation. Current files remain covered, but this change weakens the guard for the next repository added in a subfolder.

Verification and other requested judgments:

  • All 106 targeted baseline tests passed, one class at a time with DOTNET_USE_POLLING_FILE_WATCHER=1; database tests used sg docker -c. Current discovery retains exactly 65 seam interfaces, 38 DbSet entity types and 32 repository reads. Their floors remain 30. The folder rule covers 552 files with floor 500; Insights expands from 26 endpoint files to 86 Api files with floor 80. Adapter discovery retains 402 adapters and floor 40.
  • Independently reproduced the module-location seam, adapter reach, persistence-ban, Insights, tracked-read and serving-guard mutations. Each violation fails on head and passes with the corresponding main guard. The worker's real FlockRepository move remains discovered; its main selection fails stale exclusions. The saved separate persistence-ban mutation also proves adding AdapterRoots alone is insufficient.
  • Contract derivation correctly follows design §3: marks union Contracts namespaces, with duplicate membership rejected. The worker was right to follow that rule instead of requiring both sources to agree. The exact fixture set excludes nested cases, unknown owners fail registry validation, and the new public-constructor walk correctly checks repositories exposed through constructor parameters. All 160 real contracts pass.
  • NamespaceFolderAgreementTests matches the agreed rule. Its exceptions are the nine architecture rule files in one explicit directory, namespace-free files without types, and Program in Api/Program.cs. The ten tests pass. I inspected the final six design mutations and the additional duplicate-mark mutation; the corrected rerun logs support their claimed outcomes.
  • N3's green peer guard is acceptable when no peer uses the removed contract. It checks observed peer access. Every move slice must still compare the exact normalized contract sets against its base, including unused contracts. That required comparison closes the mechanical-move verification gap; a green peer test alone does not. The trial preserves all 160 contracts.
  • Keeping move.py is consistent with the approved design. It has no product or CI caller and adds no dependency. I replayed its mixed-file split/move/marks trial, checked all eight extracted declarations byte-for-byte, and built the entire solution with zero warnings/errors. I also moved the real Farm registration: extension-method callers compiled without hand fixes and all seven serving-coverage tests passed. Rewriting live path strings in test data is correct; the fictitious-module fixture avoids accidental rewrites of its examples.
  • The proof directory lacked an EF log, so I reran the EF check on my replayed trial: no pending model changes. I independently recomputed the saved comparisons: ordered DI descriptors match in all eight configurations; contract, edge, adapter, peer and table-owner reports match after the stated normalization; full OpenAPI JSON matches. The two SeededFarm renames and NestedContracts introduce no namespace shadowing; their targeted tests pass.

No additional structural or overengineering finding met the requested threshold. No full integration/browser suite or schema generator was rerun here. Local detail reports, commands and logs are preserved in /home/mforce/.cluckwork-slices/reviews/1088-astra-evidence/, especially 01-guards.md, 02-codemod-and-trial.md, results.json, nested-main-detail.log, nested-suggested-selector.log, trial-ef.log and parity.log. All temporary mutations were restored; no implementation fix or commit was made.

The module-safe selector matched only the exact Repositories and
Modules/<Owner>/Repositories folders, so a repository in a subfolder of
either dropped out of both the file walk and the reflected namespace set
and the guard stayed green. main's walk read subfolders. Both selectors
now include descendants.

Refs #1087
@mforce

mforce commented Oct 5, 2026

Copy link
Copy Markdown
Owner Author

Re Astra's review, P3 (TrackedMutationReadTests dropped repository subfolders): fixed in 16c7d86a.

IsRepositoryFolder now matches ["Repositories", ..] or ["Modules", _, "Repositories", ..], so the file walk and the reflected namespace set both include subfolders again. The floor, the exclusions and the discovery equality are unchanged.

Mutation: an untracked Task<Flock?> read in ReviewRepository, run with TrackedMutationReadTests (sg docker, polling watcher):

Probe location At 16c7d86a With the exact-folder selector from ba6b2803
Repositories/Review/ red: ReviewRepository.GetReviewAsync reported green (skipped)
Modules/FlockManagement/Repositories/Review/ red: ReviewRepository.GetReviewAsync reported green (skipped)
probe removed green, 2/2 —

The PR body's mutation table now has this as G4c. Your note that move slices must compare the exact normalized contract set against base is recorded under "Follow-on slice requirements".

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