Refactor/simplicity drop repos integration tests

.claude/agents/architecture-reviewer.md

@@ -85,22 +85,33 @@ Specific bug-classes that have bitten this repo before. When the target file mat
 
 ### When reviewing query handlers (`**/Features/Get*.cs`, `**/Application/Handlers/Get*.cs`)
 
-- **Project in EF, not in memory (Must-fix).** The handler must depend on a method that returns a DTO directly — e.g. `IOrderRepository.GetSummaryByIdAsync` returning `OrderSummaryDto?` (VSA), or `IProductReadStore.GetByIdAsync` returning `ProductDto?` (Clean). The anti-pattern signature: `var entity = await repo.GetByIdAsync(...); return Mapper.ToDto(entity);` — calling an entity-returning method and mapping in memory. That's the canonical bug this rule exists to prevent (cartesian explosion under collection `Include`, wasted column reads, double materialization). When the same `GetByIdAsync` is shared with a write/saga path, the fix is to add a **sibling** DTO-returning method on the repo; the entity-returning one stays for writes. See [docs/cqrs-data-access.md](../../docs/cqrs-data-access.md) and CLAUDE.md "Performance Rules → EF Core reads".
+- **Handler takes `DbContext` directly, projects inline (Must-fix — VSA services only).** Reads run an IQueryable inline in the handler: `context.Orders.AsNoTracking().Where(...).Select(o => new OrderSummaryDto { ... }).ToListAsync(ct)`. No `IFooRepository` interface wrapping that call. The repository wrappers were removed in the simplicity refactor — `DbContext` IS Unit-of-Work and `DbSet<T>` IS Repository. The unit-test-mocking justification for wrappers was replaced by integration tests against Testcontainers. Flag any VSA-service read handler that takes an `IFooRepository` dependency, or any read handler that loads an entity and maps via in-memory mapper.
+- **Exception — CatalogService Clean Architecture (both read AND write paths use ports).** Read handlers there depend on `IProductReadStore` (Application port; Domain can't reference Contracts where DTOs live). Write handlers there depend on `IProductRepository` (Domain port; Application can't reference Infrastructure where `CatalogDbContext` lives without a circular project ref). Both abstractions are layer-rule artifacts, not speculative wrapping. Do NOT flag the existing CatalogService repository pattern as a candidate for removal.
 - **AsNoTracking variants — know which mechanism does what.** Two independent axes (see [docs/cqrs-data-access.md "Why projection kills cartesian rows"](../../docs/cqrs-data-access.md#why-projection-kills-cartesian-rows-the-ef-mechanism)):
   - *Client-side object duplication.* `AsNoTracking() + Include` materializes the parent (or a shared related object like Category) once per row in the cartesian result — duplicate objects in memory. `AsNoTrackingWithIdentityResolution()` adds a per-query identity map so duplicates stitch into one object. **This fixes the object graph, not the SQL.**
   - *SQL row shape.* The cartesian rows still hit the wire under either tracking option. Killing them requires either projection-to-DTO with a nested collection (EF auto-splits projected collection navigations) or `AsSplitQuery()` for an entity materialization.
   - The projection rule above wins on both axes at once, which is why it's the default. `AsNoTrackingWithIdentityResolution()` is the narrow fallback for "I must materialize an entity graph without tracking" — rare on a read path. Plain `AsNoTracking()` returning an entity (not a DTO) is a *half-fix* — flag as Must-fix and direct to the projection rule.
 - **Pagination cap.** List queries must accept `(page, pageSize)` with server-side enforcement.
 - **N+1 detection.** Any `foreach` over query results that queries inside.
 
-### When reviewing repositories (`**/Infrastructure/*Repository.cs`, `**/Infrastructure/Repositories/*Repository.cs`)
+### When reviewing write handlers (`**/Features/*.cs` except `Get*`, `**/Application/Handlers/*Handler.cs` except `Get*`)
 
-- **Read/write method split (Must-fix).** Repository methods fall into two shapes:
-  - *Write loaders* (`GetByIdAsync` returning the entity, tracking ON, `Include` of mutation-relevant children) — used by command/event/saga handlers that mutate via aggregate methods.
-  - *Read projections* (`GetSummaryByIdAsync` / `GetSummariesByBuyerIdAsync` returning a DTO, `AsNoTracking() + .Select(...)` to DTO inside the IQueryable) — used by query handlers.
-  - A single method serving BOTH a read handler and a write handler is the antipattern this rule exists to remove (see [docs/cqrs-data-access.md](../../docs/cqrs-data-access.md)). Split it: add the sibling DTO method, switch the read handler.
-- **No `Mapper.ToDto` inside the repository.** If you find yourself materializing an entity in the repo just to map it before returning, you've reinvented the anti-pattern at a different layer. Project in the IQueryable.
-- **CatalogService (Clean) variant.** A DTO-returning method does NOT belong on `IProductRepository` (Domain interface — can't reference Contracts). It belongs on `IProductReadStore` in `CatalogService.Application/Interfaces/`, with the implementation in `CatalogService.Infrastructure/Repositories/`. Flag any DTO-returning method added to a Domain-layer interface.
+- **Handler takes `DbContext` directly, loads tracked, mutates, SaveChanges (Must-fix — VSA services only).** Standard write shape: `var order = await context.Orders.Include(o => o.Lines).FirstOrDefaultAsync(o => o.Id == id, ct); ...; order.MarkAsPaid(); await context.SaveChangesAsync(ct);`. No `IFooRepository` dependency. AutoApplyTransactions wraps SaveChanges + Wolverine outbox staging in one DB transaction. Flag any VSA-service write handler that takes an `IFooRepository`. CatalogService is the exception — see the Domain rule above.
+- **Aggregate state-transition methods, not direct property mutation.** `order.MarkAsPaid()`, not `order.Status = OrderStatus.Paid`. The aggregate method enforces the invariant via throw on invalid state.
+- **Handler-level idempotency for at-least-once delivery.** Saga consume handlers (e.g. `PaymentCompletedHandler`) pre-check status before calling the state-transition method: `if (order.Status != OrderStatus.Placed) return;`. The aggregate-level throw is the backstop, not the idempotency layer.
+- **Outbox-atomic non-handler code.** `BackgroundService` sweepers and other code outside the Wolverine handler pipeline need an explicit transactional wrap (`BeginTransactionAsync` → entity work + PublishAsync → SaveChangesAsync → CommitAsync). See PaymentRecoveryJob for the canonical pattern (post-refactor: inline; pre-refactor: behind `IPaymentRepository.ExecuteInTransactionAsync` — flag any re-introduction of that wrapper interface).
+
+### When reviewing Domain folders (`**/Domain/*.cs`)
+
+- **No `I*Repository` interfaces** in VSA services (Order, Payment, Shipping, Notification). They were removed in the simplicity refactor — `DbContext` IS the Repository. Flag any new file matching `I*Repository.cs` in those four services' Domain folders.
+- **Exceptions, both in CatalogService:**
+  - `IProductRepository` in `CatalogService.Domain/Interfaces/` (write-side) — survives because Application can't reference Infrastructure without a circular project ref, so handlers can't take `CatalogDbContext` directly. The repository is the layer-rule artifact.
+  - `IProductReadStore` in `CatalogService.Application/Interfaces/` (read-side) — survives because Domain can't reference Contracts (where DTOs live), so the DTO-returning interface lives in Application.
+  - Both pass the consumer-substitution test through Clean Architecture's project-reference structure. See [CLAUDE.md](../../CLAUDE.md) "Exception — CatalogService".
+- **Rich Domain Entity shape.** Factory method (`static Create(...)`) with validation; private setters; named state-transition methods (`MarkAsPaid`, not `Status = Paid`); throws on invalid transition (idempotency lives at the handler layer, not here).
+- **No mutable collection exposure.** `public IReadOnlyList<T>` over `private readonly List<T> _items`; add via named methods (`AddLine`), not direct mutation.
+- **Layer dependencies.** Domain depends on nothing — no EF, no logging, no Wolverine.
+- **Concurrency token present** (Postgres `xmin` shadow or SQL Server `RowVersion` shadow `byte[]` property in DbContext config — entity itself stays clean).
 
 ### When reviewing aggregates (`**/Domain/*.cs`)
 
@@ -111,10 +122,12 @@ Specific bug-classes that have bitten this repo before. When the target file mat
 
 ### When reviewing tests (`tests/**/*.cs`)
 
-- **AAA structure with narrative comments (per CLAUDE.md "Testing").** Every test must have `// ARRANGE`, `// ACT`, `// ASSERT` markers (all caps, em-dash explanation on the same line is the canonical form). Each phase carries a *story comment* a junior dev can follow: what's being set up and WHY, what's being called, what each assertion verifies. Lowercase markers (`// arrange`) or missing markers are a Must-fix style regression. ASSERT phases with multiple invariants must number them and explain why each matters — especially for security boundaries, idempotency guards, and ordering-sensitive operations. Reference templates: [UpdateProductHandlerTests.cs](../../tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs), [PaymentFailedHandlerTests.cs](../../tests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs), [GetShipmentByOrderHandlerTests.cs](../../tests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cs).
+- **Integration tests are the default for handler tests (Must-fix on misuse).** A handler that touches a `DbContext` belongs in `*.Tests.Integration` — booted via `WebApplicationFactory<Program>` against Testcontainers (real DB + Redis/ASB-stub as needed). Unit tests are reserved for *pure* domain logic: aggregate state-transition methods, FluentValidation rules, DTO operations — anything that doesn't reach `DbContext` or external IO. If a unit test reaches for `Substitute.For<IFooRepository>`, **two things are wrong**: the repository wrapper shouldn't exist (see the Domain rule above), and the test belongs in the integration project. Flag both. Legitimate `Substitute.For<T>` targets are still: `IEventPublisher`, `ICatalogClient`, `IPaymentGateway`, `INotificationSender`, `IProductCache`, `IProductReadStore`.
+- **AAA structure with narrative comments (per CLAUDE.md "Testing").** Every test must have `// ARRANGE`, `// ACT`, `// ASSERT` markers (all caps, em-dash explanation on the same line is the canonical form). Each phase carries a *story comment* a junior dev can follow: what's being set up and WHY, what's being called, what each assertion verifies. Lowercase markers (`// arrange`) or missing markers are a Must-fix style regression. ASSERT phases with multiple invariants must number them and explain why each matters — especially for security boundaries, idempotency guards, and ordering-sensitive operations. Reference templates (integration): `tests/OrderService.Tests.Integration/OrderSagaTests.cs`, `tests/CatalogService.Tests.Integration/ProductCachingTests.cs`. Reference templates (unit, domain-only): `tests/OrderService.Tests.Unit/Domain/OrderTests.cs`.
 - **Coverage for the contract, not just the happy path.** When a handler has security guards, idempotency short-circuits, ordering invariants, or status transitions, there must be a test for each branch. A single happy-path test on a handler with three branches is a Should-consider finding — name the missing scenarios explicitly.
-- **IDOR-test paired with scoped endpoints.** Any new endpoint that returns or mutates a buyer/seller-scoped entity must land with a test that authenticates as buyer X, requests buyer Y's resource, and asserts 404 (NOT 200, NOT 403). The absence of such a test is exactly how the original `GET /api/v1/orders/{id}` IDOR survived undetected — Must-fix when the PR adds a scoped endpoint without it. (Unit test for the handler returning null is one half; integration test for the endpoint returning 404 is the other half — call out which is missing.)
-- **NSubstitute + AwesomeAssertions** (not Moq + FluentAssertions). Plain `Substitute.For<T>` for ports, `Should().Be()` / `Should().Throw<>()` for assertions.
+- **IDOR-test paired with scoped endpoints.** Any new endpoint that returns or mutates a buyer/seller-scoped entity must land with an **integration test** that authenticates as buyer X, requests buyer Y's resource, and asserts 404 (NOT 200, NOT 403). The absence of such a test is exactly how the original `GET /api/v1/orders/{id}` IDOR survived undetected — Must-fix when the PR adds a scoped endpoint without it.
+- **NSubstitute + AwesomeAssertions** (not Moq + FluentAssertions). Plain `Substitute.For<T>` for ports listed above, `Should().Be()` / `Should().Throw<>()` for assertions.
+- **Direct handler resolution requires DI registration (Must-fix on missing pair).** If a test does `serviceProvider.GetRequiredService<*Handler>()` (or `GetService<*Handler>()`), check that the handler has a matching `services.AddScoped<*Handler>()` in the service's `AddXInfrastructure` registration. Wolverine's `opts.Discovery` populates its own internal handler-type map for `IMessageBus` dispatch but does NOT register handler types in `IServiceCollection`. Missing registration → `InvalidOperationException: No service for type 'X.Handler' has been registered` at runtime. The reverse direction matters too: a PR that removes `AddScoped<*Handler>()` without removing the test's resolution is also Must-fix — the test compiles silently and breaks in CI. See CLAUDE.md "Communication Patterns → Wolverine handler discovery is NOT DI registration". The failure mode that surfaced this rule: OrderReadProjectionTests broke in CI after the repository-wrapper refactor.
 
 ### When reviewing `.github/workflows/*.yml`