Refactor/catalog vsa collapse

.claude/agents/architecture-reviewer.md

@@ -21,7 +21,7 @@ report. You do **not** write code or edit files — you read, analyze, and repor
    rule you'll evaluate against. Pay particular attention to these sections:
    - "Architecture Principles" → SOLID, DDD, Layer Dependencies, "Interfaces earn their
      keep through consumer substitution"
-   - "Project Structure" → Clean Architecture vs VSA, the per-service shape table
+   - "Project Structure" → VSA-everywhere (with the promotion signal for when to consider Clean Architecture)
    - "Coding Standards"
    - "Performance Rules"
    - "Key Conventions"
@@ -83,31 +83,26 @@ Specific bug-classes that have bitten this repo before. When the target file mat
 - **`GlobalExceptionHandler` traceId** uses `Activity.Current?.TraceId.ToString()`, NOT `Activity.Current?.Id` (which leaks the span ID in the W3C traceparent).
 - **No exception message leak.** Response body never contains `ex.Message`, `ex.StackTrace`, `ex.ToString()`.
 
-### When reviewing query handlers (`**/Features/Get*.cs`, `**/Application/Handlers/Get*.cs`)
+### When reviewing query handlers (`**/Features/Get*.cs`)
 
-- **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.
+- **Handler takes `DbContext` directly, projects inline (Must-fix).** Reads run an IQueryable inline in the handler: `context.Products.AsNoTracking().Where(...).Select(p => new ProductDto { ... }).ToListAsync(ct)`. No `IFooRepository` / `IFooReadStore` 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. Applies to ALL services including CatalogService (the previous Clean Architecture carve-out was retired when CatalogService collapsed to VSA). Flag any read handler that takes an `IFooRepository`/`IFooReadStore` dependency, or any read handler that loads an entity and maps via in-memory mapper.
 - **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 write handlers (`**/Features/*.cs` except `Get*`, `**/Application/Handlers/*Handler.cs` except `Get*`)
+### When reviewing write handlers (`**/Features/*.cs` except `Get*`)
 
-- **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.
+- **Handler takes `DbContext` directly, loads tracked, mutates, SaveChanges (Must-fix).** 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. Applies to ALL services including CatalogService. Flag any write handler that takes an `IFooRepository`.
 - **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".
+- **No `I*Repository` interfaces in any service.** All five services (Catalog, Order, Payment, Shipping, Notification) are now VSA with handlers taking `DbContext` directly — repository wrappers were removed in the simplicity refactor + CatalogService VSA-collapse. `DbContext` IS the Repository. Flag any new file matching `I*Repository.cs` or `I*ReadStore.cs` in any service's Domain folder.
 - **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.
@@ -122,7 +117,7 @@ Specific bug-classes that have bitten this repo before. When the target file mat
 
 ### When reviewing tests (`tests/**/*.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`.
+- **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>` or `Substitute.For<IFooReadStore>`, **two things are wrong**: the 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`.
 - **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 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.

.coderabbit.yaml

@@ -44,9 +44,10 @@ reviews:
         Interfaces earn their keep through consumer substitution (test mock or 2+ impls
         today, OR concrete near-term roadmap) — flag speculative interfaces. Repository
         wrappers around EF Core (IFooRepository + FooRepository) are explicitly NOT
-        justified by the substitution rule — see the "**/Domain/I*Repository.cs" and
-        "**/Infrastructure/*Repository.cs" path_instructions below for why and what
-        to flag.
+        justified by the substitution rule — see the "**/Domain/I*Repository.cs"
+        path_instructions below. No service has repository implementations anymore
+        (they were all retired in the simplicity refactor + CatalogService VSA collapse),
+        so a separate "**/Infrastructure/*Repository.cs" rule is unnecessary.
 
     - path: "**/Features/**/*.cs"
       instructions: |
@@ -80,36 +81,30 @@ reviews:
         (Wolverine vs. test fake) and ICatalogClient (gRPC vs. test fake). Those have
         legitimate substitution stories. Anything ending in "Repository" doesn't.
 
-    - path: "CatalogService/CatalogService.Application/Interfaces/IProductReadStore.cs"
-      instructions: |
-        IProductReadStore is the one read-side port that survives the simplicity
-        refactor — it's a Clean Architecture layer-rule consequence (Domain can't
-        reference Contracts where DTOs live), not a wrapper-around-EF pattern.
-        Implementation at CatalogService.Infrastructure/Repositories/ProductReadStore.cs.
-        Methods project to DTO inside the IQueryable via AsNoTracking() + .Select(...).
-        Flag any entity-returning method on the interface or impl — IProductReadStore
-        is read-side by contract. The VSA services don't have this seam — their reads
-        happen directly in handlers because there's no Domain/Application/Contracts
-        layer rule to honor.
-
     - path: "CatalogService/**/*.cs"
       instructions: |
-        This service uses Clean Architecture (Domain / Application / Infrastructure / Api).
-        Domain depends on nothing; Application on Domain; Infrastructure on both; Api is
-        the composition root. Flag any cross-layer leak. Domain entities use factory
-        methods (static Create()) with validation, never public setters.
-
-        DATA-ACCESS SHAPE (Clean variant — CARVE-OUT vs the project-wide
-        "no repository wrappers" rule): CatalogService keeps both IProductRepository
-        (Domain port, write-side) and IProductReadStore (Application port, read-side).
-        CatalogDbContext lives in Infrastructure; Application cannot reference
-        Infrastructure (would create a circular project reference), so Application
-        handlers can't take CatalogDbContext directly — they need the abstractions.
-        Both repositories pass the consumer-substitution test through the Clean layer
-        rules, unlike the VSA services where the wrapper added no capability and was
-        removed. See CLAUDE.md "Interfaces earn their keep" / "Exception — CatalogService".
-        Flag attempts to ADD additional repository wrappers, but DON'T flag the
-        existing IProductRepository/IProductReadStore as speculative coupling.
+        CatalogService is now Vertical Slice Architecture like the other four services —
+        the Clean Architecture four-project split was retired in the VSA-collapse
+        refactor. Single CatalogService.csproj at the service root; Features/ holds the
+        co-located command/query + validator + handler files; Domain/ holds aggregates
+        (Product, Category) and the IProductCache port; Infrastructure/ holds
+        CatalogDbContext + HybridProductCache + migrations + DI; Endpoints/ and Grpc/
+        hold the HTTP and gRPC surfaces.
+
+        DATA-ACCESS SHAPE: handlers take CatalogDbContext directly. There is NO
+        IProductRepository or IProductReadStore — read handlers project inline via
+        AsNoTracking() + .Select(...) into ProductDto; write handlers load the
+        aggregate tracked, mutate via state-transition methods (Product.UpdateDetails,
+        Product.AdjustStock), and call SaveChangesAsync. Flag any new I*Repository
+        introduced in this service — that pattern was removed for the same reason it
+        was removed from the other four services (DbContext IS Unit-of-Work, DbSet<T>
+        IS Repository; mocking has been replaced by integration tests with Testcontainers
+        in CatalogService.Tests.Integration). See CLAUDE.md "Data access: DbContext
+        directly, no repository wrappers".
+
+        DOMAIN INVARIANTS: Domain entities use factory methods (static Create()) with
+        validation, never public setters. Currency must be 3 chars; Price > 0; stock
+        non-negative.
 
     - path: "**/Endpoints/**/*.cs"
       instructions: |

.gitignore

@@ -489,3 +489,6 @@ $RECYCLE.BIN/
 
 # Local smoke-test env (URLs are dynamic per Aspire run; .env.smoke.example is checked in)
 .env.smoke
+
+# Understand-Anything knowledge graphs (per-machine; regenerated via /understand-anything:understand)
+.understand-anything/