Chore/claude encode session lessons

.claude/agents/architecture-reviewer.md

@@ -85,17 +85,37 @@ 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`)
 
-- **AsNoTracking + projection** for read paths. Either `.AsNoTracking() + .Select(...)` to a DTO, OR `AsNoTrackingWithIdentityResolution()` when `Include` is needed without tracking. Plain `AsNoTracking() + Include` duplicates the included entity per row.
+- **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".
+- **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`)
+
+- **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.
+
 ### When reviewing aggregates (`**/Domain/*.cs`)
 
 - **Rich Domain Entity shape.** Factory method (`static Create(...)`) with validation; private setters; named state-transition methods (`MarkAsPaid`, not `Status = Paid`); status-guard inside the transition method for idempotency under at-least-once delivery.
 - **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 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).
+- **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.
+
 ### When reviewing `.github/workflows/*.yml`
 
 - **`set -euo pipefail`** at top of every bash `run:` block.

.coderabbit.yaml

@@ -34,9 +34,9 @@ reviews:
     - path: "**/*.cs"
       instructions: |
         Apply CLAUDE.md "Architecture Principles" (SOLID, DDD, layer dependencies) and
-        "Performance Rules" (AsNoTracking + projection for reads, await everywhere on
-        request paths, CancellationToken propagation, pagination caps at 100,
-        optimistic concurrency tokens on aggregates, outbox atomicity).
+        "Performance Rules" (project-in-EF for reads — NOT entity-then-map-in-memory,
+        await everywhere on request paths, CancellationToken propagation, pagination
+        caps at 100, optimistic concurrency tokens on aggregates, outbox atomicity).
         File-scoped namespaces required. Async methods MUST be suffixed Async and called
         HandleAsync (NOT Handle). Private instance fields prefixed with _; constants and
         static readonly use PascalCase. Sync-over-async patterns (.Result, .Wait(),
@@ -52,13 +52,52 @@ reviews:
         Task<T>. Use primary-constructor DI. Look at OrderService/Features/PlaceOrder.cs
         for the canonical shape including Tier-1 teaching comments.
 
+        READ-PATH SHAPE (VSA variant): query handlers call a sibling DTO-returning
+        method on the existing repository (e.g. IOrderRepository.GetSummaryByIdAsync
+        returning OrderSummaryDto?) — NOT GetByIdAsync returning the entity followed by
+        an in-memory Mapper.ToDto() pass. The DTO-returning method projects in EF
+        (.AsNoTracking().Select(o => new OrderSummaryDto { ... })). The entity-returning
+        method stays for saga/command paths that mutate the aggregate. Flag any read
+        handler that loads a domain entity and maps in memory — the canonical fix is to
+        add a sibling repo method and switch the handler. Reference: GetOrdersByBuyer.cs.
+
+    - path: "**/Infrastructure/*Repository.cs"
+      instructions: |
+        CQRS read/write split is the rule, not a future cleanup (see
+        docs/cqrs-data-access.md). Repository methods divide into:
+          - Write-path loaders (e.g. GetByIdAsync returning Order/Shipment/Payment):
+            tracking ON, Include navigation children needed for the mutation, callers
+            mutate via aggregate methods and SaveChanges.
+          - Read-path projections (e.g. GetSummaryByIdAsync returning OrderSummaryDto):
+            AsNoTracking() + .Select(...) to DTO inside the IQueryable, returns the DTO
+            directly. NO entity materialization, NO in-memory mapper, NO calling
+            Mapper.ToDto(entity).
+
+        Flag any new or modified repository method that materializes a domain entity
+        for a caller that then maps to DTO in memory. The fix is to project in EF and
+        return the DTO. The only exception is methods on the WRITE path where the
+        caller actually mutates the loaded aggregate.
+
+        Methods on a read path that use AsNoTracking() but still return an entity
+        type are a half-fix — flag them too. The signature itself should be the proof
+        of intent: read paths return DTOs.
+
     - 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. Queries return
-        DTOs via projection (.Select(...)), never tracked entities.
+        methods (static Create()) with validation, never public setters.
+
+        READ-PATH SHAPE (Clean variant): query handlers depend on IProductReadStore
+        (or sibling IFooReadStore) defined in CatalogService.Application/Interfaces/,
+        implementation in CatalogService.Infrastructure/Repositories/. The read store
+        returns DTOs by projecting in EF (.AsNoTracking().Select(p => new ProductDto {...})).
+        Domain-layer IProductRepository stays entity-returning for the write path only —
+        adding a DTO-returning method there would force a Domain → Contracts dependency,
+        breaking layer rules. Flag any CatalogService query handler that calls
+        IProductRepository for a read instead of IProductReadStore. See
+        docs/cqrs-data-access.md for the full pattern.
 
     - path: "**/Endpoints/**/*.cs"
       instructions: |
@@ -135,6 +174,38 @@ reviews:
         infrastructure. Unit tests use NSubstitute for ports (IRepository, IEventPublisher,
         etc.). AwesomeAssertions (not FluentAssertions) for assertions.
 
+        TEST STRUCTURE — AAA WITH NARRATIVE COMMENTS (canonical per CLAUDE.md "Testing"):
+        Every test must follow the Arrange → Act → Assert structure with `// ARRANGE`,
+        `// ACT`, `// ASSERT` markers (all caps; em-dash explanation on the same line is
+        the convention). Each phase carries a *story comment*: what's being set up and
+        WHY, what's being called, and what each assertion verifies. A junior dev should
+        be able to read a single test top-to-bottom and understand the contract +
+        failure mode without needing to read the SUT first.
+
+        When the ASSERT phase verifies multiple invariants, number them and explain
+        why each matters — especially for:
+          - Security boundaries (IDOR / ownership checks): explain that returning null →
+            404 is intentional (not 403), to avoid leaking existence.
+          - Idempotency guards under at-least-once delivery: explain that a no-op (not a
+            throw) keeps the message off the DLQ for benign late deliveries.
+          - Ordering-sensitive operations (cache invalidate AFTER save): explain the race
+            window the ordering protects against.
+
+        Trivial happy-path tests can be shorter; security/concurrency/idempotency tests
+        get the full story. Flag tests that:
+          - Use lowercase `// arrange` markers or omit them entirely.
+          - Have ASSERT phases with multiple invariants and no rationale.
+          - Test a security/idempotency/ordering invariant without explaining the
+            failure mode being guarded against.
+
+        Reference templates:
+          - tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs
+            (security boundary + cache-after-save ordering)
+          - tests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs
+            (idempotency under at-least-once delivery)
+          - tests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cs
+            (IDOR-prevention pattern with null → 404)
+
     - path: "CLAUDE.md"
       instructions: |
         This is the canonical rules file. When this changes, every paraphrase ending in

.github/copilot-instructions.md

@@ -286,7 +286,7 @@ When generating code for NextAurora, follow CLAUDE.md for:
 
 - **[docs/performance-and-data-correctness.md](../docs/performance-and-data-correctness.md)** — the *why* behind every CLAUDE.md performance rule, the outbox decision, the concurrency-token decision, and the migration tooling.
 - **[docs/architecture.md](../docs/architecture.md)** — service topology, communication patterns, domain model, polyglot persistence rationale.
-- **[docs/cqrs-data-access.md](../docs/cqrs-data-access.md)** — handler inventory and the selective `AsNoTracking` strategy (with the rationale for why shared repository methods preserve tracking).
+- **[docs/cqrs-data-access.md](../docs/cqrs-data-access.md)** — handler inventory and the read/write method split (the hard rule: read methods return DTOs by projecting in EF; write methods return tracked entities; the two paths use different methods on the repository).
 - **[docs/context-propagation.md](../docs/context-propagation.md)** — how the three IDs flow through HTTP and Wolverine.
 - **[.claude/skills/dotnet-performance/SKILL.md](../.claude/skills/dotnet-performance/SKILL.md)** — deep `.NET` / EF Core perf guidance (modern EF features, transactions, plumbing, migrations, benchmarking).