Refactor/catalog vsa collapse#31
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
WalkthroughThis PR collapses CatalogService to a single-project Vertical Slice Architecture, removes repository-wrapper interfaces, and changes handlers to use CatalogDbContext directly: reads use AsNoTracking().Select(...) to project DTOs; writes load tracked aggregates, mutate via domain methods, call SaveChangesAsync, then invalidate cache. ChangesVSA consolidation and DbContext-direct handlers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/ef-core.md (1)
457-464:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace stale repository-based examples/links with current DbContext-direct handler examples.
These changed sections still reference removed repository paths/patterns (
ProductRepository.cs,UpdateProductHandler.cs,repository.GetByIdAsync), which now conflicts with this document’s own hard rule and can send contributors toward deprecated architecture.As per coding guidelines, handlers must take
DbContextdirectly and repository wrappers around EF are an anti-pattern removed in this refactor.Also applies to: 721-728, 1066-1081, 1142-1142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ef-core.md` around lines 457 - 464, The docs still show repository-based examples/links (e.g., ProductRepository.cs, UpdateProductHandler.cs, and calls like repository.GetByIdAsync and the GetAllAsync method) which are deprecated; update each referenced example (including the snippet shown and the other occurrences at 721-728, 1066-1081, 1142) to use DbContext-first handler patterns instead: remove links to repository files, change the narrative and code examples to show handlers taking the CatalogDbContext (or appropriate DbContext) and directly querying context.Products/.Categories with AsNoTracking/Include/OrderBy/Skip/Take/ToListAsync or FindAsync, and ensure handler method names and parameter examples reflect DbContext usage rather than repository.GetByIdAsync.docs/code-flows/shippingservice.md (1)
171-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stale CQRS note in “See also” link text.
The parenthetical “both methods on same interface” conflicts with this doc’s updated DbContext-direct/no-repository-wrapper model and can confuse readers.
Suggested doc fix
-- [docs/cqrs-data-access.md](../cqrs-data-access.md) — read/write split rule (VSA variant — both methods on same interface) +- [docs/cqrs-data-access.md](../cqrs-data-access.md) — read/write split rule (handler code-shape: read projection vs tracked write)As per coding guidelines "CLAUDE.md: Prefer DbContext directly ... no repository wrappers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/code-flows/shippingservice.md` at line 171, Update the "See also" link text that currently reads "[docs/cqrs-data-access.md] — read/write split rule (VSA variant — both methods on same interface)" by removing the stale parenthetical "(VSA variant — both methods on same interface)"; edit the link caption to accurately reflect the current DbContext-direct/no-repository-wrapper model (e.g., "read/write split rule — DbContext direct (no repository wrappers)") so it aligns with the CLAUDE.md guideline and avoids contradicting the shippingservice.md text.docs/performance-and-data-correctness.md (1)
274-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate stale repository-based example to DbContext-direct shape.
This example still shows
repository.GetByIdAsync(...), which contradicts the current “handlers take DbContext directly” rule and can mislead readers.Suggested doc fix
-The handler is now a thin delegate to the cache — see [GetProductByIdHandler.cs](../CatalogService/Features/GetProductByIdHandler.cs): +The handler is now a thin delegate to the cache — see [GetProductById.cs](../CatalogService/Features/GetProductById.cs): ```csharp public Task<ProductDto?> HandleAsync(GetProductByIdQuery request, CancellationToken ct) => cache.GetOrLoadAsync(request.ProductId, async cancel => { - var product = await repository.GetByIdAsync(request.ProductId, cancel); - return product is null ? null : new ProductDto { /* projection */ }; + return await context.Products + .AsNoTracking() + .Where(p => p.Id == request.ProductId) + .Select(p => new ProductDto { /* projection */ }) + .FirstOrDefaultAsync(cancel); }, ct);</details> As per coding guidelines "CLAUDE.md: Prefer DbContext directly: handlers take DbContext ... no repository wrappers." <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/performance-and-data-correctness.mdaround lines 274 - 283, The example
in GetProductByIdHandler.cs uses repository.GetByIdAsync which is stale; update
the HandleAsync lambda passed to cache.GetOrLoadAsync to query the DbContext
directly (use context.Products with AsNoTracking, filter by request.ProductId,
project to ProductDto via Select, and call FirstOrDefaultAsync with the
cancellation token) instead of calling repository.GetByIdAsync so the handler
follows the "handlers take DbContext directly" guideline.</details> </blockquote></details> <details> <summary>OrderService/Features/PaymentFailedHandler.cs (1)</summary><blockquote> `8-19`: _🧹 Nitpick_ | _🔵 Trivial_ | _💤 Low value_ **Consider documenting idempotency layers to match sibling handler.** `PaymentCompletedHandler` has comprehensive documentation of the three-layer idempotency pattern and concurrency safety. This handler implements the same pattern but only documents "why no compensation." For teaching-grade consistency, consider adding parallel documentation of: - Idempotency layers (existence check, status guard, domain guard) - Concurrency safety via RowVersion + Wolverine's retry policy The code is correct; this is a documentation consistency suggestion. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@OrderService/Features/PaymentFailedHandler.csaround lines 8 - 19, Add
parallel documentation to PaymentFailedHandler mirroring
PaymentCompletedHandler: describe the three idempotency layers (existence check,
handler-level status guard, domain-level status guard) and mention concurrency
safety using the Order aggregate's RowVersion plus Wolverine's retry policy;
reference PaymentFailedHandler and PaymentCompletedHandler by name and ensure
the new XML summary explains that these layers prevent duplicates and concurrent
updates, matching the sibling handler's explanatory style.</details> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In @.coderabbit.yaml: - Around line 45-49: The referenced path instruction "**/Infrastructure/*Repository.cs" in .coderabbit.yaml is missing; update the file to either remove or correct that dangling cross-reference and ensure the guidance aligns with the canonical CLAUDE.md: either add a corresponding "**/Infrastructure/*Repository.cs" path_instructions section that explains why repository wrappers (IFooRepository + FooRepository) are flagged, or change the text to point to the existing path_instructions (e.g., "**/Domain/I*Repository.cs") so the reviewer guidance is consistent and actionable. In `@CatalogService/Features/ReserveStock.cs`: - Around line 43-47: The SaveChangesAsync call in ReserveStock handler can throw DbUpdateConcurrencyException which should be handled and cause the method to return false; wrap the call to context.SaveChangesAsync (and the subsequent cache.InvalidateAsync call) in a try/catch that catches Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException and returns false from the handler (preserving other exceptions to bubble), ensuring cache.InvalidateAsync is only awaited if SaveChangesAsync succeeds; look for the ReserveStock handler method and the SaveChangesAsync + cache.InvalidateAsync sequence to apply this change. In `@CatalogService/Features/SearchProducts.cs`: - Around line 32-35: Guard against empty or whitespace search terms before building the SQL pattern: in the handler that reads request.Query (reference request.Query and the local variable pattern), add an early check (e.g., string.IsNullOrWhiteSpace(request.Query)) and short-circuit — either return an empty result set or a validation error/BadRequest instead of continuing to build "%{request.Query}%" and querying context.Products.AsNoTracking().Where(...).OrderBy(p => p.Id); this prevents the "%%" pattern from matching all products. In `@CatalogService/Features/UpdateProduct.cs`: - Around line 50-52: The UpdateProduct handler is eagerly loading Category even though it's never used; remove the Include(p => p.Category) from the product load so the query is just context.Products.FirstOrDefaultAsync(...), reducing unnecessary tracking/joins. Update the code in the method that assigns var product (the query against context.Products) to drop the .Include call and run tests to ensure no other code in this handler relies on Category. In `@docs/architecture.md`: - Around line 67-77: The fenced code block that starts with "ServiceName/" is missing a language tag (causing markdownlint MD040); update the opening fence to include a language identifier (for example use "text" or "bash") so it becomes ```text, preserving the exact block content and comments (the Features/ ... ServiceName.csproj lines) and ensure the closing fence remains ```; no other content should be changed. In `@docs/dotnet-10-features.md`: - Line 138: Update the docs entry that references a repository-style read to reflect the current Catalog handler pattern: replace the path and example pointing to CatalogService.Infrastructure/Repositories/ProductRepository.cs with guidance showing handler-inline projection using CatalogDbContext (mentioning AsNoTracking() + projection on reads and Include(p => p.Category) if still relevant). Reference the CatalogDbContext and the handler-inline projection approach in the text, remove any mention of repository wrappers/read stores, and ensure the wording and example align with the CatalogService coding guidelines. In `@docs/how-it-works.md`: - Around line 71-80: The fenced code block that lists "ServiceName/ Features/ Domain/ Infrastructure/ Endpoints/ Grpc/ Program.cs" needs a language identifier for markdownlint MD040; update the opening triple backticks to include a language (e.g., change "```" to "```text") so the block becomes a fenced code block with a specified language and preserves the existing lines and comments. In `@docs/performance-and-data-correctness.md`: - Line 129: Update the broken handler links in the cache-invalidation rule: replace references to GetProductByIdHandler, UpdateProductHandler, and ReserveStockHandler files with the actual handler filenames used in this repo—GetProductById.cs, UpdateProduct.cs, and ReserveStock.cs—so the links under CatalogService/Features point to those real files; ensure the display text still mentions the handler types (e.g., GetProductById) but the target URLs reference the correct .cs filenames. In `@docs/project-decisions.md`: - Line 269: Update the visible link label so it matches the updated target path: replace the label text "CatalogService.Api/Program.cs" with "CatalogService/Program.cs" wherever the markdown link points to "../CatalogService/Program.cs" (also fix the identical occurrence noted at the second location). Ensure the link syntax remains [CatalogService/Program.cs](../CatalogService/Program.cs). In `@docs/STATUS.md`: - Line 5: The "Last updated:" timestamp in docs/STATUS.md is stale; update the string "**Last updated:** 2026-05-25" to "**Last updated:** 2026-05-26" so the file's Last updated date matches today's date, making sure you only change the date portion of the "Last updated:" line. In `@OrderService/Features/GetOrderById.cs`: - Around line 14-37: The handler currently allows any user to fetch orders by ID; change the GetOrderByIdQuery to include a RequestingBuyerId (Guid) and update GetOrderByIdHandler.HandleAsync to filter Orders by both o.Id == request.OrderId AND o.BuyerId == request.RequestingBuyerId, returning null if no match; also ensure the endpoint (OrderEndpoints) extracts ClaimTypes.NameIdentifier from HttpContext, parses it to a Guid and passes it as RequestingBuyerId when invoking the handler so only the owning buyer can retrieve the order. In `@OrderService/Features/PlaceOrder.cs`: - Around line 140-143: The comment is misleading about SaveChangesAsync committing the transaction; update the comment near the context.Orders.AddAsync / await context.SaveChangesAsync(cancellationToken) to state that Wolverine's AutoApplyTransactions opens a transaction before the handler, SaveChangesAsync only flushes the entity into that transaction (does not commit), PublishAsync stages the outgoing OrderPlacedEvent envelope into the same transaction, and the transaction is committed by AutoApplyTransactions when the handler returns. Mention the specific symbols SaveChangesAsync, PublishAsync, AutoApplyTransactions, and OrderPlacedEvent to make the sequence clear. In `@README.md`: - Line 14: Update the README paragraph that claims CatalogService still uses repository wrappers so it reflects the refactor: change the text to state that all services, including CatalogService, now use the VSA pattern with handlers taking DbContext directly (no IFooRepository wrappers), and reference the CLAUDE.md/STATUS.md guidance about preferring DbContext; ensure the sentence about the v1-repository-pattern tag remains but remove the assertion that CatalogService is a Clean Architecture carve-out. In `@tests/OrderService.Tests.Integration/OrderReadProjectionTests.cs`: - Around line 20-23: The XML summary incorrectly states that unit tests mock handler dependencies (OrderDbContext) which contradicts the project's integration-first handler-testing policy; update the comment to say that handler tests (e.g., GetOrderByIdHandler and GetOrdersByBuyerHandler) are integration tests that exercise the real OrderDbContext/SQL projection so future readers understand these tests validate EF-generated SQL and DTO projections rather than being mocked unit tests. Ensure you reference the handlers GetOrderByIdHandler and GetOrdersByBuyerHandler and OrderDbContext in the revised summary and remove or replace the word "unit" with "integration" and the note about mocking. --- Outside diff comments: In `@docs/code-flows/shippingservice.md`: - Line 171: Update the "See also" link text that currently reads "[docs/cqrs-data-access.md] — read/write split rule (VSA variant — both methods on same interface)" by removing the stale parenthetical "(VSA variant — both methods on same interface)"; edit the link caption to accurately reflect the current DbContext-direct/no-repository-wrapper model (e.g., "read/write split rule — DbContext direct (no repository wrappers)") so it aligns with the CLAUDE.md guideline and avoids contradicting the shippingservice.md text. In `@docs/ef-core.md`: - Around line 457-464: The docs still show repository-based examples/links (e.g., ProductRepository.cs, UpdateProductHandler.cs, and calls like repository.GetByIdAsync and the GetAllAsync method) which are deprecated; update each referenced example (including the snippet shown and the other occurrences at 721-728, 1066-1081, 1142) to use DbContext-first handler patterns instead: remove links to repository files, change the narrative and code examples to show handlers taking the CatalogDbContext (or appropriate DbContext) and directly querying context.Products/.Categories with AsNoTracking/Include/OrderBy/Skip/Take/ToListAsync or FindAsync, and ensure handler method names and parameter examples reflect DbContext usage rather than repository.GetByIdAsync. In `@docs/performance-and-data-correctness.md`: - Around line 274-283: The example in GetProductByIdHandler.cs uses repository.GetByIdAsync which is stale; update the HandleAsync lambda passed to cache.GetOrLoadAsync to query the DbContext directly (use context.Products with AsNoTracking, filter by request.ProductId, project to ProductDto via Select, and call FirstOrDefaultAsync with the cancellation token) instead of calling repository.GetByIdAsync so the handler follows the "handlers take DbContext directly" guideline. In `@OrderService/Features/PaymentFailedHandler.cs`: - Around line 8-19: Add parallel documentation to PaymentFailedHandler mirroring PaymentCompletedHandler: describe the three idempotency layers (existence check, handler-level status guard, domain-level status guard) and mention concurrency safety using the Order aggregate's RowVersion plus Wolverine's retry policy; reference PaymentFailedHandler and PaymentCompletedHandler by name and ensure the new XML summary explains that these layers prevent duplicates and concurrent updates, matching the sibling handler's explanatory style.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID:
7f764e66-219d-4ed7-b2de-6e9132c39f91📒 Files selected for processing (113)
.claude/agents/architecture-reviewer.md.coderabbit.yamlCLAUDE.mdCatalogService/CatalogService.Application/CatalogService.Application.csprojCatalogService/CatalogService.Application/Commands/CreateProductCommand.csCatalogService/CatalogService.Application/Commands/ReserveStockCommand.csCatalogService/CatalogService.Application/Commands/UpdateProductCommand.csCatalogService/CatalogService.Application/Handlers/CreateProductHandler.csCatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.csCatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.csCatalogService/CatalogService.Application/Handlers/ReserveStockHandler.csCatalogService/CatalogService.Application/Handlers/SearchProductsHandler.csCatalogService/CatalogService.Application/Handlers/UpdateProductHandler.csCatalogService/CatalogService.Application/Interfaces/IProductReadStore.csCatalogService/CatalogService.Application/Queries/GetAllProductsQuery.csCatalogService/CatalogService.Application/Queries/GetProductByIdQuery.csCatalogService/CatalogService.Application/Queries/SearchProductsQuery.csCatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.csCatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.csCatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.csCatalogService/CatalogService.Domain/CatalogService.Domain.csprojCatalogService/CatalogService.Domain/Interfaces/IProductRepository.csCatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csprojCatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.csCatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.csCatalogService/CatalogService.csprojCatalogService/Domain/Category.csCatalogService/Domain/IProductCache.csCatalogService/Domain/Product.csCatalogService/Endpoints/CatalogEndpoints.csCatalogService/Features/CreateProduct.csCatalogService/Features/GetAllProducts.csCatalogService/Features/GetProductById.csCatalogService/Features/ReserveStock.csCatalogService/Features/SearchProducts.csCatalogService/Features/UpdateProduct.csCatalogService/Grpc/CatalogGrpcService.csCatalogService/Infrastructure/Caching/HybridProductCache.csCatalogService/Infrastructure/Data/CatalogDbContext.csCatalogService/Infrastructure/Data/CatalogDbContextFactory.csCatalogService/Infrastructure/DependencyInjection.csCatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.Designer.csCatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.csCatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.Designer.csCatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.csCatalogService/Infrastructure/Migrations/CatalogDbContextModelSnapshot.csCatalogService/Program.csCatalogService/Properties/launchSettings.jsonCatalogService/Protos/catalog.protoCatalogService/appsettings.Development.jsonCatalogService/appsettings.jsonNextAurora.AppHost/AppHost.csNextAurora.AppHost/NextAurora.AppHost.csprojNextAurora.slnxOrderService/Domain/IOrderRepository.csOrderService/Features/GetOrderById.csOrderService/Features/GetOrdersByBuyer.csOrderService/Features/PaymentCompletedHandler.csOrderService/Features/PaymentFailedHandler.csOrderService/Features/PlaceOrder.csOrderService/Features/ShipmentDispatchedHandler.csOrderService/Infrastructure/DependencyInjection.csOrderService/Infrastructure/GrpcCatalogClient.csOrderService/Infrastructure/OrderRepository.csOrderService/OrderService.csprojPaymentService/Domain/IPaymentRepository.csPaymentService/Features/ProcessPayment.csPaymentService/Infrastructure/DependencyInjection.csPaymentService/Infrastructure/PaymentRecoveryJob.csPaymentService/Infrastructure/PaymentRepository.csREADME.mdShippingService/Domain/IShipmentRepository.csShippingService/Features/CreateShipment.csShippingService/Features/GetShipmentByOrder.csShippingService/Infrastructure/DependencyInjection.csShippingService/Infrastructure/ShipmentRepository.csdocs/STATUS.mddocs/architecture.mddocs/code-flows/catalogservice.mddocs/code-flows/orderservice.mddocs/code-flows/paymentservice.mddocs/code-flows/shippingservice.mddocs/cqrs-data-access.mddocs/dotnet-10-features.mddocs/ef-core.mddocs/how-it-works.mddocs/performance-and-data-correctness.mddocs/project-decisions.mdtests/CatalogService.Tests.Integration/CatalogService.Tests.Integration.csprojtests/CatalogService.Tests.Integration/ProductAuthorizationTests.cstests/CatalogService.Tests.Integration/ProductCachingTests.cstests/CatalogService.Tests.Integration/ProductReadProjectionTests.cstests/CatalogService.Tests.Unit/Application/CreateProductCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cstests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cstests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cstests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cstests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cstests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cstests/CatalogService.Tests.Unit/Builders/ProductBuilder.cstests/CatalogService.Tests.Unit/CatalogService.Tests.Unit.csprojtests/OrderService.Tests.Integration/OrderApiFactory.cstests/OrderService.Tests.Integration/OrderReadProjectionTests.cstests/OrderService.Tests.Unit/Application/GetOrderByIdHandlerTests.cstests/OrderService.Tests.Unit/Application/GetOrdersByBuyerHandlerTests.cstests/OrderService.Tests.Unit/Application/PaymentCompletedHandlerTests.cstests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cstests/OrderService.Tests.Unit/Application/PlaceOrderHandlerTests.cstests/OrderService.Tests.Unit/Application/ShipmentDispatchedHandlerTests.cstests/PaymentService.Tests.Unit/Application/PaymentRecoveryJobTests.cstests/PaymentService.Tests.Unit/Application/ProcessPaymentHandlerTests.cstests/ShippingService.Tests.Unit/Application/CreateShipmentHandlerTests.cstests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cs💤 Files with no reviewable changes (44)
- CatalogService/CatalogService.Application/Handlers/SearchProductsHandler.cs
- CatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.cs
- CatalogService/CatalogService.Application/Queries/GetProductByIdQuery.cs
- PaymentService/Infrastructure/PaymentRepository.cs
- CatalogService/CatalogService.Application/Handlers/CreateProductHandler.cs
- CatalogService/CatalogService.Application/Queries/GetAllProductsQuery.cs
- CatalogService/CatalogService.Application/Queries/SearchProductsQuery.cs
- CatalogService/CatalogService.Application/Commands/ReserveStockCommand.cs
- tests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cs
- CatalogService/CatalogService.Application/Commands/CreateProductCommand.cs
- CatalogService/CatalogService.Domain/CatalogService.Domain.csproj
- tests/ShippingService.Tests.Unit/Application/CreateShipmentHandlerTests.cs
- CatalogService/CatalogService.Domain/Interfaces/IProductRepository.cs
- tests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cs
- CatalogService/CatalogService.Application/Commands/UpdateProductCommand.cs
- CatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.cs
- CatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.cs
- CatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.cs
- CatalogService/CatalogService.Application/CatalogService.Application.csproj
- tests/OrderService.Tests.Unit/Application/GetOrderByIdHandlerTests.cs
- ShippingService/Domain/IShipmentRepository.cs
- tests/PaymentService.Tests.Unit/Application/ProcessPaymentHandlerTests.cs
- CatalogService/CatalogService.Application/Handlers/ReserveStockHandler.cs
- CatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.cs
- tests/OrderService.Tests.Unit/Application/PaymentCompletedHandlerTests.cs
- tests/OrderService.Tests.Unit/Application/GetOrdersByBuyerHandlerTests.cs
- OrderService/Domain/IOrderRepository.cs
- tests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cs
- tests/OrderService.Tests.Unit/Application/PlaceOrderHandlerTests.cs
- PaymentService/Domain/IPaymentRepository.cs
- tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs
- tests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cs
- CatalogService/CatalogService.Application/Interfaces/IProductReadStore.cs
- tests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs
- ShippingService/Infrastructure/ShipmentRepository.cs
- CatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.cs
- tests/PaymentService.Tests.Unit/Application/PaymentRecoveryJobTests.cs
- CatalogService/CatalogService.Application/Handlers/UpdateProductHandler.cs
- CatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csproj
- CatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.cs
- tests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cs
- tests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cs
- tests/OrderService.Tests.Unit/Application/ShipmentDispatchedHandlerTests.cs
- OrderService/Infrastructure/OrderRepository.cs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/CatalogService.Tests.Integration/ProductReadProjectionTests.cs`:
- Around line 136-157: The oversized PageSize path isn't actually validated
because the test only checks NotBeNull(); update the test around
oversizedPageSize (created by calling handler.HandleAsync(new
GetAllProductsQuery(Page: 1, PageSize: 5000), ...)) to seed enough matching
products so pagination can be observed, then assert the clamp directly (e.g.,
oversizedPageSize.Count <= 50 or Equals(expectedClampedPageSize)) to ensure the
handler's clamp logic is exercised; keep the overflowed and negativePage
assertions but add the new count assertion for oversizedPageSize and seed data
so it can fail if clamping is removed.
In
`@tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs`:
- Around line 12-75: Add the mandatory AAA narrative comments to every test
method in ReserveStockCommandValidatorTests: for each test (e.g.,
Validate_WithValidCommand_ReturnsNoErrors,
Validate_WithEmptyProductId_ReturnsError,
Validate_WithZeroQuantity_ReturnsError,
Validate_WithNegativeQuantity_ReturnsError,
Validate_WithQuantityAtUpperBound_ReturnsNoErrors,
Validate_WithQuantityOverUpperBound_ReturnsError) insert a single-line comment
"// ARRANGE" before creating the command, "// ACT" before calling
_sut.Validate(...), and "// ASSERT" before the FluentAssertions assertions so
each test clearly shows the Arrange/Act/Assert phases.
In
`@tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs`:
- Around line 13-98: Each test (e.g., Validate_WithValidCommand_ReturnsNoErrors,
Validate_WithEmptyProductId_ReturnsError, etc.) must be refactored to include
the AAA markers and short narrative comments: add a "// ARRANGE - <brief
narrative>" above the ValidCommand() setup, a "// ACT - <brief narrative>" above
the call to _sut.Validate(command), and a "// ASSERT - <brief narrative>" above
the result assertions; keep the existing calls (ValidCommand(),
_sut.Validate(command), and result assertions) unchanged, only insert the three
commented markers and a one-line narrative for each phase in every test method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e460752b-1614-4678-84ad-4ea0420525df
📒 Files selected for processing (3)
tests/CatalogService.Tests.Integration/ProductReadProjectionTests.cstests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/CatalogService.Tests.Integration/ProductCachingTests.cs`:
- Around line 162-163: The test methods introduced as async Task must be
suffixed with Async to follow the repo convention: rename the methods declared
at the three locations (the method currently named
PostProduct_byOwner_returns201AndPersistsProduct and the two other new async
test methods at the other noted spots) to include the Async suffix (e.g.,
PostProduct_byOwner_returns201AndPersistsProductAsync) and update any call sites
or references (test attributes, helpers, or runners) to the new names so
compilation and test discovery continue to work.
- Around line 215-250: The test
ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCache currently only
asserts DB state after invoking ReserveStockCommand via IMessageBus; update it
to either (A) actually verify cache behavior by resolving the cache abstraction
used in tests (e.g., IDistributedCache or your Product cache service) from the
same scope, seed/read the cached Product before/after bus.InvokeAsync and assert
the cache entry was removed/updated, or (B) if cache verification is out of
scope, rename the test method and any references to remove "InvalidatesCache" so
the name matches the asserted behavior; locate references to
ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCache,
ReserveStockCommand, and IMessageBus to implement the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3abe284-64fc-4bd3-8de8-7125c1f5b469
📒 Files selected for processing (1)
tests/CatalogService.Tests.Integration/ProductCachingTests.cs
emeraldleaf
force-pushed
the
refactor/catalog-vsa-collapse
branch
from
May 26, 2026 02:12
a80792f to
e23d15b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/CatalogService.Tests.Unit/Application/CreateProductCommandValidatorTests.cs (1)
14-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd required AAA narrative markers in each unit test.
These tests omit the mandated
// ARRANGE,// ACT,// ASSERTphase markers. Please apply the required structure consistently across all methods, even for simple validator tests.As per coding guidelines, "
**/*.Tests*/**/*.cs: Test structure — AAA with narrative comments. Every test: Arrange → Act → Assert with // ARRANGE, // ACT, // ASSERT markers (all caps, em-dash explanation)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Unit/Application/CreateProductCommandValidatorTests.cs` around lines 14 - 38, Add the mandated AAA narrative comments to each test: for Validate_WithValidCommand_ReturnsNoErrors and Validate_WithEmptyName_ReturnsError insert three comment lines marking the phases above their blocks — use "// ARRANGE —" before the setup where ValidCommand() is created, "// ACT —" before the _sut.Validate(command) invocation, and "// ASSERT —" before the FluentAssertions checks (result.IsValid... and result.Errors...). Ensure the markers are all caps, use an em-dash and a short phase description, and place them in each test method consistently.
♻️ Duplicate comments (8)
tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs (1)
14-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd required AAA narrative markers in each unit test.
These tests omit the mandated
// ARRANGE,// ACT,// ASSERTphase markers. Please apply the required structure consistently across all eight test methods.As per coding guidelines, "
**/*.Tests*/**/*.cs: Test structure — AAA with narrative comments. Every test: Arrange → Act → Assert with // ARRANGE, // ACT, // ASSERT markers (all caps, em-dash explanation)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs` around lines 14 - 98, Each unit test (Validate_WithValidCommand_ReturnsNoErrors, Validate_WithEmptyProductId_ReturnsError, Validate_WithEmptySellerId_ReturnsError, Validate_WithEmptyName_ReturnsError, Validate_WithNameOver200Chars_ReturnsError, Validate_WithDescriptionOver2000Chars_ReturnsError, Validate_WithZeroPrice_ReturnsError, Validate_WithNegativePrice_ReturnsError) must include the AAA narrative markers: add a top-line comment "// ARRANGE — set up test data and dependencies" before creating the command, a comment "// ACT — execute _sut.Validate(command)" immediately before the call to _sut.Validate(command), and a comment "// ASSERT — verify result expectations" before the Should(). assertions so each test clearly follows Arrange → Act → Assert.README.md (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winREADME architectural-era note is still factually inconsistent with the refactor.
This line claims CatalogService "keeps its repositories as a deliberate Clean Architecture carve-out," but the repo was collapsed to VSA with DbContext-direct handlers. Lines 233-240 correctly state "CatalogService previously used Clean Architecture (4 projects) but was collapsed to VSA." Please update line 14 to match the current state.
As per coding guidelines, "CatalogService is now Vertical Slice Architecture... There is NO IProductRepository or IProductReadStore — read handlers project inline via AsNoTracking() + .Select(...) into ProductDto."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 14, Update the README sentence that claims CatalogService "keeps its repositories as a deliberate Clean Architecture carve-out" to reflect the current refactor: state that CatalogService was collapsed to Vertical Slice Architecture (VSA) and no longer uses IProductRepository or IProductReadStore; mention handlers now take DbContext directly and read handlers perform AsNoTracking() with .Select(...) into ProductDto (i.e., there is NO IProductRepository/IProductReadStore). Keep wording consistent with lines 233-240 that already describe the collapse to VSA.docs/how-it-works.md (1)
71-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify a language for the fenced code block (MD040).
Add a language identifier (for example,
text) to the opening fence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/how-it-works.md` around lines 71 - 80, The fenced code block shown (the opening "```" before the ServiceName/ tree) is missing a language identifier which violates MD040; update the opening fence to include a language tag such as "text" (i.e., change "```" to "```text") so the block is a labeled code block and the linter stops flagging it; ensure the same closing "```" remains unchanged.CatalogService/Features/ReserveStock.cs (1)
42-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle concurrency exceptions to preserve the
falsecontract.A racing update can throw
DbUpdateConcurrencyExceptionat save time, but this handler currently bubbles the exception instead of returningfalseas documented.Proposed diff
product.AdjustStock(product.StockQuantity - request.Quantity); - await context.SaveChangesAsync(cancellationToken); + try + { + await context.SaveChangesAsync(cancellationToken); + } + catch (DbUpdateConcurrencyException) + { + return false; + } // Invalidate AFTER the save. See UpdateProduct for the cache-ordering rationale. await cache.InvalidateAsync(request.ProductId, cancellationToken); return true;#!/bin/bash # Verify current handler has no concurrency catch around SaveChangesAsync. rg -n -C3 'SaveChangesAsync|DbUpdateConcurrencyException|InvalidateAsync' CatalogService/Features/ReserveStock.csAs per coding guidelines
CLAUDE.md: optimistic concurrency is required and handler contracts should remain explicit under concurrency races.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/ReserveStock.cs` around lines 42 - 47, Wrap the call to context.SaveChangesAsync in a try/catch that catches DbUpdateConcurrencyException, returning false from the ReserveStock handler when that exception occurs; keep the AdjustStock call as-is, and ensure cache.InvalidateAsync(request.ProductId, cancellationToken) is only called after a successful save (i.e., do not call InvalidateAsync inside the catch). Reference the existing symbols product.AdjustStock, context.SaveChangesAsync, cache.InvalidateAsync and the ReserveStock handler to locate where to add the catch and return false.tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs (1)
12-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd mandatory AAA narrative comments in each test.
Each test still needs
// ARRANGE — ...,// ACT — ..., and// ASSERT — ...story markers to satisfy the enforced test format.As per coding guidelines
**/*.Tests*/**/*.cs: “Every test: Arrange → Act → Assert with // ARRANGE, // ACT, // ASSERT markers (all caps, em-dash explanation).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs` around lines 12 - 75, Add the mandatory AAA narrative comments to each test method by inserting three single-line comments above the arrange, act and assert sections: "// ARRANGE — set up test data" before creating command via ValidCommand() (for methods Validate_WithValidCommand_ReturnsNoErrors, Validate_WithEmptyProductId_ReturnsError, Validate_WithZeroQuantity_ReturnsError, Validate_WithNegativeQuantity_ReturnsError, Validate_WithQuantityAtUpperBound_ReturnsNoErrors, Validate_WithQuantityOverUpperBound_ReturnsError), "// ACT — execute validator" before calling _sut.Validate(command), and "// ASSERT — verify results" before the FluentAssertions assertions; keep wording consistent, use the exact all-caps "ARRANGE", "ACT", "ASSERT" and an em-dash plus short explanation.CatalogService/Features/UpdateProduct.cs (1)
50-52: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unused eager-loading from the update query.
Categoryis not used in this handler, so theIncludeadds unnecessary overhead on a hot write path.Proposed diff
- var product = await context.Products - .Include(p => p.Category) - .FirstOrDefaultAsync(p => p.Id == request.ProductId, cancellationToken); + var product = await context.Products + .FirstOrDefaultAsync(p => p.Id == request.ProductId, cancellationToken);#!/bin/bash # Verify Category is not consumed in this handler, so Include can be safely removed. rg -n -C2 'Include\(p => p.Category\)|product\.Category' CatalogService/Features/UpdateProduct.csAs per coding guidelines
**/*.cs: “Performance Rules … project-in-EF for reads …” and avoid unnecessary data-loading work on request paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/UpdateProduct.cs` around lines 50 - 52, The query in UpdateProduct handler eagerly loads Category via Include(p => p.Category) but Category isn't used; remove the unnecessary Include from the context.Products query (the var product = await context.Products...FirstOrDefaultAsync(...) call) to avoid extra overhead on the hot write path, leaving the FirstOrDefaultAsync predicate unchanged and verifying no later code in this method references product.Category before committing the change.docs/architecture.md (1)
67-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFenced code block missing language tag.
The code block starting at line 67 is missing a language identifier, triggering markdownlint MD040.
🔧 Suggested fix
-``` +```text ServiceName/ Features/ # One file per use case (command/query + validator + handler co-located). ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` around lines 67 - 77, The fenced code block that shows the project layout (starting with "ServiceName/" and listing Features/, Domain/, Infrastructure/, Endpoints/, Grpc/, Program.cs, ServiceName.csproj) is missing a language tag and triggers markdownlint MD040; update the opening fence for that block from ``` to a language-tagged fence such as ```text (or ```txt) so the block is explicitly marked as plain text and the linter warning is resolved..coderabbit.yaml (1)
45-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDangling cross-reference remains unresolved.
Line 48 still references
"**/Infrastructure/*Repository.cs" path_instructions below, but that section does not exist in this file. The only repository-related path instruction is**/Domain/I*Repository.csat lines 69-81.🔧 Proposed fix
- 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 for why and what to flag.As per coding guidelines, rule encodings must stay aligned and actionable across reviewer surfaces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yaml around lines 45 - 49, The comment references a non-existent section `"**/Infrastructure/*Repository.cs" path_instructions below`; fix this dangling cross-reference by either removing that quoted reference, replacing it with the correct existing section (`"**/Domain/I*Repository.cs"`) or add the missing `"**/Infrastructure/*Repository.cs"` path_instructions block to the file so the reference resolves; search for the exact string `"**/Infrastructure/*Repository.cs"` and update the surrounding sentence to point to an actual section or create the corresponding path_instructions block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CatalogService/Features/SearchProducts.cs`:
- Line 19: Add a FluentValidation validator class named
SearchProductsQueryValidator that inherits
AbstractValidator<SearchProductsQuery> and register it via DI (ensuring it's in
the same assembly/namespace so the Wolverine FluentValidation pipeline discovers
it). In the validator enforce that Query is not null/empty and trimmed length >
0 and has a reasonable max length (e.g., <= 500), that Page is >= 1, and that
PageSize is between 1 and a sensible upper bound (e.g., 1..100). Place the
validator alongside other validators (matching naming pattern
CreateProductCommandValidator/UpdateProductCommandValidator) so requests to
SearchProductsQuery are validated before the handler runs.
In `@CLAUDE.md`:
- Around line 58-63: The "Vertical Slice Architecture for every service"
paragraph conflicts with later guidance that treats cross-service pattern
differences as intentional; update the text so the repo adopts a single
consistent rule: either state that all five services (Catalog, Order, Payment,
Shipping, Notification) use the Vertical Slice Architecture (VSA) and
remove/replace mentions of CatalogService using Clean Architecture, or
conversely state that CatalogService intentionally uses Clean Architecture while
the other four use VSA and make that explicit; ensure you update the phrases
referencing "Clean Architecture for CatalogService", "VSA for the other four",
and the retired-diff rationale so the section and any later guidance are
consistent and reference the chosen pattern unambiguously.
- Line 65: The markdown fenced code block in CLAUDE.md that shows the
ServiceName/ tree is unlabeled (triggers MD040); update that fence from ``` to a
labeled fence such as ```text so the block is explicitly marked as plain text
(e.g., change the opening fence for the ServiceName/ tree snippet to ```text) to
satisfy markdownlint.
---
Outside diff comments:
In
`@tests/CatalogService.Tests.Unit/Application/CreateProductCommandValidatorTests.cs`:
- Around line 14-38: Add the mandated AAA narrative comments to each test: for
Validate_WithValidCommand_ReturnsNoErrors and
Validate_WithEmptyName_ReturnsError insert three comment lines marking the
phases above their blocks — use "// ARRANGE —" before the setup where
ValidCommand() is created, "// ACT —" before the _sut.Validate(command)
invocation, and "// ASSERT —" before the FluentAssertions checks
(result.IsValid... and result.Errors...). Ensure the markers are all caps, use
an em-dash and a short phase description, and place them in each test method
consistently.
---
Duplicate comments:
In @.coderabbit.yaml:
- Around line 45-49: The comment references a non-existent section
`"**/Infrastructure/*Repository.cs" path_instructions below`; fix this dangling
cross-reference by either removing that quoted reference, replacing it with the
correct existing section (`"**/Domain/I*Repository.cs"`) or add the missing
`"**/Infrastructure/*Repository.cs"` path_instructions block to the file so the
reference resolves; search for the exact string
`"**/Infrastructure/*Repository.cs"` and update the surrounding sentence to
point to an actual section or create the corresponding path_instructions block.
In `@CatalogService/Features/ReserveStock.cs`:
- Around line 42-47: Wrap the call to context.SaveChangesAsync in a try/catch
that catches DbUpdateConcurrencyException, returning false from the ReserveStock
handler when that exception occurs; keep the AdjustStock call as-is, and ensure
cache.InvalidateAsync(request.ProductId, cancellationToken) is only called after
a successful save (i.e., do not call InvalidateAsync inside the catch).
Reference the existing symbols product.AdjustStock, context.SaveChangesAsync,
cache.InvalidateAsync and the ReserveStock handler to locate where to add the
catch and return false.
In `@CatalogService/Features/UpdateProduct.cs`:
- Around line 50-52: The query in UpdateProduct handler eagerly loads Category
via Include(p => p.Category) but Category isn't used; remove the unnecessary
Include from the context.Products query (the var product = await
context.Products...FirstOrDefaultAsync(...) call) to avoid extra overhead on the
hot write path, leaving the FirstOrDefaultAsync predicate unchanged and
verifying no later code in this method references product.Category before
committing the change.
In `@docs/architecture.md`:
- Around line 67-77: The fenced code block that shows the project layout
(starting with "ServiceName/" and listing Features/, Domain/, Infrastructure/,
Endpoints/, Grpc/, Program.cs, ServiceName.csproj) is missing a language tag and
triggers markdownlint MD040; update the opening fence for that block from ``` to
a language-tagged fence such as ```text (or ```txt) so the block is explicitly
marked as plain text and the linter warning is resolved.
In `@docs/how-it-works.md`:
- Around line 71-80: The fenced code block shown (the opening "```" before the
ServiceName/ tree) is missing a language identifier which violates MD040; update
the opening fence to include a language tag such as "text" (i.e., change "```"
to "```text") so the block is a labeled code block and the linter stops flagging
it; ensure the same closing "```" remains unchanged.
In `@README.md`:
- Line 14: Update the README sentence that claims CatalogService "keeps its
repositories as a deliberate Clean Architecture carve-out" to reflect the
current refactor: state that CatalogService was collapsed to Vertical Slice
Architecture (VSA) and no longer uses IProductRepository or IProductReadStore;
mention handlers now take DbContext directly and read handlers perform
AsNoTracking() with .Select(...) into ProductDto (i.e., there is NO
IProductRepository/IProductReadStore). Keep wording consistent with lines
233-240 that already describe the collapse to VSA.
In
`@tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs`:
- Around line 12-75: Add the mandatory AAA narrative comments to each test
method by inserting three single-line comments above the arrange, act and assert
sections: "// ARRANGE — set up test data" before creating command via
ValidCommand() (for methods Validate_WithValidCommand_ReturnsNoErrors,
Validate_WithEmptyProductId_ReturnsError,
Validate_WithZeroQuantity_ReturnsError,
Validate_WithNegativeQuantity_ReturnsError,
Validate_WithQuantityAtUpperBound_ReturnsNoErrors,
Validate_WithQuantityOverUpperBound_ReturnsError), "// ACT — execute validator"
before calling _sut.Validate(command), and "// ASSERT — verify results" before
the FluentAssertions assertions; keep wording consistent, use the exact all-caps
"ARRANGE", "ACT", "ASSERT" and an em-dash plus short explanation.
In
`@tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs`:
- Around line 14-98: Each unit test (Validate_WithValidCommand_ReturnsNoErrors,
Validate_WithEmptyProductId_ReturnsError,
Validate_WithEmptySellerId_ReturnsError, Validate_WithEmptyName_ReturnsError,
Validate_WithNameOver200Chars_ReturnsError,
Validate_WithDescriptionOver2000Chars_ReturnsError,
Validate_WithZeroPrice_ReturnsError, Validate_WithNegativePrice_ReturnsError)
must include the AAA narrative markers: add a top-line comment "// ARRANGE — set
up test data and dependencies" before creating the command, a comment "// ACT —
execute _sut.Validate(command)" immediately before the call to
_sut.Validate(command), and a comment "// ASSERT — verify result expectations"
before the Should(). assertions so each test clearly follows Arrange → Act →
Assert.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5cdd5375-b00b-45e0-9161-d70e92ec7916
📒 Files selected for processing (84)
.claude/agents/architecture-reviewer.md.coderabbit.yamlCLAUDE.mdCatalogService/CatalogService.Application/CatalogService.Application.csprojCatalogService/CatalogService.Application/Commands/CreateProductCommand.csCatalogService/CatalogService.Application/Commands/ReserveStockCommand.csCatalogService/CatalogService.Application/Commands/UpdateProductCommand.csCatalogService/CatalogService.Application/Handlers/CreateProductHandler.csCatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.csCatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.csCatalogService/CatalogService.Application/Handlers/ReserveStockHandler.csCatalogService/CatalogService.Application/Handlers/SearchProductsHandler.csCatalogService/CatalogService.Application/Handlers/UpdateProductHandler.csCatalogService/CatalogService.Application/Interfaces/IProductReadStore.csCatalogService/CatalogService.Application/Queries/GetAllProductsQuery.csCatalogService/CatalogService.Application/Queries/GetProductByIdQuery.csCatalogService/CatalogService.Application/Queries/SearchProductsQuery.csCatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.csCatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.csCatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.csCatalogService/CatalogService.Domain/CatalogService.Domain.csprojCatalogService/CatalogService.Domain/Interfaces/IProductRepository.csCatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csprojCatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.csCatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.csCatalogService/CatalogService.csprojCatalogService/Domain/Category.csCatalogService/Domain/IProductCache.csCatalogService/Domain/Product.csCatalogService/Endpoints/CatalogEndpoints.csCatalogService/Features/CreateProduct.csCatalogService/Features/GetAllProducts.csCatalogService/Features/GetProductById.csCatalogService/Features/ReserveStock.csCatalogService/Features/SearchProducts.csCatalogService/Features/UpdateProduct.csCatalogService/Grpc/CatalogGrpcService.csCatalogService/Infrastructure/Caching/HybridProductCache.csCatalogService/Infrastructure/Data/CatalogDbContext.csCatalogService/Infrastructure/Data/CatalogDbContextFactory.csCatalogService/Infrastructure/DependencyInjection.csCatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.Designer.csCatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.csCatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.Designer.csCatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.csCatalogService/Infrastructure/Migrations/CatalogDbContextModelSnapshot.csCatalogService/Program.csCatalogService/Properties/launchSettings.jsonCatalogService/Protos/catalog.protoCatalogService/appsettings.Development.jsonCatalogService/appsettings.jsonNextAurora.AppHost/AppHost.csNextAurora.AppHost/NextAurora.AppHost.csprojNextAurora.slnxOrderService/Infrastructure/DependencyInjection.csOrderService/Infrastructure/GrpcCatalogClient.csOrderService/OrderService.csprojREADME.mdcodecov.ymldocs/STATUS.mddocs/architecture.mddocs/code-flows/catalogservice.mddocs/cqrs-data-access.mddocs/dotnet-10-features.mddocs/ef-core.mddocs/how-it-works.mddocs/performance-and-data-correctness.mddocs/project-decisions.mdtests/CatalogService.Tests.Integration/CatalogService.Tests.Integration.csprojtests/CatalogService.Tests.Integration/ProductAuthorizationTests.cstests/CatalogService.Tests.Integration/ProductCachingTests.cstests/CatalogService.Tests.Integration/ProductReadProjectionTests.cstests/CatalogService.Tests.Integration/ProductReadStoreTests.cstests/CatalogService.Tests.Unit/Application/CreateProductCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cstests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cstests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cstests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cstests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cstests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cstests/CatalogService.Tests.Unit/Builders/ProductBuilder.cstests/CatalogService.Tests.Unit/CatalogService.Tests.Unit.csproj
💤 Files with no reviewable changes (29)
- CatalogService/CatalogService.Application/Queries/GetProductByIdQuery.cs
- CatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.cs
- CatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csproj
- CatalogService/CatalogService.Application/Commands/UpdateProductCommand.cs
- CatalogService/CatalogService.Application/Interfaces/IProductReadStore.cs
- tests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cs
- CatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.cs
- tests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cs
- CatalogService/CatalogService.Domain/CatalogService.Domain.csproj
- tests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cs
- CatalogService/CatalogService.Application/Commands/CreateProductCommand.cs
- tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs
- CatalogService/CatalogService.Domain/Interfaces/IProductRepository.cs
- CatalogService/CatalogService.Application/Queries/SearchProductsQuery.cs
- CatalogService/CatalogService.Application/Queries/GetAllProductsQuery.cs
- CatalogService/CatalogService.Application/Handlers/ReserveStockHandler.cs
- CatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.cs
- CatalogService/CatalogService.Application/Handlers/UpdateProductHandler.cs
- CatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.cs
- CatalogService/CatalogService.Application/CatalogService.Application.csproj
- CatalogService/CatalogService.Application/Handlers/SearchProductsHandler.cs
- CatalogService/CatalogService.Application/Handlers/CreateProductHandler.cs
- tests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cs
- CatalogService/CatalogService.Application/Commands/ReserveStockCommand.cs
- CatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.cs
- tests/CatalogService.Tests.Integration/ProductReadStoreTests.cs
- CatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.cs
- CatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.cs
- tests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cs
emeraldleaf
force-pushed
the
refactor/catalog-vsa-collapse
branch
from
May 26, 2026 02:25
e23d15b to
c32c67b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/cqrs-data-access.md (1)
233-238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the handler inventory consistent with the new “no repository wrapper” rule.
This section declares DbContext-direct/no-wrapper as the hard rule, but the same document still describes repository-based command handlers elsewhere. Align all sections to one model.
As per coding guidelines, the canonical data-access shape is DbContext-direct handlers rather than repository wrappers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/cqrs-data-access.md` around lines 233 - 238, The document currently mixes two models — DbContext-direct handlers and repository-wrapper handlers — so make the handler inventory consistent by removing or converting any repository-based examples to the canonical DbContext-direct style described under "Query handlers project to DTO inline", "Command/event/saga handlers load tracked aggregates", and "No repository wrapper."; specifically locate any examples, diagrams or sections that mention or show I*Repository/IRepository wrappers and replace them with DbContext/DbSet usage, update example handler pseudocode to use AsNoTracking().Select(...) for reads and tracked loads + aggregate methods + SaveChangesAsync for commands/events/sagas, and ensure the statement "DbContext IS Unit-of-Work, DbSet<T> IS Repository" is reflected everywhere (including tests guidance) so all references to handler inventory use the DbContext-direct model.docs/ef-core.md (1)
457-464:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winChanged doc examples still reference removed repository/handler file paths.
These sections point to
CatalogService/Infrastructure/Repositories/ProductRepository.csandCatalogService/Features/*Handler.cs, which no longer match the documented VSA layout (Features/*.cs, DbContext-first handlers). Please update links/examples to current files (GetProductById.cs,UpdateProduct.cs, etc.).As per coding guidelines, “CatalogService … repository wrappers removed … handlers take CatalogDbContext directly.”
Also applies to: 721-728, 1066-1081
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ef-core.md` around lines 457 - 464, Update all doc examples and links that reference the removed repository layout (e.g., ProductRepository.cs and Handler.cs files) to the current VSA layout: replace references to ProductRepository.GetAllAsync with the DbContext-first handlers and files (e.g., GetProductById.cs, UpdateProduct.cs) and update sample code to call CatalogDbContext directly (e.g., using CatalogDbContext.Products with AsNoTracking(), Include(p => p.Category), OrderBy/Skip/Take, and ToListAsync). Ensure the three reported sections (around lines 457, 721, 1066) no longer point to CatalogService/Infrastructure/Repositories/ProductRepository.cs or CatalogService/Features/*Handler.cs but instead link to the new feature files (GetProductById.cs, UpdateProduct.cs) and show examples that use CatalogDbContext and the GetAllAsync behavior in that context.
♻️ Duplicate comments (12)
CLAUDE.md (1)
65-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language to the fenced code block.
The unlabeled fence triggers markdownlint (
MD040).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` at line 65, The fenced code block in CLAUDE.md is unlabeled (the closing ``` at line shown); update the opening fence for that code block to include an explicit language tag (for example ```bash, ```json, or ```text as appropriate) so the block is no longer unlabeled and MD040 is resolved; ensure the opening triple-backtick that matches the shown closing backtick is updated to include the chosen language..coderabbit.yaml (1)
47-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix dangling reference to missing
**/Infrastructure/*Repository.csinstructions.This points reviewers to a path block that is not present, so the guidance is incomplete. Either add that path block or update the reference to existing blocks.
As per coding guidelines, “rule encodings must stay aligned and actionable across reviewer surfaces (CLAUDE.md canonical +
.coderabbit.yamlpath instructions).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yaml around lines 47 - 49, The comment points to a dangling reference to the path instruction "**/Infrastructure/*Repository.cs" (paired with "**/Domain/I*Repository.cs") that does not exist in the rule set; fix by either adding the missing path block for "**/Infrastructure/*Repository.cs" with the appropriate guidance or update the existing reference to point to an existing path block, and make sure the CLAUDE.md canonical rule encoding and the .coderabbit.yaml path_instructions remain aligned and actionable (update any mention of "**/Domain/I*Repository.cs" if needed to match the new or corrected block).CatalogService/Features/UpdateProduct.cs (1)
50-52: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDrop the unused
Include(p => p.Category)from the update load query.Line 51 eagerly loads
Category, but no subsequent code uses it in this handler.Proposed fix
var product = await context.Products - .Include(p => p.Category) .FirstOrDefaultAsync(p => p.Id == request.ProductId, cancellationToken);#!/bin/bash # Verify Category navigation is only eagerly loaded, not consumed, in this handler. rg -n "Include\\(p => p.Category\\)|product\\.Category|Category" CatalogService/Features/UpdateProduct.cs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/UpdateProduct.cs` around lines 50 - 52, The query in the UpdateProduct handler eagerly includes Category but the loaded navigation is never used; remove the unnecessary Include(p => p.Category) from the Product load query in the method that assigns var product (the query against context.Products in UpdateProduct.cs) so the handler simply awaits context.Products.FirstOrDefaultAsync(p => p.Id == request.ProductId, cancellationToken); leaving all other logic unchanged.CatalogService/Features/ReserveStock.cs (1)
43-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle optimistic-concurrency conflicts to preserve the
boolcontract.Line 43 can throw
DbUpdateConcurrencyException, so the handler may fail the RPC instead of returningfalseas documented on Lines 13-17.Proposed fix
product.AdjustStock(product.StockQuantity - request.Quantity); - await context.SaveChangesAsync(cancellationToken); + try + { + await context.SaveChangesAsync(cancellationToken); + } + catch (DbUpdateConcurrencyException) + { + return false; + } // Invalidate AFTER the save. See UpdateProduct for the cache-ordering rationale. await cache.InvalidateAsync(request.ProductId, cancellationToken); return true;#!/bin/bash # Verify documented concurrency contract vs implementation in ReserveStock handler. rg -n "Concurrency story|DbUpdateConcurrencyException|SaveChangesAsync|return false" CatalogService/Features/ReserveStock.cs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/ReserveStock.cs` around lines 43 - 47, The SaveChangesAsync call in the ReserveStock handler can throw DbUpdateConcurrencyException which currently would bubble up and fail the RPC instead of returning false as the method contract requires; update the ReserveStock handler to catch DbUpdateConcurrencyException around the await context.SaveChangesAsync(cancellationToken), return false from the catch, and ensure cache.InvalidateAsync(request.ProductId, cancellationToken) only runs when the save succeeds; rethrow or let other exceptions propagate as before.CatalogService/Features/SearchProducts.cs (1)
19-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject empty/whitespace search terms before building the
ILikepattern.At Line 32, an empty query becomes
%%, turning this into an unbounded “match-all” search path.Proposed fix
+using FluentValidation; using CatalogService.Infrastructure.Data; using Microsoft.EntityFrameworkCore; using NextAurora.Contracts.DTOs; @@ public record SearchProductsQuery(string Query, int Page = 1, int PageSize = 50); + +public class SearchProductsQueryValidator : AbstractValidator<SearchProductsQuery> +{ + public SearchProductsQueryValidator() + { + RuleFor(x => x.Query).NotEmpty().Must(q => !string.IsNullOrWhiteSpace(q)).MaximumLength(500); + } +} @@ public async Task<IReadOnlyList<ProductDto>> HandleAsync(SearchProductsQuery request, CancellationToken cancellationToken) { + if (string.IsNullOrWhiteSpace(request.Query)) + return []; + var safePage = request.Page < 1 ? 1 : request.Page; var safePageSize = request.PageSize is < 1 or > 100 ? 50 : request.PageSize; @@ - var pattern = $"%{request.Query}%"; + var pattern = $"%{request.Query.Trim()}%";#!/bin/bash # Verify absence of guard/validator for empty search queries. rg -n "SearchProductsQueryValidator|string.IsNullOrWhiteSpace\\(request.Query\\)|var pattern = \\$\"%\\{request.Query\\}%\"" CatalogService/Features/SearchProducts.cs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/SearchProducts.cs` around lines 19 - 35, The handler currently builds an ILike pattern using request.Query which means an empty or whitespace query becomes "%%" and matches everything; inside SearchProductsHandler.HandleAsync, add a guard that checks string.IsNullOrWhiteSpace(request.Query) (or request.Query?.Trim().Length == 0) and immediately return an empty IReadOnlyList<ProductDto> before computing pattern/EF query, otherwise continue and build pattern with the trimmed query (e.g., use request.Query.Trim() when forming pattern).docs/architecture.md (1)
67-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced code block.
The code fence opening should include a language (e.g., ```text) to satisfy MD040.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` at line 67, The fenced code block in docs/architecture.md currently opens with three backticks only; to satisfy MD040 add a language tag to the opening fence (for example change ``` to ```text or ```markdown) so the code block is language-specified; update the opening backticks for the fenced block (the triple-backtick fence) accordingly.README.md (1)
14-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve architecture contradiction at the top of README.
This section still says CatalogService is a Clean Architecture carve-out with repositories, but later (Line 233 onward) the file states VSA everywhere. Keep one canonical statement to avoid misleading contributors.
As per coding guidelines, CatalogService is documented as VSA with DbContext-direct handlers and no repository wrappers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 14 - 16, The README contains a contradictory description claiming CatalogService is a Clean Architecture carve-out with repositories while elsewhere stating all services use VSA; update the top-level architecture paragraphs so they consistently state that CatalogService follows the same VSA pattern (handlers take DbContext directly, no IFooRepository wrappers) per the coding guidelines, remove or rewrite the "Clean Architecture carve-out" sentence and any wording that implies CatalogService uses repository abstractions, and ensure references to STATUS.md and CLAUDE.md remain correct while preserving the note about the v1-repository-pattern tag as historical only.docs/STATUS.md (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
Last updatedto today’s date.
docs/STATUS.mdchanged in this PR, but the banner still says2026-05-25instead of2026-05-26.As per coding guidelines, when
docs/STATUS.mdchanges, the “Last updated” date must be today.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/STATUS.md` at line 5, Update the "**Last updated:**" line in docs/STATUS.md to today's date by replacing the existing "2026-05-25" timestamp in the line that currently reads "**Last updated:** 2026-05-25 (CatalogService Clean Architecture → VSA collapse; carve-out retired — all five services now share one shape)" with "2026-05-26" so the banner reflects the current date.docs/how-it-works.md (1)
71-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
Line 71 should use a language tag (e.g., ```text) to satisfy MD040 and keep lint clean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/how-it-works.md` around lines 71 - 80, The fenced code block that contains the directory listing starting with "ServiceName/" is missing a language identifier, triggering MD040; update the opening fence from ``` to a tagged fence such as ```text (or ```markdown) so the block becomes ```text and retains the same content; ensure only the opening fence is changed and the closing ``` remains untouched.docs/performance-and-data-correctness.md (1)
129-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCache-handler links still target non-existent
*Handler.csfiles.The changed docs still reference
.../Features/GetProductByIdHandler.cs-style paths. In this repo shape, these should point toGetProductById.cs,UpdateProduct.cs, andReserveStock.cs.As per coding guidelines, “Features/ contains one file per use case (command/query record + validator + handler co-located).”
Also applies to: 274-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/performance-and-data-correctness.md` at line 129, Update the broken doc links that point to non-existent Handler files: replace references to GetProductByIdHandler, UpdateProductHandler, and ReserveStockHandler with the actual feature filenames GetProductById.cs, UpdateProduct.cs, and ReserveStock.cs (e.g., adjust the links currently pointing to ../CatalogService/Features/GetProductByIdHandler.cs, ../CatalogService/Features/UpdateProductHandler.cs, ../CatalogService/Features/ReserveStockHandler.cs). Make the same replacements for the other occurrences noted (around the section lines referenced, e.g., the block at 274-283) so all Features/ links follow the “one file per use case” convention used by the codebase.docs/project-decisions.md (1)
269-269:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVisible link label should match the updated target path.
These labels still say
CatalogService.Api/Program.cswhile linking to../CatalogService/Program.cs. Please align label text with the target.Also applies to: 535-535
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/project-decisions.md` at line 269, Update the visible link label so it matches the actual target path: replace the label text "CatalogService.Api/Program.cs" with "CatalogService/Program.cs" (to match the target "../CatalogService/Program.cs") in the docs entry shown and the other occurrence noted (line 535 equivalent); ensure both the link text and href remain consistent.docs/dotnet-10-features.md (1)
138-138:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale EF read-path reference still points to removed repository wrapper.
Line 138 links to
CatalogService/Infrastructure/Repositories/ProductRepository.cs, but Catalog now documents handler-inline projection withCatalogDbContextand no repository wrapper. Please update this reference to the current handler path/pattern.As per coding guidelines, “handlers take DbContext directly (no IFooRepository wrappers); read paths must be AsNoTracking() + DTO projection inside IQueryable.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dotnet-10-features.md` at line 138, The docs still link to the old repository wrapper (ProductRepository.cs); update the paragraph and link to the current handler that demonstrates handler-inline projection using CatalogDbContext (the handler file that performs .AsNoTracking() and projects into DTOs inside the IQueryable), remove the reference to a repository wrapper, and change the wording to state “handlers take CatalogDbContext directly (no IFooRepository wrappers); read paths must use .AsNoTracking() + DTO projection inside the IQueryable.” Ensure the link points to the handler file showing this exact pattern and mention CatalogDbContext and the AsNoTracking() + projection pattern by name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 58-63: The document currently contradicts itself by declaring
"Vertical Slice Architecture for every service" (the VSA statement around the
"Vertical Slice Architecture for every service" paragraph) while later lines
(the paragraph described at lines ~100-102) still assert intentional
cross-service pattern differences; update CLAUDE.md to make the canonical rule
consistent by removing or rewriting the later paragraph so it matches the
VSA-for-all-services rule: delete the sentence(s) that describe cross-service
pattern differences and, if needed, replace them with a short line that
reiterates the single-project, feature-organized VSA rule used for all five
services (Catalog, Order, Payment, Shipping, Notification) and remove any
references to Clean Architecture being used for CatalogService.
In `@docs/dotnet-10-features.md`:
- Line 27: The markdown link labels in docs/dotnet-10-features.md are
inconsistent with their targets (e.g., the label "CatalogService.Api/Program.cs"
points to "../CatalogService/Program.cs#L72"); update the link text to match the
actual target path (for example change "CatalogService.Api/Program.cs" to
"CatalogService/Program.cs") for the listed occurrences (around lines 27, 81,
101, 156, 190) so labels accurately reflect the destination.
---
Outside diff comments:
In `@docs/cqrs-data-access.md`:
- Around line 233-238: The document currently mixes two models —
DbContext-direct handlers and repository-wrapper handlers — so make the handler
inventory consistent by removing or converting any repository-based examples to
the canonical DbContext-direct style described under "Query handlers project to
DTO inline", "Command/event/saga handlers load tracked aggregates", and "No
repository wrapper."; specifically locate any examples, diagrams or sections
that mention or show I*Repository/IRepository wrappers and replace them with
DbContext/DbSet usage, update example handler pseudocode to use
AsNoTracking().Select(...) for reads and tracked loads + aggregate methods +
SaveChangesAsync for commands/events/sagas, and ensure the statement "DbContext
IS Unit-of-Work, DbSet<T> IS Repository" is reflected everywhere (including
tests guidance) so all references to handler inventory use the DbContext-direct
model.
In `@docs/ef-core.md`:
- Around line 457-464: Update all doc examples and links that reference the
removed repository layout (e.g., ProductRepository.cs and Handler.cs files) to
the current VSA layout: replace references to ProductRepository.GetAllAsync with
the DbContext-first handlers and files (e.g., GetProductById.cs,
UpdateProduct.cs) and update sample code to call CatalogDbContext directly
(e.g., using CatalogDbContext.Products with AsNoTracking(), Include(p =>
p.Category), OrderBy/Skip/Take, and ToListAsync). Ensure the three reported
sections (around lines 457, 721, 1066) no longer point to
CatalogService/Infrastructure/Repositories/ProductRepository.cs or
CatalogService/Features/*Handler.cs but instead link to the new feature files
(GetProductById.cs, UpdateProduct.cs) and show examples that use
CatalogDbContext and the GetAllAsync behavior in that context.
---
Duplicate comments:
In @.coderabbit.yaml:
- Around line 47-49: The comment points to a dangling reference to the path
instruction "**/Infrastructure/*Repository.cs" (paired with
"**/Domain/I*Repository.cs") that does not exist in the rule set; fix by either
adding the missing path block for "**/Infrastructure/*Repository.cs" with the
appropriate guidance or update the existing reference to point to an existing
path block, and make sure the CLAUDE.md canonical rule encoding and the
.coderabbit.yaml path_instructions remain aligned and actionable (update any
mention of "**/Domain/I*Repository.cs" if needed to match the new or corrected
block).
In `@CatalogService/Features/ReserveStock.cs`:
- Around line 43-47: The SaveChangesAsync call in the ReserveStock handler can
throw DbUpdateConcurrencyException which currently would bubble up and fail the
RPC instead of returning false as the method contract requires; update the
ReserveStock handler to catch DbUpdateConcurrencyException around the await
context.SaveChangesAsync(cancellationToken), return false from the catch, and
ensure cache.InvalidateAsync(request.ProductId, cancellationToken) only runs
when the save succeeds; rethrow or let other exceptions propagate as before.
In `@CatalogService/Features/SearchProducts.cs`:
- Around line 19-35: The handler currently builds an ILike pattern using
request.Query which means an empty or whitespace query becomes "%%" and matches
everything; inside SearchProductsHandler.HandleAsync, add a guard that checks
string.IsNullOrWhiteSpace(request.Query) (or request.Query?.Trim().Length == 0)
and immediately return an empty IReadOnlyList<ProductDto> before computing
pattern/EF query, otherwise continue and build pattern with the trimmed query
(e.g., use request.Query.Trim() when forming pattern).
In `@CatalogService/Features/UpdateProduct.cs`:
- Around line 50-52: The query in the UpdateProduct handler eagerly includes
Category but the loaded navigation is never used; remove the unnecessary
Include(p => p.Category) from the Product load query in the method that assigns
var product (the query against context.Products in UpdateProduct.cs) so the
handler simply awaits context.Products.FirstOrDefaultAsync(p => p.Id ==
request.ProductId, cancellationToken); leaving all other logic unchanged.
In `@CLAUDE.md`:
- Line 65: The fenced code block in CLAUDE.md is unlabeled (the closing ``` at
line shown); update the opening fence for that code block to include an explicit
language tag (for example ```bash, ```json, or ```text as appropriate) so the
block is no longer unlabeled and MD040 is resolved; ensure the opening
triple-backtick that matches the shown closing backtick is updated to include
the chosen language.
In `@docs/architecture.md`:
- Line 67: The fenced code block in docs/architecture.md currently opens with
three backticks only; to satisfy MD040 add a language tag to the opening fence
(for example change ``` to ```text or ```markdown) so the code block is
language-specified; update the opening backticks for the fenced block (the
triple-backtick fence) accordingly.
In `@docs/dotnet-10-features.md`:
- Line 138: The docs still link to the old repository wrapper
(ProductRepository.cs); update the paragraph and link to the current handler
that demonstrates handler-inline projection using CatalogDbContext (the handler
file that performs .AsNoTracking() and projects into DTOs inside the
IQueryable), remove the reference to a repository wrapper, and change the
wording to state “handlers take CatalogDbContext directly (no IFooRepository
wrappers); read paths must use .AsNoTracking() + DTO projection inside the
IQueryable.” Ensure the link points to the handler file showing this exact
pattern and mention CatalogDbContext and the AsNoTracking() + projection pattern
by name.
In `@docs/how-it-works.md`:
- Around line 71-80: The fenced code block that contains the directory listing
starting with "ServiceName/" is missing a language identifier, triggering MD040;
update the opening fence from ``` to a tagged fence such as ```text (or
```markdown) so the block becomes ```text and retains the same content; ensure
only the opening fence is changed and the closing ``` remains untouched.
In `@docs/performance-and-data-correctness.md`:
- Line 129: Update the broken doc links that point to non-existent Handler
files: replace references to GetProductByIdHandler, UpdateProductHandler, and
ReserveStockHandler with the actual feature filenames GetProductById.cs,
UpdateProduct.cs, and ReserveStock.cs (e.g., adjust the links currently pointing
to ../CatalogService/Features/GetProductByIdHandler.cs,
../CatalogService/Features/UpdateProductHandler.cs,
../CatalogService/Features/ReserveStockHandler.cs). Make the same replacements
for the other occurrences noted (around the section lines referenced, e.g., the
block at 274-283) so all Features/ links follow the “one file per use case”
convention used by the codebase.
In `@docs/project-decisions.md`:
- Line 269: Update the visible link label so it matches the actual target path:
replace the label text "CatalogService.Api/Program.cs" with
"CatalogService/Program.cs" (to match the target "../CatalogService/Program.cs")
in the docs entry shown and the other occurrence noted (line 535 equivalent);
ensure both the link text and href remain consistent.
In `@docs/STATUS.md`:
- Line 5: Update the "**Last updated:**" line in docs/STATUS.md to today's date
by replacing the existing "2026-05-25" timestamp in the line that currently
reads "**Last updated:** 2026-05-25 (CatalogService Clean Architecture → VSA
collapse; carve-out retired — all five services now share one shape)" with
"2026-05-26" so the banner reflects the current date.
In `@README.md`:
- Around line 14-16: The README contains a contradictory description claiming
CatalogService is a Clean Architecture carve-out with repositories while
elsewhere stating all services use VSA; update the top-level architecture
paragraphs so they consistently state that CatalogService follows the same VSA
pattern (handlers take DbContext directly, no IFooRepository wrappers) per the
coding guidelines, remove or rewrite the "Clean Architecture carve-out" sentence
and any wording that implies CatalogService uses repository abstractions, and
ensure references to STATUS.md and CLAUDE.md remain correct while preserving the
note about the v1-repository-pattern tag as historical only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1348ac76-ebe0-46f4-8c19-bd4d1a03e4f2
📒 Files selected for processing (84)
.claude/agents/architecture-reviewer.md.coderabbit.yamlCLAUDE.mdCatalogService/CatalogService.Application/CatalogService.Application.csprojCatalogService/CatalogService.Application/Commands/CreateProductCommand.csCatalogService/CatalogService.Application/Commands/ReserveStockCommand.csCatalogService/CatalogService.Application/Commands/UpdateProductCommand.csCatalogService/CatalogService.Application/Handlers/CreateProductHandler.csCatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.csCatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.csCatalogService/CatalogService.Application/Handlers/ReserveStockHandler.csCatalogService/CatalogService.Application/Handlers/SearchProductsHandler.csCatalogService/CatalogService.Application/Handlers/UpdateProductHandler.csCatalogService/CatalogService.Application/Interfaces/IProductReadStore.csCatalogService/CatalogService.Application/Queries/GetAllProductsQuery.csCatalogService/CatalogService.Application/Queries/GetProductByIdQuery.csCatalogService/CatalogService.Application/Queries/SearchProductsQuery.csCatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.csCatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.csCatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.csCatalogService/CatalogService.Domain/CatalogService.Domain.csprojCatalogService/CatalogService.Domain/Interfaces/IProductRepository.csCatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csprojCatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.csCatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.csCatalogService/CatalogService.csprojCatalogService/Domain/Category.csCatalogService/Domain/IProductCache.csCatalogService/Domain/Product.csCatalogService/Endpoints/CatalogEndpoints.csCatalogService/Features/CreateProduct.csCatalogService/Features/GetAllProducts.csCatalogService/Features/GetProductById.csCatalogService/Features/ReserveStock.csCatalogService/Features/SearchProducts.csCatalogService/Features/UpdateProduct.csCatalogService/Grpc/CatalogGrpcService.csCatalogService/Infrastructure/Caching/HybridProductCache.csCatalogService/Infrastructure/Data/CatalogDbContext.csCatalogService/Infrastructure/Data/CatalogDbContextFactory.csCatalogService/Infrastructure/DependencyInjection.csCatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.Designer.csCatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.csCatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.Designer.csCatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.csCatalogService/Infrastructure/Migrations/CatalogDbContextModelSnapshot.csCatalogService/Program.csCatalogService/Properties/launchSettings.jsonCatalogService/Protos/catalog.protoCatalogService/appsettings.Development.jsonCatalogService/appsettings.jsonNextAurora.AppHost/AppHost.csNextAurora.AppHost/NextAurora.AppHost.csprojNextAurora.slnxOrderService/Infrastructure/DependencyInjection.csOrderService/Infrastructure/GrpcCatalogClient.csOrderService/OrderService.csprojPaymentService/Infrastructure/PaymentRecoveryJob.csREADME.mddocs/STATUS.mddocs/architecture.mddocs/code-flows/catalogservice.mddocs/cqrs-data-access.mddocs/dotnet-10-features.mddocs/ef-core.mddocs/how-it-works.mddocs/performance-and-data-correctness.mddocs/project-decisions.mdtests/CatalogService.Tests.Integration/CatalogService.Tests.Integration.csprojtests/CatalogService.Tests.Integration/ProductAuthorizationTests.cstests/CatalogService.Tests.Integration/ProductCachingTests.cstests/CatalogService.Tests.Integration/ProductReadProjectionTests.cstests/CatalogService.Tests.Integration/ProductReadStoreTests.cstests/CatalogService.Tests.Unit/Application/CreateProductCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cstests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cstests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cstests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cstests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cstests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cstests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cstests/CatalogService.Tests.Unit/Builders/ProductBuilder.cstests/CatalogService.Tests.Unit/CatalogService.Tests.Unit.csproj
💤 Files with no reviewable changes (29)
- CatalogService/CatalogService.Application/Queries/GetProductByIdQuery.cs
- CatalogService/CatalogService.Application/Commands/CreateProductCommand.cs
- CatalogService/CatalogService.Application/Queries/SearchProductsQuery.cs
- tests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cs
- CatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.cs
- CatalogService/CatalogService.Application/Handlers/CreateProductHandler.cs
- CatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.cs
- CatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.cs
- tests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cs
- CatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.cs
- CatalogService/CatalogService.Application/Handlers/UpdateProductHandler.cs
- tests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cs
- CatalogService/CatalogService.Application/Handlers/ReserveStockHandler.cs
- CatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.cs
- CatalogService/CatalogService.Application/Handlers/SearchProductsHandler.cs
- CatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.cs
- CatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csproj
- CatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.cs
- tests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cs
- CatalogService/CatalogService.Application/CatalogService.Application.csproj
- CatalogService/CatalogService.Domain/Interfaces/IProductRepository.cs
- CatalogService/CatalogService.Application/Commands/UpdateProductCommand.cs
- CatalogService/CatalogService.Domain/CatalogService.Domain.csproj
- CatalogService/CatalogService.Application/Commands/ReserveStockCommand.cs
- tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs
- tests/CatalogService.Tests.Integration/ProductReadStoreTests.cs
- CatalogService/CatalogService.Application/Queries/GetAllProductsQuery.cs
- tests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cs
- CatalogService/CatalogService.Application/Interfaces/IProductReadStore.cs
emeraldleaf
force-pushed
the
refactor/catalog-vsa-collapse
branch
2 times, most recently
from
May 26, 2026 03:17
60caf66 to
92ee0ce
Compare
…ngle VSA project
The simplicity refactor (refactor/simplicity-drop-repos-integration-tests) dropped
repository wrappers in 4 services but kept CatalogService on Clean Architecture as a
"carve-out" because of the layer-rule constraint (Application couldn't reference
Infrastructure where CatalogDbContext lived). On reflection that carve-out was
infrastructure-shape deciding architecture-shape, not architecture earning its keep:
at ~2k LOC and 2 aggregates CatalogService didn't hit the threshold where build-time
layer enforcement pays for itself. "We calibrate per service" was a weaker story
than "one consistent shape across the repo."
This commit collapses CatalogService to the same VSA shape as the other four services.
Code changes:
- DELETED 4 sub-csprojs (CatalogService.Domain, .Application, .Infrastructure, .Api);
added single CatalogService.csproj at the service root.
- Folder structure now matches Order/Payment/Shipping/Notification:
Features/ (CreateProduct, UpdateProduct, ReserveStock, GetProductById,
GetAllProducts, SearchProducts — command/query + validator +
handler per file)
Domain/ (Product, Category aggregates + IProductCache port)
Infrastructure/ (CatalogDbContext + HybridProductCache + Migrations + DI)
Endpoints/ (REST surface)
Grpc/ (gRPC server peer for OrderService's gRPC client)
Protos/ (catalog.proto)
Program.cs (composition root)
- DELETED IProductRepository (Domain), IProductReadStore (Application), and their
ProductRepository / ProductReadStore impls. Handlers now take CatalogDbContext
directly: read handlers project inline via AsNoTracking().Select(ProductDto);
write handlers load tracked, mutate via aggregate methods, SaveChangesAsync.
- IProductCache survives — passes consumer-substitution (test fakes for HybridCache).
- Proto namespace updated: csharp_namespace "CatalogService.Api.Grpc" → "CatalogService.Grpc".
OrderService's gRPC client + DI updated to match.
- All namespaces flattened: Domain.Entities → Domain; Application.Commands/Queries/
Handlers/Validators → Features; Api.Endpoints → Endpoints; Api.Services → Grpc.
- Read handlers explicitly AddScoped'd in AddCatalogInfrastructure (Wolverine handler
discovery doesn't populate IServiceCollection — see CLAUDE.md "Communication
Patterns → Wolverine handler discovery is NOT DI registration").
Solution + dependent project updates:
- NextAurora.slnx: 4 CatalogService project entries → 1.
- NextAurora.AppHost.csproj: ProjectReference path updated.
- NextAurora.AppHost/AppHost.cs: Projects.CatalogService_Api → Projects.CatalogService.
- OrderService.csproj: Protobuf Include path updated to ../CatalogService/Protos/.
Test changes:
- DELETED 6 mock-based unit tests (CreateProductHandlerTests, UpdateProductHandlerTests,
ReserveStockHandlerTests, GetProductByIdHandlerTests, GetAllProductsHandlerTests,
SearchProductsHandlerTests — all used Substitute.For<IProductRepository> or
<IProductReadStore>). Coverage of those handler paths is provided by:
* ProductCachingTests (API-level read + cache + invalidate)
* ProductAuthorizationTests (IDOR + rejected-write invariants)
* ProductReadProjectionTests (renamed from ProductReadStoreTests — refactored
to resolve handlers directly from DI, preserves projection-in-isolation coverage)
- KEPT pure-domain unit tests: CreateProductCommandValidatorTests, ProductBuilder helper.
- CatalogService.Tests.Unit + Integration csproj references collapsed to single project.
- Build clean (0 warnings, 0 errors); 98 unit tests pass (down 23 from 121 — the deleted
CatalogService mock tests).
Carve-out retirement everywhere:
- CLAUDE.md: removed "Exception — CatalogService (Clean Architecture variant)" bullet
+ "Repository interfaces are NOT justified by this rule" updated to drop the
IProductReadStore mention. Project Structure section rewritten as "VSA everywhere
with a promotion signal" (5+ aggregates → consider Clean Architecture) instead of
"we calibrate per service."
- .coderabbit.yaml: removed IProductReadStore + CatalogService/**/*.cs Clean Architecture
path_instructions; replaced with VSA-shape rules for CatalogService.
- .claude/agents/architecture-reviewer.md: pattern checklist updated. "When reviewing
query handlers" / "write handlers" / "Domain folders" / "tests" all dropped the
"VSA services only" qualifier and the CatalogService exception — same rule applies
to all five services now.
- README.md: project tree rewritten — CatalogService entry now shows VSA layout.
Architecture description updated to "one shape across all five services."
- docs/architecture.md: "Per-Service Architecture" section rewritten; "EF Core Read/Write
Method Split" rewritten to show handler-direct DbContext shape; design-patterns table
updated; IProductCache file path updated.
- docs/how-it-works.md: project tree updated; "Per-Service Architecture" section
rewritten as "Vertical Slice Architecture" (everywhere); Domain-folder description
updated.
- docs/cqrs-data-access.md: "Canonical shape per architecture style" rewritten as
"Canonical shape — DbContext directly, no repository wrapper"; handler-inventory
tables updated; "Key principles" rewritten.
- docs/ef-core.md: §1 Overview rewritten; §7.4 canonical shape, §8 write-side, §9 read/
write split, §10 repository pattern all rewritten for DbContext-direct; FAQ updated.
- docs/performance-and-data-correctness.md: "Decision: read/write method split" rewritten;
Clean Architecture variant subsection deleted.
- docs/code-flows/catalogservice.md: architecture intro rewritten; Mermaid diagram
participants Repo/RS → Ctx (CatalogDbContext); "Read/write data-access split"
subgraph rewritten; file inventory updated.
- docs/STATUS.md: "Last updated" reflects this collapse; entry added to "Recently
landed"; "Open issues" gains the EF migration FQN snapshot-drift note (latent
issue shared with OrderService since the original VSA refactor — silent at runtime,
bites only when a new migration is generated).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…verage Lifts patch coverage on the CatalogService VSA-collapse PR by covering branches the integration suite doesn't reach today: - UpdateProductCommandValidatorTests (8 tests) — restored; got deleted with the mocked-handler tests in the collapse, but the validator is pure FluentValidation logic that legitimately belongs in the unit tier (per CLAUDE.md "Testing": "Unit tests are reserved for pure domain logic — FluentValidation rules"). - ReserveStockCommandValidatorTests (6 tests) — restored for the same reason, including a boundary test for the 10_000-quantity cap. - Two new integration tests in ProductReadProjectionTests covering the pagination clamp branches in GetAllProductsHandler + SearchProductsHandler: negative page, oversized PageSize, and the long-arithmetic skip-offset overflow guard that returns [] before touching the DB. All three branches were uncovered — existing tests only pass valid inputs. Net: 14 new unit tests + 2 new integration tests; CatalogService.Tests.Unit now 29 passing (was 15). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ntegration Closes the two biggest patch-coverage gaps on the CatalogService VSA-collapse PR: - PostProduct_byOwner_returns201AndPersistsProduct — POST /api/v1/products with the authenticated test client. Covers CreateProductHandler.HandleAsync end-to- end: the endpoint's JWT-vs-body check, the FluentValidation pipeline (validator is unit-tested separately), the handler's context.Products.AddAsync + SaveChangesAsync, and the 201-with-Id-body response shape. Parses the response with JsonDocument to avoid CA1812 noise on a typed DTO that's only built via reflection. - ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCache — resolves IMessageBus from the test container scope and dispatches ReserveStockCommand directly. Same handler invocation path the gRPC server uses (CatalogGrpcService translates each gRPC request into bus.InvokeAsync<bool>(command, ct)). Asserts the handler's full happy path: load tracked → AdjustStock(remaining) → save, with the IsAvailable invariant preserved. Together these cover the two handlers that integration tests didn't reach before — closing the gap that the mocked-handler unit tests previously contributed coverage for. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
emeraldleaf
force-pushed
the
refactor/catalog-vsa-collapse
branch
from
May 26, 2026 04:07
92ee0ce to
a8fc620
Compare
…hing knowledge graphs
…es + doc sweep CodeRabbit flagged 17 findings on PR #31. Resolved in two layers: CODE CORRECTNESS (4 real issues): - OrderService/Features/GetOrderById.cs (Major — IDOR): the existing endpoint + handler had NO buyer-scope check. Any authenticated user could read any other user's order by ID. Fix follows the canonical pattern from ShippingService.GetShipmentByOrder (and CLAUDE.md "Security Requirements"): * GetOrderByIdQuery now carries RequestingBuyerId * Endpoint extracts the JWT NameIdentifier claim, parses to Guid, passes it into the query (403 if claim missing/invalid; caller can't supply RequestingBuyerId via URL or body) * Handler's EF Where clause filters by BOTH OrderId AND BuyerId — non- owner rows never cross the wire from Postgres * Endpoint maps null → 404, NOT 403 — anti-enumeration property * Added integration test GetSummaryByIdAsync_returns_null_for_non_owner_buyer_IDOR_protection seeding an order for buyer A, requesting as buyer B, asserting null * Updated the two existing integration tests to pass RequestingBuyerId - CatalogService/Features/SearchProducts.cs (Major): empty/whitespace Query becomes `%%` which ILike matches against EVERY product. Without the guard, an empty search request hits a full-table scan (capped by pagination, but still unintended). Added early-return on string.IsNullOrWhiteSpace. - CatalogService/Features/ReserveStock.cs (Major): the xmin token's DbUpdateConcurrencyException was un-caught. Now wrapped in try/catch returning false on the loser side, so two concurrent reservations don't surface a 500 to the buyer via the gRPC path; the saga's compensation flow runs cleanly. - CatalogService/Features/UpdateProduct.cs (Trivial): removed unused .Include(p => p.Category). The handler only writes UpdateDetails(name, description, price) + SellerId — never touches the Category nav. Removing the Include eliminates a wasted LEFT JOIN per update. DOC + CONFIG SWEEP (13 stale references): - .coderabbit.yaml: removed dangling reference to "**/Infrastructure/ *Repository.cs" path_instructions section that no longer exists (and shouldn't — no service has repository impls anymore) - docs/STATUS.md: bumped "Last updated:" from 2026-05-25 to 2026-05-26 - docs/architecture.md (line ~70) + docs/how-it-works.md (line ~74): added `text` language tag to two fenced code blocks (MD040 lint) - docs/ef-core.md: 4 stale examples updated * Section 7 ProductRepository.GetAllAsync → GetAllProductsHandler with inline projection * Section 13 (pagination) same refactor * Section on cache invalidation: UpdateProductHandler example rewritten to use DbContext directly (no repository.GetByIdAsync / repository.UpdateAsync) * FAQ "AsNoTracking everywhere?" answer rewritten to describe handler- shape split rather than shared-repository-method tradeoff - docs/performance-and-data-correctness.md: GetProductByIdHandler example rewritten to use inline EF projection (no repository.GetByIdAsync) - docs/dotnet-10-features.md (line 138): "AsNoTracking + projection on every read" example now points to Features/GetProductById.cs (was ProductRepository.cs) - docs/code-flows/shippingservice.md (line 171): "See also" link text updated to reflect DbContext-direct shape, not "VSA variant on same interface" - docs/project-decisions.md: 2 references to "CatalogService.Api/Program.cs" → "CatalogService/Program.cs" (catalog VSA collapse renamed the path) - OrderService/Features/PaymentFailedHandler.cs: XML doc expanded to mirror PaymentCompletedHandler — describes the three-layer idempotency pattern and RowVersion concurrency safety. CodeRabbit flagged this as a Trivial doc parity item; doing it makes the saga-handler trio uniformly documented. Verification: 0 build warnings, 0 errors. 112 unit tests pass. The new IDOR integration test will be verified in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLAUDE.md (1)
1-303: 🧹 Nitpick | 🔵 TrivialRun /check-rules locally to audit paraphrases against this diff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 1 - 303, The PR comment asks you to run the repository audit command to ensure CLAUDE.md paraphrases and rule encodings match the diff; run the local auditor by executing the /check-rules hook (or the repository's equivalent script) and fix any reported mismatches by updating CLAUDE.md, the .claude/ path_instructions, or related agent/skill files so the new or changed rules from CLAUDE.md are encoded; verify that entries mentioned by the audit (e.g., the new Debugging Discipline additions, handler-registration guidance, and outbox transactional pattern) are present in .claude/* and docs/STATUS.md and re-run /check-rules until it reports no paraphrase errors.docs/ef-core.md (1)
457-464:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace stale repository-based examples and links in this doc section.
These changed snippets still reference
ProductRepository/repository.*andUpdateProductHandler.cs, which conflicts with the same document’s DbContext-direct guidance after the VSA collapse. Please update these examples to current handler files (CatalogService/Features/*) and DbContext-inline query/write shapes.As per coding guidelines: CatalogService handlers must use
CatalogDbContextdirectly with inline projection for reads and tracked aggregate mutation for writes.Also applies to: 721-728, 1066-1080
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ef-core.md` around lines 457 - 464, The doc still shows repository-based examples (ProductRepository.cs / GetAllAsync and references to UpdateProductHandler.cs and repository.*); update those sections (including the other occurrences at the noted ranges) to reference the current handler files under CatalogService/Features/* and change the code examples to use CatalogDbContext directly: for reads use CatalogDbContext queries with inline projection (e.g., context.Products.AsNoTracking().Select(...) pagination) and for writes show tracked aggregate mutation via CatalogDbContext (load tracked entity, modify properties, SaveChangesAsync) instead of repository methods; ensure all dead links and filenames now point to the Features/* handlers and remove any ProductRepository or repository.* mentions.
♻️ Duplicate comments (16)
CLAUDE.md (3)
65-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced block.
This fence is unlabeled and triggers markdownlint MD040.
Suggested fix
-``` +```text ServiceName/ Features/ # One file per use case: command/query record + validator + handler co-located. @@ Program.cs # Composition root. ServiceName.csproj # Single Web SDK project.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@CLAUDE.mdaround lines 65 - 75, The fenced code block in CLAUDE.md is
unlabeled causing markdownlint MD040; update the opening fence to include a
language identifier (e.g., changetotext) so the directory listing block
is explicitly marked as text—modify the fenced block containing the ServiceName/
tree in CLAUDE.md to use ```text as the opening fence.</details> --- `101-103`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Remove leftover “cross-service pattern diff is intentional” text.** This conflicts with the new “VSA for every service” rule above. Canonical rules should keep one architecture stance. As per coding guidelines, CLAUDE.md is canonical and must stay internally consistent. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 101 - 103, Remove the leftover paragraph that asserts "Don't apply both patterns uniformly across a single service..." so the doc no longer contradicts the new "VSA for every service" rule; locate the exact sentence block (the bolded line starting "Don't apply both patterns uniformly across a single service" and the following two lines about the diff being intentional) and delete it, then run a quick pass to ensure CLAUDE.md's canonical guidance references only the single "VSA for every service" stance and update any adjacent wording for seamless flow. ``` </details> --- `207-209`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Unify test-tier rule at the opening bullet.** “Unit tests for domain logic and handlers” conflicts with the integration-default guidance for EF/IO-touching handlers in the same section. As per coding guidelines, handler correctness that touches DbContext belongs to integration tests by default. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 207 - 209, Update the opening bullet that currently reads "Unit tests for domain logic and handlers" to explicitly separate pure-unit tests from handlers that touch IO/DbContext: state that domain logic and pure handlers are unit-tested, but any handler that accesses DbContext, external IO, or EF tokens defaults to integration tests (with Testcontainers/WebApplicationFactory). Adjust the sentence to include keywords "DbContext", "IO", "EF", and "integration by default" so examples like CatalogService and OrderService remain consistent with this rule. ``` </details> </blockquote></details> <details> <summary>docs/performance-and-data-correctness.md (1)</summary><blockquote> `129-129`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix stale handler file links in cache sections.** These references still point to `*Handler.cs` files that do not exist in the VSA layout (`GetProductById.cs`, `UpdateProduct.cs`, `ReserveStock.cs`). As per coding guidelines, CatalogService uses feature files under `Features/<UseCase>.cs` in the collapsed VSA structure. Also applies to: 274-274 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/performance-and-data-correctness.md` at line 129, Update the broken handler file links in docs/performance-and-data-correctness.md so they reference the VSA feature filenames instead of legacy Handler filenames: replace GetProductByIdHandler -> GetProductById.cs, UpdateProductHandler -> UpdateProduct.cs, and ReserveStockHandler -> ReserveStock.cs and ensure the paths point to Features/<UseCase>.cs (e.g., ../CatalogService/Features/GetProductById.cs). Apply the same replacements for the second occurrence noted (around the other mention at line 274). ``` </details> </blockquote></details> <details> <summary>CatalogService/Features/SearchProducts.cs (1)</summary><blockquote> `19-19`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Add query validation and reject blank search terms before SQL pattern construction.** `request.Query` is used directly to build the wildcard pattern. Blank/whitespace input can still hit the search path and devolve into a broad scan pattern. Add a `SearchProductsQueryValidator` and a defensive in-handler guard. <details> <summary>Proposed fix</summary> ```diff +using FluentValidation; using CatalogService.Infrastructure.Data; using Microsoft.EntityFrameworkCore; using NextAurora.Contracts.DTOs; @@ public record SearchProductsQuery(string Query, int Page = 1, int PageSize = 50); +public class SearchProductsQueryValidator : AbstractValidator<SearchProductsQuery> +{ + public SearchProductsQueryValidator() + { + RuleFor(x => x.Query) + .NotEmpty() + .Must(q => !string.IsNullOrWhiteSpace(q)) + .MaximumLength(500); + RuleFor(x => x.Page).GreaterThanOrEqualTo(1); + RuleFor(x => x.PageSize).InclusiveBetween(1, 100); + } +} + public class SearchProductsHandler(CatalogDbContext context) { public async Task<IReadOnlyList<ProductDto>> HandleAsync(SearchProductsQuery request, CancellationToken cancellationToken) { + if (string.IsNullOrWhiteSpace(request.Query)) + return []; + var safePage = request.Page < 1 ? 1 : request.Page; var safePageSize = request.PageSize is < 1 or > 100 ? 50 : request.PageSize; @@ - var pattern = $"%{request.Query}%"; + var pattern = $"%{request.Query.Trim()}%"; ``` </details> As per coding guidelines, feature slices should include validator + handler and enforce pagination/input constraints at the handler boundary. Also applies to: 32-35 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/SearchProducts.cs` at line 19, Add a validator and defensive guard to prevent blank/whitespace queries and enforce pagination limits: create a SearchProductsQueryValidator that ensures Query is not null/empty/whitespace and that Page >= 1 and PageSize is within allowed bounds (e.g. 1..100), register it with your validation pipeline, and update the SearchProducts handler (the code using SearchProductsQuery) to defensively check string.IsNullOrWhiteSpace(request.Query) and throw/return a validation error if true before constructing the SQL wildcard pattern; also clamp or validate Page and PageSize in the handler to avoid unbounded scans. ``` </details> </blockquote></details> <details> <summary>.coderabbit.yaml (1)</summary><blockquote> `45-49`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix dangling cross-reference to a non-existent path instruction.** This still points to `"**/Infrastructure/*Repository.cs" path_instructions below`, but that path block is not present, so the rule is incomplete for reviewers. <details> <summary>Suggested minimal fix</summary> ```diff - 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 for why and what to flag. ``` </details> As per coding guidelines, rule encodings should remain aligned and actionable across reviewer surfaces. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yaml around lines 45 - 49, The comment references a non-existent path_instructions block for "**/Infrastructure/*Repository.cs"; update the rule text to either remove the dangling reference or add the corresponding path_instructions block for "**/Infrastructure/*Repository.cs" so the guidance is actionable; specifically edit the string referencing "**/Infrastructure/*Repository.cs" (paired with the existing "**/Domain/I*Repository.cs") and either create the missing Infrastructure path_instructions block describing what to flag for Repository wrappers (e.g., justification criteria) or delete the clause that points to the missing block. ``` </details> </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `14-16`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Resolve architecture contradiction in the intro bullets.** This section still says CatalogService remains a Clean Architecture carve-out, but later in the same README the repo is documented as VSA across all five services. Keep one canonical statement. As per coding guidelines, CatalogService is now VSA with DbContext-direct handlers and no repository-wrapper carve-out. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 14 - 16, The intro bullets contradict each other about CatalogService's architecture; update the README text to state a single canonical architecture: CatalogService now uses Vertical Slice Architecture (VSA) like the other services with handlers taking DbContext directly (remove references to "Clean Architecture", "4-project split", and "IFooRepository" carve-out), and revise or remove the sentence that claims CatalogService is a deliberate Clean Architecture exception so the README consistently documents VSA across all five services; keep links to STATUS.md/CLAUDE.md but ensure any mention in those links reflects the same VSA decision for CatalogService. ``` </details> </blockquote></details> <details> <summary>docs/architecture.md (1)</summary><blockquote> `67-77`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Specify a language for the fenced code block.** The block starting at Line 67 is still missing a fence language (e.g., `text`), which triggers markdownlint MD040. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/architecture.mdaround lines 67 - 77, The fenced code block containing
the project structure (the block that begins with ServiceName/ and lists
Features/, Domain/, Infrastructure/, Endpoints/, Grpc/, Program.cs,
ServiceName.csproj) is missing a language hint which triggers markdownlint
MD040; fix it by adding a language specifier (for example use ```text)
immediately after the opening backticks so the block becomes a fenced code block
with a language tag.</details> </blockquote></details> <details> <summary>tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs (1)</summary><blockquote> `13-98`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Add required AAA narrative markers to each test method.** These tests still miss the mandated `// ARRANGE`, `// ACT`, `// ASSERT` phase markers with short narrative comments (e.g., why setup matters and what each assertion proves). As per coding guidelines: “Every test must have `// ARRANGE`, `// ACT`, `// ASSERT` markers (all caps)... Each phase carries a story comment...”. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs` around lines 13 - 98, Each test method (e.g., Validate_WithValidCommand_ReturnsNoErrors, Validate_WithEmptyProductId_ReturnsError, etc.) must include the three AAA markers: add a comment "// ARRANGE — set up a valid/invalid command using ValidCommand() to establish preconditions" before the command construction, "// ACT — execute the validator via _sut.Validate(command) to perform the behavior under test" immediately before the Validate call, and "// ASSERT — verify result.IsValid and specific result.Errors to confirm expected validation outcome" before the assertions; update every test in this file to include those three all-caps phase markers with the short narrative phrases so each test clearly documents arrange/act/assert steps and intent. ``` </details> </blockquote></details> <details> <summary>CatalogService/Features/ReserveStock.cs (1)</summary><blockquote> `13-17`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Preserve the handler contract on concurrency conflicts (`false`, not exception).** At Line 15 the contract says the loser returns `false`, but Line 43 currently lets `DbUpdateConcurrencyException` escape and fail the call path. <details> <summary>Proposed fix</summary> ```diff product.AdjustStock(product.StockQuantity - request.Quantity); - await context.SaveChangesAsync(cancellationToken); - - // Invalidate AFTER the save. See UpdateProduct for the cache-ordering rationale. - await cache.InvalidateAsync(request.ProductId, cancellationToken); + try + { + await context.SaveChangesAsync(cancellationToken); + // Invalidate AFTER the save. See UpdateProduct for the cache-ordering rationale. + await cache.InvalidateAsync(request.ProductId, cancellationToken); + } + catch (DbUpdateConcurrencyException) + { + return false; + } return true; ``` </details> ```shell #!/bin/bash # Verify contract text and missing concurrency catch in ReserveStock handler. rg -n "loser gets .*DbUpdateConcurrencyException.*returns false|SaveChangesAsync|catch \\(DbUpdateConcurrencyException\\)" CatalogService/Features/ReserveStock.cs -n -C2 ``` Also applies to: 43-47 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/ReserveStock.cs` around lines 13 - 17, The ReserveStock handler's contract promises to return false on a DbUpdateConcurrencyException but currently lets DbUpdateConcurrencyException escape; update the handler method (ReserveStock handler in ReserveStock.cs, the code that calls SaveChangesAsync) to catch DbUpdateConcurrencyException and return false instead of rethrowing, ensuring any surrounding try/catch around SaveChangesAsync or the method that performs the reservation handles this specific exception and maps it to a boolean false while leaving other exceptions unchanged. ``` </details> </blockquote></details> <details> <summary>tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs (1)</summary><blockquote> `12-75`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Apply the required AAA test narrative structure across this class.** The test methods still omit required `// ARRANGE`, `// ACT`, `// ASSERT` markers and narrative comments. As per coding guidelines: “Every test must have `// ARRANGE`, `// ACT`, `// ASSERT` markers (all caps)... Each phase carries a story comment...”. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs` around lines 12 - 75, The tests in ReserveStockCommandValidatorTests (methods Validate_WithValidCommand_ReturnsNoErrors, Validate_WithEmptyProductId_ReturnsError, Validate_WithZeroQuantity_ReturnsError, Validate_WithNegativeQuantity_ReturnsError, Validate_WithQuantityAtUpperBound_ReturnsNoErrors, Validate_WithQuantityOverUpperBound_ReturnsError) lack the required AAA comments; update each test to explicitly mark the three phases with comment lines "// ARRANGE", "// ACT", "// ASSERT" and add a short story comment for each phase (e.g., "// ARRANGE: create a valid ReserveStockCommand via ValidCommand()", "// ACT: call _sut.Validate(command)", "// ASSERT: verify result.IsValid and result.Errors as expected") so the Arrange uses ValidCommand(), Act invokes _sut.Validate(command), and Assert checks result.IsValid/ result.Errors per existing assertions. ``` </details> </blockquote></details> <details> <summary>CatalogService/Features/UpdateProduct.cs (1)</summary><blockquote> `50-52`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove the unnecessary `Include` from the update query.** `Category` is not used in this handler, so eager-loading it adds avoidable query/tracking overhead on a write path. <details> <summary>Suggested diff</summary> ```diff - var product = await context.Products - .Include(p => p.Category) - .FirstOrDefaultAsync(p => p.Id == request.ProductId, cancellationToken); + var product = await context.Products + .FirstOrDefaultAsync(p => p.Id == request.ProductId, cancellationToken); ``` </details> As per coding guidelines: write handlers should load only what they need on tracked aggregates and persist via `SaveChangesAsync`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CatalogService/Features/UpdateProduct.cs` around lines 50 - 52, In the UpdateProduct handler, remove the unnecessary eager-load of Category from the query that fetches the tracked product (the call that currently uses .Include(p => p.Category).FirstOrDefaultAsync(p => p.Id == request.ProductId, ...)); change the query to load only the Product entity so it avoids extra join/tracking overhead, keep the fetched entity tracked and persist changes with the existing SaveChangesAsync call on the DbContext. ``` </details> </blockquote></details> <details> <summary>tests/CatalogService.Tests.Integration/ProductCachingTests.cs (2)</summary><blockquote> `162-163`: _🛠️ Refactor suggestion_ | _🟠 Major_ | _⚡ Quick win_ **Rename async test methods to include `Async` suffix.** Both newly added async tests should follow the repo convention. <details> <summary>Suggested rename</summary> ```diff -public async Task PostProduct_byOwner_returns201AndPersistsProduct() +public async Task PostProduct_byOwner_returns201AndPersistsProductAsync() -public async Task ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCache() +public async Task ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCacheAsync() ``` </details> As per coding guidelines: “Async methods MUST be suffixed Async and called HandleAsync (NOT Handle).” Also applies to: 215-216 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Integration/ProductCachingTests.cs` around lines 162 - 163, The test method PostProduct_byOwner_returns201AndPersistsProduct is async and must be renamed to include the Async suffix (e.g., PostProduct_byOwner_returns201AndPersistsProductAsync) to follow repo convention; do the same for the other newly added async test referenced at lines 215-216, and update any references/calls/attributes to those method names (e.g., test runner or references in the class) so the signatures and names stay consistent with the Async naming rule. ``` </details> --- `215-250`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Align the test name with what the test actually verifies.** `ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCache` currently verifies persisted stock/availability only. Either add a cache-observable assertion or rename to avoid claiming invalidation coverage. As per coding guidelines: test assertions should clearly explain and verify ordering/cache invariants they claim to protect. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Integration/ProductCachingTests.cs` around lines 215 - 250, The test ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCache claims to verify cache invalidation but only asserts DB state; either update the test to actually assert cache behavior or rename it to reflect DB-only checks. To fix: if asserting cache, use the IMessageBus + cache retrieval path (e.g., call the cache-backed Product read method or IMemoryCache/ICacheProvider used by the app after invoking new ReserveStockCommand(productId, Quantity: 3)) and assert the cache entry was updated/removed; otherwise rename the test to ReserveStock_viaMessageBus_decrementsStockAndPersistsStock (or similar) and update test method name and any comments referencing invalidation to avoid claiming cache coverage. Ensure references to ReserveStockCommand and ReserveStockHandler remain intact so the tested flow is unchanged. ``` </details> </blockquote></details> <details> <summary>docs/project-decisions.md (1)</summary><blockquote> `269-269`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Align the link label text with the updated target path.** The target points to `../CatalogService/Program.cs`, but the visible label still says `CatalogService.Api/Program.cs`. Please make the label match the destination. [CatalogService/Program.cs](../CatalogService/Program.cs) Also applies to: 535-535 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/project-decisions.md` at line 269, Update the markdown link label to match its target path: replace the visible text "CatalogService.Api/Program.cs" with "CatalogService/Program.cs" for the link that points to "../CatalogService/Program.cs" (and make the same replacement for the second occurrence referenced around line 535); locate the link in docs/project-decisions.md and change only the label portion of the markdown link so the displayed text matches the destination path. ``` </details> </blockquote></details> <details> <summary>tests/CatalogService.Tests.Integration/ProductReadProjectionTests.cs (1)</summary><blockquote> `136-157`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Strengthen oversized page-size assertions to actually verify clamping.** `oversizedPageSize.Should().NotBeNull()` does not validate the cap behavior. Assert the capped count (and seed enough matching rows in the search test) so this fails if clamping regresses. As per coding guidelines: list endpoints must enforce pagination caps, and tests should verify those invariants explicitly. Also applies to: 175-186 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CatalogService.Tests.Integration/ProductReadProjectionTests.cs` around lines 136 - 157, Replace the weak assertion on oversizedPageSize with an explicit check that the result was clamped to the configured maximum: after calling handler.HandleAsync(new GetAllProductsQuery(Page: 1, PageSize: 5000), ...) assert oversizedPageSize.Count == DefaultPageSize (or the expected cap, e.g., 50) rather than just NotBeNull, and ensure the test seed in ProductReadProjectionTests provides at least that many matching rows so the check is meaningful; make the identical change for the parallel assertion block referenced around lines 175-186 (same handler.HandleAsync/GetAllProductsQuery usage). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@docs/code-flows/catalogservice.md:
- Around line 152-154: The diagram text is out of sync: it shows
Products.Include(Category)/JOIN but the actual handler uses a simple
FirstOrDefaultAsync by product id without Include; update the flow in
docs/code-flows/catalogservice.md so the request line shows H->>Ctx:
Products.FirstOrDefaultAsync(p => p.Id == id) (or similar) and change the DB
interaction line to a simple SELECT * FROM products WHERE id =@id(tracked)
instead of a JOIN/Include, keeping any remarks about tracking/xmin as
appropriate.In
@docs/cqrs-data-access.md:
- Around line 97-101: The docs are contradictory: this section states handlers
should take DbContext directly (no IRepository / IReadStore wrapper) but
earlier text and examples still describe repository-method-based CQRS flow;
update the earlier wording and examples to match the canonical shape by removing
or replacing references to repository interfaces and repository-method examples
with the DbContext-first pattern, revise any sample handler signatures to accept
DbContext (not IRepository/IReadStore), and clarify that read/write splits are
enforced by handler code-shape rather than separate repository interfaces.In
@docs/performance-and-data-correctness.md:
- Line 613: Update the stale project name "CatalogService.Api" to
"CatalogService" in the migration-tooling note so it matches the current project
layout; also mention the project is a single CatalogService.csproj at the
service root and ensure the example reference to the design-time factory
(CatalogDbContextFactory.cs) corresponds to that project name change.In
@tests/CatalogService.Tests.Integration/ProductReadProjectionTests.cs:
- Line 35: Rename the async test methods so their names end with the Async
suffix; specifically rename
GetProductById_projects_entity_into_ProductDto_with_category_name to
GetProductById_projects_entity_into_ProductDto_with_category_nameAsync and apply
the same pattern to the other public async Task test methods mentioned, update
any references/usages (e.g., test runner discovery or any inline calls) to the
new names, and ensure the method signatures (public async Task ...) and test
attributes remain unchanged.
Outside diff comments:
In@CLAUDE.md:
- Around line 1-303: The PR comment asks you to run the repository audit command
to ensure CLAUDE.md paraphrases and rule encodings match the diff; run the local
auditor by executing the /check-rules hook (or the repository's equivalent
script) and fix any reported mismatches by updating CLAUDE.md, the .claude/
path_instructions, or related agent/skill files so the new or changed rules from
CLAUDE.md are encoded; verify that entries mentioned by the audit (e.g., the new
Debugging Discipline additions, handler-registration guidance, and outbox
transactional pattern) are present in .claude/* and docs/STATUS.md and re-run
/check-rules until it reports no paraphrase errors.In
@docs/ef-core.md:
- Around line 457-464: The doc still shows repository-based examples
(ProductRepository.cs / GetAllAsync and references to UpdateProductHandler.cs
and repository.); update those sections (including the other occurrences at the
noted ranges) to reference the current handler files under
CatalogService/Features/ and change the code examples to use CatalogDbContext
directly: for reads use CatalogDbContext queries with inline projection (e.g.,
context.Products.AsNoTracking().Select(...) pagination) and for writes show
tracked aggregate mutation via CatalogDbContext (load tracked entity, modify
properties, SaveChangesAsync) instead of repository methods; ensure all dead
links and filenames now point to the Features/* handlers and remove any
ProductRepository or repository.* mentions.
Duplicate comments:
In @.coderabbit.yaml:
- Around line 45-49: The comment references a non-existent path_instructions
block for "/Infrastructure/*Repository.cs"; update the rule text to either
remove the dangling reference or add the corresponding path_instructions block
for "/Infrastructure/Repository.cs" so the guidance is actionable;
specifically edit the string referencing "/Infrastructure/*Repository.cs"
(paired with the existing "/Domain/IRepository.cs") and either create the
missing Infrastructure path_instructions block describing what to flag for
Repository wrappers (e.g., justification criteria) or delete the clause that
points to the missing block.In
@CatalogService/Features/ReserveStock.cs:
- Around line 13-17: The ReserveStock handler's contract promises to return
false on a DbUpdateConcurrencyException but currently lets
DbUpdateConcurrencyException escape; update the handler method (ReserveStock
handler in ReserveStock.cs, the code that calls SaveChangesAsync) to catch
DbUpdateConcurrencyException and return false instead of rethrowing, ensuring
any surrounding try/catch around SaveChangesAsync or the method that performs
the reservation handles this specific exception and maps it to a boolean false
while leaving other exceptions unchanged.In
@CatalogService/Features/SearchProducts.cs:
- Line 19: Add a validator and defensive guard to prevent blank/whitespace
queries and enforce pagination limits: create a SearchProductsQueryValidator
that ensures Query is not null/empty/whitespace and that Page >= 1 and PageSize
is within allowed bounds (e.g. 1..100), register it with your validation
pipeline, and update the SearchProducts handler (the code using
SearchProductsQuery) to defensively check
string.IsNullOrWhiteSpace(request.Query) and throw/return a validation error if
true before constructing the SQL wildcard pattern; also clamp or validate Page
and PageSize in the handler to avoid unbounded scans.In
@CatalogService/Features/UpdateProduct.cs:
- Around line 50-52: In the UpdateProduct handler, remove the unnecessary
eager-load of Category from the query that fetches the tracked product (the call
that currently uses .Include(p => p.Category).FirstOrDefaultAsync(p => p.Id ==
request.ProductId, ...)); change the query to load only the Product entity so it
avoids extra join/tracking overhead, keep the fetched entity tracked and persist
changes with the existing SaveChangesAsync call on the DbContext.In
@CLAUDE.md:
- Around line 65-75: The fenced code block in CLAUDE.md is unlabeled causing
markdownlint MD040; update the opening fence to include a language identifier
(e.g., changetotext) so the directory listing block is explicitly
marked as text—modify the fenced block containing the ServiceName/ tree in
CLAUDE.md to use ```text as the opening fence.- Around line 101-103: Remove the leftover paragraph that asserts "Don't apply
both patterns uniformly across a single service..." so the doc no longer
contradicts the new "VSA for every service" rule; locate the exact sentence
block (the bolded line starting "Don't apply both patterns uniformly across a
single service" and the following two lines about the diff being intentional)
and delete it, then run a quick pass to ensure CLAUDE.md's canonical guidance
references only the single "VSA for every service" stance and update any
adjacent wording for seamless flow.- Around line 207-209: Update the opening bullet that currently reads "Unit
tests for domain logic and handlers" to explicitly separate pure-unit tests from
handlers that touch IO/DbContext: state that domain logic and pure handlers are
unit-tested, but any handler that accesses DbContext, external IO, or EF tokens
defaults to integration tests (with Testcontainers/WebApplicationFactory).
Adjust the sentence to include keywords "DbContext", "IO", "EF", and
"integration by default" so examples like CatalogService and OrderService remain
consistent with this rule.In
@docs/architecture.md:
- Around line 67-77: The fenced code block containing the project structure (the
block that begins with ServiceName/ and lists Features/, Domain/,
Infrastructure/, Endpoints/, Grpc/, Program.cs, ServiceName.csproj) is missing a
language hint which triggers markdownlint MD040; fix it by adding a language
specifier (for example use ```text) immediately after the opening backticks so
the block becomes a fenced code block with a language tag.In
@docs/performance-and-data-correctness.md:
- Line 129: Update the broken handler file links in
docs/performance-and-data-correctness.md so they reference the VSA feature
filenames instead of legacy Handler filenames: replace GetProductByIdHandler ->
GetProductById.cs, UpdateProductHandler -> UpdateProduct.cs, and
ReserveStockHandler -> ReserveStock.cs and ensure the paths point to
Features/.cs (e.g., ../CatalogService/Features/GetProductById.cs).
Apply the same replacements for the second occurrence noted (around the other
mention at line 274).In
@docs/project-decisions.md:
- Line 269: Update the markdown link label to match its target path: replace the
visible text "CatalogService.Api/Program.cs" with "CatalogService/Program.cs"
for the link that points to "../CatalogService/Program.cs" (and make the same
replacement for the second occurrence referenced around line 535); locate the
link in docs/project-decisions.md and change only the label portion of the
markdown link so the displayed text matches the destination path.In
@README.md:
- Around line 14-16: The intro bullets contradict each other about
CatalogService's architecture; update the README text to state a single
canonical architecture: CatalogService now uses Vertical Slice Architecture
(VSA) like the other services with handlers taking DbContext directly (remove
references to "Clean Architecture", "4-project split", and "IFooRepository"
carve-out), and revise or remove the sentence that claims CatalogService is a
deliberate Clean Architecture exception so the README consistently documents VSA
across all five services; keep links to STATUS.md/CLAUDE.md but ensure any
mention in those links reflects the same VSA decision for CatalogService.In
@tests/CatalogService.Tests.Integration/ProductCachingTests.cs:
- Around line 162-163: The test method
PostProduct_byOwner_returns201AndPersistsProduct is async and must be renamed to
include the Async suffix (e.g.,
PostProduct_byOwner_returns201AndPersistsProductAsync) to follow repo
convention; do the same for the other newly added async test referenced at lines
215-216, and update any references/calls/attributes to those method names (e.g.,
test runner or references in the class) so the signatures and names stay
consistent with the Async naming rule.- Around line 215-250: The test
ReserveStock_viaMessageBus_decrementsStockAndInvalidatesCache claims to verify
cache invalidation but only asserts DB state; either update the test to actually
assert cache behavior or rename it to reflect DB-only checks. To fix: if
asserting cache, use the IMessageBus + cache retrieval path (e.g., call the
cache-backed Product read method or IMemoryCache/ICacheProvider used by the app
after invoking new ReserveStockCommand(productId, Quantity: 3)) and assert the
cache entry was updated/removed; otherwise rename the test to
ReserveStock_viaMessageBus_decrementsStockAndPersistsStock (or similar) and
update test method name and any comments referencing invalidation to avoid
claiming cache coverage. Ensure references to ReserveStockCommand and
ReserveStockHandler remain intact so the tested flow is unchanged.In
@tests/CatalogService.Tests.Integration/ProductReadProjectionTests.cs:
- Around line 136-157: Replace the weak assertion on oversizedPageSize with an
explicit check that the result was clamped to the configured maximum: after
calling handler.HandleAsync(new GetAllProductsQuery(Page: 1, PageSize: 5000),
...) assert oversizedPageSize.Count == DefaultPageSize (or the expected cap,
e.g., 50) rather than just NotBeNull, and ensure the test seed in
ProductReadProjectionTests provides at least that many matching rows so the
check is meaningful; make the identical change for the parallel assertion block
referenced around lines 175-186 (same handler.HandleAsync/GetAllProductsQuery
usage).In
@tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs:
- Around line 12-75: The tests in ReserveStockCommandValidatorTests (methods
Validate_WithValidCommand_ReturnsNoErrors,
Validate_WithEmptyProductId_ReturnsError,
Validate_WithZeroQuantity_ReturnsError,
Validate_WithNegativeQuantity_ReturnsError,
Validate_WithQuantityAtUpperBound_ReturnsNoErrors,
Validate_WithQuantityOverUpperBound_ReturnsError) lack the required AAA
comments; update each test to explicitly mark the three phases with comment
lines "// ARRANGE", "// ACT", "// ASSERT" and add a short story comment for each
phase (e.g., "// ARRANGE: create a valid ReserveStockCommand via
ValidCommand()", "// ACT: call _sut.Validate(command)", "// ASSERT: verify
result.IsValid and result.Errors as expected") so the Arrange uses
ValidCommand(), Act invokes _sut.Validate(command), and Assert checks
result.IsValid/ result.Errors per existing assertions.In
@tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs:
- Around line 13-98: Each test method (e.g.,
Validate_WithValidCommand_ReturnsNoErrors,
Validate_WithEmptyProductId_ReturnsError, etc.) must include the three AAA
markers: add a comment "// ARRANGE — set up a valid/invalid command using
ValidCommand() to establish preconditions" before the command construction, "//
ACT — execute the validator via _sut.Validate(command) to perform the behavior
under test" immediately before the Validate call, and "// ASSERT — verify
result.IsValid and specific result.Errors to confirm expected validation
outcome" before the assertions; update every test in this file to include those
three all-caps phase markers with the short narrative phrases so each test
clearly documents arrange/act/assert steps and intent.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: ASSERTIVE **Plan**: Pro Plus **Run ID**: `42e73a0f-d37f-4510-bf1f-7703e495c251` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e23d15be7da4789e9d390161436d93cd7921a0b2 and d1ac3d3e2056b7da8953c0004292deb9a707113b. </details> <details> <summary>📒 Files selected for processing (84)</summary> * `.claude/agents/architecture-reviewer.md` * `.coderabbit.yaml` * `.gitignore` * `CLAUDE.md` * `CatalogService/CatalogService.Application/CatalogService.Application.csproj` * `CatalogService/CatalogService.Application/Commands/CreateProductCommand.cs` * `CatalogService/CatalogService.Application/Commands/ReserveStockCommand.cs` * `CatalogService/CatalogService.Application/Commands/UpdateProductCommand.cs` * `CatalogService/CatalogService.Application/Handlers/CreateProductHandler.cs` * `CatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.cs` * `CatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.cs` * `CatalogService/CatalogService.Application/Handlers/ReserveStockHandler.cs` * `CatalogService/CatalogService.Application/Handlers/SearchProductsHandler.cs` * `CatalogService/CatalogService.Application/Handlers/UpdateProductHandler.cs` * `CatalogService/CatalogService.Application/Interfaces/IProductReadStore.cs` * `CatalogService/CatalogService.Application/Queries/GetAllProductsQuery.cs` * `CatalogService/CatalogService.Application/Queries/GetProductByIdQuery.cs` * `CatalogService/CatalogService.Application/Queries/SearchProductsQuery.cs` * `CatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.cs` * `CatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.cs` * `CatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.cs` * `CatalogService/CatalogService.Domain/CatalogService.Domain.csproj` * `CatalogService/CatalogService.Domain/Interfaces/IProductRepository.cs` * `CatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csproj` * `CatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.cs` * `CatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.cs` * `CatalogService/CatalogService.csproj` * `CatalogService/Domain/Category.cs` * `CatalogService/Domain/IProductCache.cs` * `CatalogService/Domain/Product.cs` * `CatalogService/Endpoints/CatalogEndpoints.cs` * `CatalogService/Features/CreateProduct.cs` * `CatalogService/Features/GetAllProducts.cs` * `CatalogService/Features/GetProductById.cs` * `CatalogService/Features/ReserveStock.cs` * `CatalogService/Features/SearchProducts.cs` * `CatalogService/Features/UpdateProduct.cs` * `CatalogService/Grpc/CatalogGrpcService.cs` * `CatalogService/Infrastructure/Caching/HybridProductCache.cs` * `CatalogService/Infrastructure/Data/CatalogDbContext.cs` * `CatalogService/Infrastructure/Data/CatalogDbContextFactory.cs` * `CatalogService/Infrastructure/DependencyInjection.cs` * `CatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.Designer.cs` * `CatalogService/Infrastructure/Migrations/20260503040949_InitialCreate.cs` * `CatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.Designer.cs` * `CatalogService/Infrastructure/Migrations/20260518133000_SeedDemoCatalog.cs` * `CatalogService/Infrastructure/Migrations/CatalogDbContextModelSnapshot.cs` * `CatalogService/Program.cs` * `CatalogService/Properties/launchSettings.json` * `CatalogService/Protos/catalog.proto` * `CatalogService/appsettings.Development.json` * `CatalogService/appsettings.json` * `NextAurora.AppHost/AppHost.cs` * `NextAurora.AppHost/NextAurora.AppHost.csproj` * `NextAurora.slnx` * `OrderService/Infrastructure/DependencyInjection.cs` * `OrderService/Infrastructure/GrpcCatalogClient.cs` * `OrderService/OrderService.csproj` * `README.md` * `docs/STATUS.md` * `docs/architecture.md` * `docs/code-flows/catalogservice.md` * `docs/cqrs-data-access.md` * `docs/dotnet-10-features.md` * `docs/ef-core.md` * `docs/how-it-works.md` * `docs/performance-and-data-correctness.md` * `docs/project-decisions.md` * `tests/CatalogService.Tests.Integration/CatalogService.Tests.Integration.csproj` * `tests/CatalogService.Tests.Integration/ProductAuthorizationTests.cs` * `tests/CatalogService.Tests.Integration/ProductCachingTests.cs` * `tests/CatalogService.Tests.Integration/ProductReadProjectionTests.cs` * `tests/CatalogService.Tests.Integration/ProductReadStoreTests.cs` * `tests/CatalogService.Tests.Unit/Application/CreateProductCommandValidatorTests.cs` * `tests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cs` * `tests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cs` * `tests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cs` * `tests/CatalogService.Tests.Unit/Application/ReserveStockCommandValidatorTests.cs` * `tests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cs` * `tests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cs` * `tests/CatalogService.Tests.Unit/Application/UpdateProductCommandValidatorTests.cs` * `tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs` * `tests/CatalogService.Tests.Unit/Builders/ProductBuilder.cs` * `tests/CatalogService.Tests.Unit/CatalogService.Tests.Unit.csproj` </details> <details> <summary>💤 Files with no reviewable changes (29)</summary> * CatalogService/CatalogService.Application/CatalogService.Application.csproj * CatalogService/CatalogService.Infrastructure/CatalogService.Infrastructure.csproj * CatalogService/CatalogService.Application/Validators/ReserveStockCommandValidator.cs * CatalogService/CatalogService.Application/Queries/SearchProductsQuery.cs * CatalogService/CatalogService.Application/Validators/UpdateProductCommandValidator.cs * tests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cs * CatalogService/CatalogService.Application/Handlers/UpdateProductHandler.cs * tests/CatalogService.Tests.Unit/Application/CreateProductHandlerTests.cs * CatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.cs * CatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.cs * CatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.cs * CatalogService/CatalogService.Application/Validators/CreateProductCommandValidator.cs * CatalogService/CatalogService.Application/Commands/ReserveStockCommand.cs * CatalogService/CatalogService.Application/Interfaces/IProductReadStore.cs * CatalogService/CatalogService.Application/Queries/GetProductByIdQuery.cs * CatalogService/CatalogService.Domain/Interfaces/IProductRepository.cs * CatalogService/CatalogService.Domain/CatalogService.Domain.csproj * CatalogService/CatalogService.Application/Handlers/CreateProductHandler.cs * CatalogService/CatalogService.Application/Queries/GetAllProductsQuery.cs * tests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cs * CatalogService/CatalogService.Application/Commands/CreateProductCommand.cs * tests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cs * CatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.cs * CatalogService/CatalogService.Application/Commands/UpdateProductCommand.cs * tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs * CatalogService/CatalogService.Application/Handlers/ReserveStockHandler.cs * tests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cs * tests/CatalogService.Tests.Integration/ProductReadStoreTests.cs * CatalogService/CatalogService.Application/Handlers/SearchProductsHandler.cs </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
emeraldleaf
dismissed
coderabbitai[bot]’s stale review
May 26, 2026 05:22
All 17 findings from review 4359737042 addressed in commit 0c9f3ab:
- GetOrderById IDOR: BuyerId predicate in EF Where + endpoint extracts JWT;
added integration test for non-owner → null → 404 anti-enumeration - SearchProducts empty-term guard
- ReserveStock DbUpdateConcurrencyException catch
- UpdateProduct unused .Include removed
- 13 doc/config cleanups (coderabbit.yaml dangling ref, STATUS date,
2 fenced-block MD040 lint, 4 ef-core.md stale repo examples,
perf-correctness.md + dotnet-10-features.md + project-decisions.md- shippingservice.md walkthroughs, PaymentFailedHandler XML doc parity)
Verified: 0 build warnings, 0 errors, 112 unit tests pass.
CodeRabbit re-review didn't fire (likely auto-paused after multiple
pushes); dismissing the stale Request-changes verdict so merge can proceed.
emeraldleaf
added a commit
that referenced
this pull request
May 26, 2026
Three doc/diagram updates that the PR #31 code changes made stale: 1. docs/code-flows/catalogservice.md — Flow 2 (PUT /products/{id}) Mermaid showed `Products.Include(Category).FirstOrDefaultAsync(...)` with a SELECT * FROM products JOIN categories. After PR #31 removed the unused .Include(p => p.Category), the actual query is `Products.FirstOrDefaultAsync(...)` with no JOIN. Updated the diagram line + added a Note explaining why the Include was removed. 2. docs/code-flows/catalogservice.md — Flow 3 (gRPC ReserveStock) The xmin-stale branch said "DbUpdateConcurrencyException bubbles up as gRPC Internal status — OrderService sees the call fail and aborts the order." That described the pre-fix behavior. After PR #31's try/catch around SaveChangesAsync, the handler now returns false and the gRPC response is `ReserveStockResponse { Success = false }` — the same shape as insufficient stock. OrderService aborts cleanly without surfacing a 500. Updated the Note + added the explicit return-path lines in the diagram. 3. docs/code-flows/orderservice.md — file inventory Updated the OrderEndpoints.cs and GetOrderById.cs entries to describe the new IDOR contract: endpoint extracts JWT NameIdentifier and passes it as RequestingBuyerId; handler's EF Where clause filters by both Id AND BuyerId (non-owner → null straight from the database → 404 per anti-enumeration). 4. CLAUDE.md "Authorization" rule (Security Requirements) The previous wording — "Handler loads the entity, then returns null on owner mismatch" — described the post-materialization in-memory check, which the current implementations don't use. Both ShippingService .GetShipmentByOrder and (after PR #31) OrderService.GetOrderById push the ownership predicate into the EF Where clause for tighter defense-in-depth. The rule now distinguishes: - Read handlers: predicate in SQL (preferred) - Write handlers: tracked load + in-memory check (required because they need the tracked entity to mutate) Reference templates list expanded with explicit file pairings + which pattern each demonstrates. 5. .claude/agents/architecture-reviewer.md — IDOR heuristic update Synced with the CLAUDE.md rule rewrite. A read handler whose ownership check is in C# rather than SQL is now flagged as Should-consider (structurally weaker), not Must-fix (still satisfies the external null → 404 contract). Reference templates updated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
emeraldleaf
added a commit
that referenced
this pull request
May 27, 2026
PR #31 collapsed CatalogService from Clean Architecture to VSA, but the README still referenced CatalogService as a "deliberate Clean Architecture carve-out" and described the design as "mixed per-service architecture." Both claims contradicted the (correct) Project Structure section that said VSA across all five services. Changes: - Collapsed the four-bullet "About this repo" preamble into two bullets: Monorepo + single architectural shape (was three contradictory bullets plus a long MySQL-translation aside), and the two-database-engines bullet retained. - Updated the How It Works doc description: "Clean Architecture" -> "VSA layout" (matches docs/how-it-works.md which was already updated in the collapse). - Dropped the seven `Source: file.excalidraw` subtitle lines under the hero diagram + each of the six reference diagrams. The SVG is already embedded and clickable to full-size; the editable .excalidraw sources are now noted once in the section intro. Net: -20 lines, -1 contradiction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
May 27, 2026
… follow-up
Three new rules + one rule tightening + one open-issue acknowledgement,
all sourced from two Milan Jovanović / Kerim Kara articles on scaling
patterns. PR scope continues to be "encode the rules before the next
instance hits review."
Rules added:
1. NON-SARGABLE PREDICATES DEFEAT INDEXES — fix at write time.
A Where(...) that wraps a column in a function (u.Email.ToLower() == x,
o.CreatedAt.Date == today, EF.Functions.ILike with leading wildcard)
defeats any B-tree index on that column. Fix at write time: normalize
on insert/update + Where against the normalized column, or use
case-insensitive column collation. Leading-wildcard substring search
isn't B-tree-indexable in any database — escalate to Postgres tsvector
full-text or a dedicated search engine when load justifies it.
CatalogService/Features/SearchProducts.cs already documents the
leading-wildcard trade-off explicitly; updated its cross-reference to
point at the new rule by name.
2. FAN-OUT BELONGS ON THE MESSAGE BUS, NOT IN A SYNCHRONOUS HANDLER LOOP.
A handler that iterates a recipient list inline and awaits a sender
per recipient holds the request open for N × per-recipient-latency
and concentrates work on one process. Right shape: publish one
Wolverine message per recipient (or per batch of K), return immediately,
let per-recipient handlers run in parallel under
MaxDegreeOfParallelism throttle. Preventative — not retroactively
violated today (NotificationService receives one event = one outbound
notification).
3. PARALLELIZE INDEPENDENT AWAITS WITH TASK.WHENALL.
Sequential await of independent I/O calls serializes latency for free.
When a handler makes N independent I/O calls (N gRPC to different
services, N HTTP to different external APIs, N queries against
DIFFERENT DbContexts), Task.WhenAll pays the max latency, not the sum.
Reference: OrderService/Features/PlaceOrder.cs:93 (gRPC fan-out over
per-line GetProductAsync with the DbContext-safety caveat documented
inline). Explicit caveats encoded: don't WhenAll dependent operations,
operations sharing the same DbContext scope (not thread-safe), or
operations whose per-call failure observability matters (WhenAll
surfaces only the first exception).
Rule tightening (from prior architecture-reviewer feedback):
4. 202 ACCEPTED RULE — two definitional clarifications.
(a) "Tracking row" can be the aggregate itself when its ID is the
polling key (POST /api/v1/orders pattern), not necessarily a
separate jobs table.
(b) "Commit atomically" branches by dispatch path: bus.InvokeAsync
gets AutoApplyTransactions for free; inline persist+publish needs
the explicit BeginTransactionAsync → SaveChangesAsync → CommitAsync
wrap from the existing Outbox-outside-handler rule. Cross-reference
added.
STATUS.md follow-up:
5. POST /api/v1/payments/process noted as a partial Should-consider
under the new long-running-work rule. Endpoint returns 202 but the
handler awaits Stripe synchronously; typical sub-second but tail
latency on degraded states is seconds-to-30s. Deferred (current
shape works for the demo; rule is encoded preventatively).
Note on the second Kerim Kara article ("Every Senior .NET Developer Has
Defended This Architecture"): most of its lessons are already encoded in
NextAurora — the I*Repository deletion (PR #30), the CatalogService
Clean→VSA collapse (PR #31), the "Interfaces earn their keep through
consumer substitution" rule, the integration-tests-over-mocked-repository
discipline, the DbContext-direct data-access shape. The article reads
as a victory lap for the simplicity refactor. The one net-new takeaway
was the sequential-awaits trap, encoded here as rule #3.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 tasks
emeraldleaf
added a commit
that referenced
this pull request
May 27, 2026
…#39) * chore(claude): encode Factory Pattern / keyed-services rule across config Adds the "factory pattern earns its keep at 2+ impls" rule to the three canonical encoding sites + a worked sketch at the reference site: - CLAUDE.md: new bullet right after the consumer-substitution rule. Codifies that .NET keyed services (AddKeyedScoped + [FromKeyedServices]) is the canonical multi-impl factory; don't hand-roll IPortFactory; don't pre-build the factory while there's only one impl. NotificationService cited as the canonical "ready for the factory, not yet wearing it" case. - .claude/agents/architecture-reviewer.md: new section keyed to Infrastructure/DependencyInjection.cs files. Two flagged patterns: premature factory (Must-fix) and missing factory at 2+ impls (Should-consider). Explicit guidance that IServiceProvider's keyed- services API IS the canonical factory. - .coderabbit.yaml: matching path_instructions block under **/Infrastructure/DependencyInjection.cs, encoding the same two patterns so CodeRabbit catches future violations at PR-review time. - NotificationService/Infrastructure/DependencyInjection.cs: composition- root XML doc now sketches the future keyed-services rewrite that the current single-AddScoped will become when SendGrid/Twilio actually ship — so the rule lands with a concrete reference shape in code, not just in config. Why now: PR #30 + #31 collapsed the I*Repository wrappers and tightened the consumer-substitution rule. The factory-pattern shape is the natural next codification — same lesson (layers without capability are speculative coupling), one tier up in abstraction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(readme): drop stale Clean Architecture refs + trim verbose preamble PR #31 collapsed CatalogService from Clean Architecture to VSA, but the README still referenced CatalogService as a "deliberate Clean Architecture carve-out" and described the design as "mixed per-service architecture." Both claims contradicted the (correct) Project Structure section that said VSA across all five services. Changes: - Collapsed the four-bullet "About this repo" preamble into two bullets: Monorepo + single architectural shape (was three contradictory bullets plus a long MySQL-translation aside), and the two-database-engines bullet retained. - Updated the How It Works doc description: "Clean Architecture" -> "VSA layout" (matches docs/how-it-works.md which was already updated in the collapse). - Dropped the seven `Source: file.excalidraw` subtitle lines under the hero diagram + each of the six reference diagrams. The SVG is already embedded and clickable to full-size; the editable .excalidraw sources are now noted once in the section intro. Net: -20 lines, -1 contradiction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(claude): apply review fixes + encode async-API / 202 Accepted rule Two threads in one commit: REVIEW FIXES (3) from the architecture-reviewer pass on the prior commits: 1. Reconciled categorization drift on "MISSING FACTORY at 2+ impls" between architecture-reviewer.md (was "Must-fix on absence when per-call selection is required") and .coderabbit.yaml (was "Should-consider", no qualifier). Both now agree: Must-fix when per-call routing is intended (latent bug — plain AddScoped registrations collide silently, DI returns last-registered impl every time, dropping routing intent); Should-consider when impls are interchangeable (no live bug, but tighten for explicitness). 2. Split architecture-reviewer.md's dual-category bullet into two cleaner bullets — one for the per-call-selection Must-fix case, one for the interchangeable Should-consider case. 3. Narrowed the Infrastructure DI section heading: the glob now stands alone (`**/Infrastructure/DependencyInjection.cs`), and the broader port-adapter scope is lifted into the body so the glob and the prose claim match. NEW RULE — long-running work / 202 Accepted shape. Encoded across CLAUDE.md "Performance Rules" + .coderabbit.yaml Endpoints path + architecture-reviewer Endpoints section. Rule: if a write handler can take more than ~1 second, reshape as 202 Accepted (validate + persist intent + publish Wolverine message + return job ID with Location header). Background handler does the work; client polls or gets pushed. NextAurora already has all the machinery (Wolverine + Service Bus + outbox + saga) — the rule is "use it when a handler would otherwise block on minutes-scale work." Reference shape: POST /api/v1/orders (place → publish OrderPlaced → return OrderId; downstream PaymentService + ShippingService handle saga via async events). Same rule applies to Wolverine handlers themselves — if the handler body runs for minutes, factor the work into a follow-up message handler. Why now: the rule isn't actively violated in the codebase today (no minutes-scale synchronous endpoints exist), but the encoding is what guarantees future endpoints don't introduce one. Same logic as the factory pattern rule landed in the prior commit — encode the shape before the violation, not after. Verified no existing "See CLAUDE.md" markers paraphrase a contradictory version of the new rule (the two nearest markers reference money calculations and consumer substitution, both unaffected). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(claude): encode 3 more rules + tighten 202 Accepted + STATUS.md follow-up Three new rules + one rule tightening + one open-issue acknowledgement, all sourced from two Milan Jovanović / Kerim Kara articles on scaling patterns. PR scope continues to be "encode the rules before the next instance hits review." Rules added: 1. NON-SARGABLE PREDICATES DEFEAT INDEXES — fix at write time. A Where(...) that wraps a column in a function (u.Email.ToLower() == x, o.CreatedAt.Date == today, EF.Functions.ILike with leading wildcard) defeats any B-tree index on that column. Fix at write time: normalize on insert/update + Where against the normalized column, or use case-insensitive column collation. Leading-wildcard substring search isn't B-tree-indexable in any database — escalate to Postgres tsvector full-text or a dedicated search engine when load justifies it. CatalogService/Features/SearchProducts.cs already documents the leading-wildcard trade-off explicitly; updated its cross-reference to point at the new rule by name. 2. FAN-OUT BELONGS ON THE MESSAGE BUS, NOT IN A SYNCHRONOUS HANDLER LOOP. A handler that iterates a recipient list inline and awaits a sender per recipient holds the request open for N × per-recipient-latency and concentrates work on one process. Right shape: publish one Wolverine message per recipient (or per batch of K), return immediately, let per-recipient handlers run in parallel under MaxDegreeOfParallelism throttle. Preventative — not retroactively violated today (NotificationService receives one event = one outbound notification). 3. PARALLELIZE INDEPENDENT AWAITS WITH TASK.WHENALL. Sequential await of independent I/O calls serializes latency for free. When a handler makes N independent I/O calls (N gRPC to different services, N HTTP to different external APIs, N queries against DIFFERENT DbContexts), Task.WhenAll pays the max latency, not the sum. Reference: OrderService/Features/PlaceOrder.cs:93 (gRPC fan-out over per-line GetProductAsync with the DbContext-safety caveat documented inline). Explicit caveats encoded: don't WhenAll dependent operations, operations sharing the same DbContext scope (not thread-safe), or operations whose per-call failure observability matters (WhenAll surfaces only the first exception). Rule tightening (from prior architecture-reviewer feedback): 4. 202 ACCEPTED RULE — two definitional clarifications. (a) "Tracking row" can be the aggregate itself when its ID is the polling key (POST /api/v1/orders pattern), not necessarily a separate jobs table. (b) "Commit atomically" branches by dispatch path: bus.InvokeAsync gets AutoApplyTransactions for free; inline persist+publish needs the explicit BeginTransactionAsync → SaveChangesAsync → CommitAsync wrap from the existing Outbox-outside-handler rule. Cross-reference added. STATUS.md follow-up: 5. POST /api/v1/payments/process noted as a partial Should-consider under the new long-running-work rule. Endpoint returns 202 but the handler awaits Stripe synchronously; typical sub-second but tail latency on degraded states is seconds-to-30s. Deferred (current shape works for the demo; rule is encoded preventatively). Note on the second Kerim Kara article ("Every Senior .NET Developer Has Defended This Architecture"): most of its lessons are already encoded in NextAurora — the I*Repository deletion (PR #30), the CatalogService Clean→VSA collapse (PR #31), the "Interfaces earn their keep through consumer substitution" rule, the integration-tests-over-mocked-repository discipline, the DbContext-direct data-access shape. The article reads as a victory lap for the simplicity refactor. The one net-new takeaway was the sequential-awaits trap, encoded here as rule #3. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(claude): encode Server-side pricing rule + tighten paraphrase Surfaced by /check-rules audit: `OrderService/Features/PlaceOrder.cs:138` had a `See CLAUDE.md.` marker with no specific section named, paraphrasing a principle ("never trust client-submitted prices for money calculations") that wasn't a named rule in CLAUDE.md. Option 2 (per the audit prompt): add the named rule, tighten the paraphrase to point at it. Changes: - CLAUDE.md "Security Requirements" gets a new named bullet: "Server-controlled fields are computed server-side, never trusted from the client" — covers money fields (Price/Currency/Tax), authorization identifiers (BuyerId/SellerId), state-machine columns (Status), and security flags (IsAdmin/IsDeleted). Specifies the failure mode (price tampering — client submits Price=0.01 for a $999 product) and the canonical pattern (fetch authoritative value from its source; treat the request DTO as untrusted input). References PlaceOrder.cs as the reference example. Cross-links to the existing "Mass assignment" check in the architecture-reviewer. - OrderService/Features/PlaceOrder.cs:138 cross-reference tightened from generic `See CLAUDE.md.` to the named rule. No other files paraphrase this principle today (verified via the prior /check-rules audit run that found this single drift). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
Jun 4, 2026
) * docs+infra: refresh stale refs from VSA-collapse + add broken-link CI guard After the /check-rules audit + the doc-currency sweep flagged multiple stale references inherited from PR #31 (CatalogService Clean → VSA collapse) and from the IPaymentRepository deletion. Most of these had been sitting in the repo for months because nothing fails when a markdown link rots. Drift fixed: - CLAUDE.md:341 — outbox-atomic example linked to PaymentRepository.cs (deleted in simplicity refactor). Repointed at PaymentRecoveryJob, the current inline implementation site. CLAUDE.md:27 already correctly described the move; only the deep-link was stale. - docs/demo-deployment.md + docs/demo-deployment-story.md — multiple references to CatalogService.Api/, CatalogService.Infrastructure/, the pre-collapse 4-project Clean layout. All paths refreshed to the current single-project structure (CatalogService/Program.cs, CatalogService/ Infrastructure/Data/CatalogDbContext.cs, etc.). Two `dotnet ef migrations add` snippets that referenced the old --project / --startup-project pairing simplified to `--project CatalogService`. - docs/performance-and-data-correctness.md:129,326 — handler citations used the *Handler.cs file naming convention (Clean) but VSA co-locates command + validator + handler in a single use-case file. Renamed to GetProductById.cs / UpdateProduct.cs / ReserveStock.cs. The dead GetProductByIdHandlerTests.cs citation replaced with the existing ProductCachingTests.cs (integration tier — the right tier for cache-projection behavior). - Dockerfile.catalog — rewrote the build stage for the single-project structure. The old Dockerfile COPYed CatalogService/CatalogService.Api/ *.csproj and 3 sibling projects that haven't existed since PR #31. Anyone triggering the Fly.io deploy workflow today would have hit a build failure on the very first COPY. Verified locally with `docker build --platform=linux/amd64 -f Dockerfile.catalog -t catalog-api .` → produces a 116 MB runtime image. Mechanical guard added: - .github/workflows/ci.yml — broken-link audit step in the build job. Scans every markdown file for relative links to .cs/.csproj/.props/.sh/.yml/ .yaml/.svg/.excalidraw/.cls/.md files, fails the build if any don't resolve. Skips http(s)://, // bare URLs, and anchors-only. Process substitution throughout so the failure flag survives the loop (a piped while runs in a subshell and loses its updates — this is the trap that bit the first draft). Same shape as the existing "Concurrency audit" step: grep + fail. Would have caught every drift in this PR mechanically. Smoke-tested locally against the post-fix tree (exit 0). Why option B (full fix) over option A (doc-only): the deployed Fly demo at https://catalog-api-demo.fly.dev/ is still running pre-collapse code (last successful deploy was SHA 73388e8, before #31 merged). Any redeploy attempt would have failed silently — the broken Dockerfile + a workflow nobody had triggered in 2 weeks meant the failure mode would surface as a confusing CI error on the next demo refresh, not when the actual drift landed. Knocking out the Dockerfile + the docs that describe how to use it in the same PR keeps the system internally consistent. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: exclude .claude/audits/INDEX.md from broken-link audit INDEX.md ships in the repo but its links point at per-article audit files that are gitignored for copyright reasons (verbatim quoted prose). See .claude/commands/article-audit.md step 5 'Copyright note' — contract is 'INDEX ships, per-article files don't.' On a contributor's machine the links resolve; on the CI runner they don't, by design. Surfaced by #112's first run — guard correctly flagged 14 broken refs, but they're all from this one intentionally-excluded file. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
Jun 5, 2026
…n-CLAUDE.md) (#114) * chore: prevention pass — hook + CLAUDE.md rule + CodeRabbit instruction (parts 2-4 of file-move discipline) Three of four layers of the file-move drift prevention loop. Part #1 (extend the CI broken-link guard to scan Dockerfile*) lands in a follow-up commit on this same PR once #112 is merged and rebased. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: extend prevention to Dockerfile COPY + diagrams + topology docs Three more layers on top of cf27628 (parts 2-4 of the file-move prevention loop). Same compounding-loop principle, broader coverage: (a) Broken-COPY audit — Dockerfile* COPY/ADD source paths that don't exist in the build context. Catches the kind of drift that left Dockerfile.catalog referencing the pre-VSA-collapse 4-project layout for months after PR #31. Skips `--from=<stage>` cross-stage copies and wildcards (resolved at build time, not against the repo tree). (b) Diagram-pair audit — every docs/*.excalidraw must have its sibling docs/*.svg and vice versa. Reviewers look at the .svg on github.com to understand the system; .excalidraw is the editable source. If one exists without the other, the diagram review surface is broken. Does NOT verify the .svg matches what would be regenerated from the .excalidraw source (that needs Playwright in CI — separate, heavier gate). The pair-existence check is the cheap mechanical floor. (c) CLAUDE.md "Doc-and-diagram discipline" rule. Sibling to the file-move rule. Encodes that docs and diagrams are the REVIEW SURFACE, not byproducts — when reviewers look at the system, they read docs/architecture.md and look at docs/nextaurora-architecture.svg. If those are stale, every review reasons against a fiction. Names concrete pairings: AppHost.cs ↔ architecture.md/svg, Extensions.cs middleware order ↔ service-request-flow.svg, EF/cache/outbox changes ↔ perf-doc + their sibling diagrams. (d) CodeRabbit path_instructions for NextAurora.AppHost/AppHost.cs and NextAurora.ServiceDefaults/Extensions.cs — when topology or middleware-order code changes, flag missing paired doc/diagram updates at review time. PR-description waivers acceptable when the deferred update is named in a tracking issue. All three new mechanical guards (broken-link from #112, broken-COPY, diagram-pair) smoke-tested locally on the current tree → exit 0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: trim CLAUDE.md (5 surfaces, lean discipline, perf + observability) Third commit on this branch — the lean-CLAUDE.md pass on top of the file-move (cf27628) + doc-and-diagram (4e1ba94) prevention layers. CLAUDE.md drops from 358 → 301 lines (~16% smaller) by moving deep-dive content out and leaving headlines + links. Every rule preserved; nothing the AI needs to know got dropped. Continuous Rule Encoding — 6 surfaces → 5: - GitHub Issues moved out of the encoding loop (it's a deferral mechanism, not encoding — closed issues aren't re-read in future sessions) - Mirror update in docs/dev-loop.md (table + prose) - Diagram redesigned with 5 surfaces in a clean row, simpler tier boxes (no crammed tool lists), CLAUDE.md annotated "(kept lean)" to reinforce the discipline visually New "one-paragraph max per rule" discipline on CLAUDE.md surface 1: - "If a rule needs more than ~6 lines, the rule stays as a bolded headline + one-paragraph summary in CLAUDE.md; detail moves to docs/ or skills/" - Test: "could this rule + its rationale fit on one screen?" CI size guard (build job): - CLAUDE.md soft-warning at 400 lines, hard-fail at 500 - Mechanical floor on bloat regression Performance Rules trim (~30 dense bullets → 22 headlines + links): - Rules themselves unchanged (project-not-map, Task.WhenAll, outbox-atomic, Guid v7, AsSpan, Dapper escape hatch, etc.) - Deep-dive paragraphs → docs/performance-and-data-correctness.md + dotnet-performance skill (both already exist) Observability section trim (~78 lines → 21): - Kept 3 always-on traps: HTTP middleware order, Wolverine middleware instance-methods, outbox-outside-handler atomicity - Moved "how it works" detail → docs/architecture.md "Cross-Cutting Concerns" and "Event-Driven Architecture" (both already cover this) Verification: - wc -l CLAUDE.md: 301 (well under the 400/500 size budget) - broken-link audit on CLAUDE.md: exit 0 - diagram-pair audit: exit 0 - broken-COPY audit: exit 0 - All rule headlines preserved; rules with paraphrases in code comments ("See CLAUDE.md") still align with canonical wording Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: actually move CLAUDE.md trim content to destination docs The prior commit (5c938b2) trimmed CLAUDE.md from 358→301 lines and added "See docs/..." links. An audit found the content I trimmed wasn't actually in those destination docs — I'd deleted it, not moved it. This commit does the real moves. New file: docs/observability-and-context-propagation.md - Section-aligned with CLAUDE.md "Observability & Context Propagation" - Full mechanism + headers/baggage mapping table + sources - HTTP middleware order with the canonical 4-line code example - Wolverine pipeline scope detail (pipeline order: validation → context → handler → AutoApplyTransactions) - Wolverine envelope context extraction mechanism - Transactional Outbox config + outbox-outside-handler atomicity trap with the canonical safe wrapper code block - Structured logging scope hygiene - Event Replay note Additions to docs/performance-and-data-correctness.md: - New "## Additional always-on patterns" section after "The 14 always-on rules" — patterns that don't fit a single-rule shape - Non-sargable predicates + EmailNormalized normalize-at-write-time pattern - Task.WhenAll parallel awaits with three caveats (dependent ops, shared DbContext, multi-failure observability) - Long-running work / 202 Accepted pattern with two atomicity paths and cloud-managed alternatives (Durable Functions, Step Functions, Temporal) - Fan-out on message bus + MaxDegreeOfParallelism throttle - Guid.CreateVersion7 with the time-decodable trade-off - AsSpan ref-struct + async-boundary constraint CLAUDE.md link redirects (no rule text changes): - "Long-running work" → docs/performance-and-data-correctness.md "Long-running work belongs on the message bus" (new section) - "Observability & Context Propagation" mechanism → docs/observability-and-context-propagation.md (new file) - "Outbox outside a Wolverine handler" wrapper → docs/observability-and-context-propagation.md anchor (new file) Naming convention: section-aligned filenames where new docs are created (observability-and-context-propagation.md mirrors the CLAUDE.md heading). Existing docs kept where they already align by topic (performance-and-data-correctness.md). Verification: - wc -l CLAUDE.md: 301 (unchanged; under 400 soft / 500 hard budget) - broken-link audit: exit 0 (full repo) - All trimmed content now lives in either the new doc or the existing perf doc; nothing the AI needs to know got dropped Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
How it was built
AI-assisted
Summary by CodeRabbit
Refactor
Documentation
Tests