Chore/claude encode session lessons#28
Merged
Merged
Conversation
The 8-file architecture-reviewer agent pass on 2026-05-24 surfaced 12 must-fix items + 17 should-considers across security, EF Core, async, distributed systems, and observability. The fixes shipped as PRs; this commit captures the patterns so future code generation prevents the same bugs rather than re-discovering them in review. CLAUDE.md — 4 sections expanded: 1. "Security Requirements" — concrete IDOR-prevention pattern (was just "validate buyerId matches authenticated user"; now spells out the endpoint→query→handler→null→404 shape with reference templates + the 403-vs-404 reasoning). Adds explicit JWT defaults (ValidateIssuerSigningKey=true, ClockSkew=30s — not the dangerous 5-minute default). Adds the trace-ID-exposure rule (use Activity.TraceId.ToString, not Activity.Id which leaks the span). Also explicitly cross-references the integration-test requirement. 2. "Transactional Outbox (Wolverine)" — adds the outbox-outside-handler atomicity trap that caused the PaymentRecoveryJob silent event drop. Wolverine's AutoApplyTransactions only wraps handler chains; code running outside (BackgroundService, recovery sweepers) needs an explicit SaveChangesAsync between PublishAsync and Commit, or Wolverine's staged envelope never reaches the outbox table. Includes the canonical safe wrapper as runnable code. 3. "Observability — HTTP middleware order" — strict ordering rule (UseExceptionHandler → UseAuthentication → CorrelationIdMiddleware → UseAuthorization). The first ServiceDefaults bug was middleware reading context.User before UseAuthentication populated it (UserId silently always null); the over-correction placed it after UseAuthorization which dropped UserId from 401/403 audit logs. Documents both failure modes + the correct middle ground. 4. "Testing" — IDOR test is now REQUIRED for every scoped-entity endpoint. Authorization behavior is only proven by an authorization-failure test; dotnet build + unit tests passing isn't sufficient. Also adds outbox-in-non-handler test requirement for sweepers/jobs. .coderabbit.yaml — 3 new path_instructions blocks: - **/Endpoints/**/*.cs gets explicit IDOR-prevention guidance with the canonical 4-step pattern + mass-assignment check + endpoint hygiene reminders. - **/*RecoveryJob*.cs gets outbox-outside-handler trap guidance + per-iteration scope rule + distributed-lock rule. - NextAurora.ServiceDefaults/**/*.cs gets the middleware-order rule, JWT explicit-defaults rule, trace-ID-exposure rule, and a hint that changes here should bump unit-test coverage (because the project has historically been undertested and codecov/patch will complain). architecture-reviewer agent — new "Pattern checklist" section with specific per-file-category scan rules. Six categories covered: Endpoints (IDOR + mass-assignment), RecoveryJob (outbox + scope + lock), ServiceDefaults (middleware + JWT + trace-ID), query handlers (AsNoTracking + pagination + N+1), aggregates (rich domain entity + collection encapsulation + concurrency token), workflows (bash hygiene + persist-credentials + permissions). The agent now scans for these patterns explicitly instead of re-deriving from first principles on every review. Issues doc (/tmp/nextaurora-issues-to-create.md) — 8 new backlog entries from this session's findings, sized + scoped with acceptance criteria. Includes the IDOR-test backfill, ServiceDefaults integration tests, PaymentRecoveryJob outbox integration test, per-iteration DI scope refactor, CORS helper, security headers, PlaceOrder defense-in- depth, and HTTPS-redirect consolidation. Paraphrase sweep: zero stale paraphrases. The new rules are NEW additions, not modifications of existing wording, so none of the existing `See CLAUDE.md` markers in code or docs need updating. The in-flight fix PRs (IDOR, outbox, middleware) will add new markers in their respective inline comments when they merge — that's PR-scope, not lessons-learned-PR-scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rule
The existing "Debugging Discipline" rule mandates that lessons from
debugging sessions get encoded in CLAUDE.md + supporting docs before
moving on. This commit generalizes that same discipline to ALL review
surfaces — architecture-reviewer agent passes, CodeRabbit findings,
manual code review, integration-test failures, prod incidents, security
audits.
CLAUDE.md: new "Continuous Rule Encoding (the compounding loop)" section
adjacent to Debugging Discipline. It explicitly mandates that when an
anti-pattern / rule / spec is identified worth keeping, it gets written
to ALL applicable artifacts in the same session:
1. CLAUDE.md — the canonical rule
2. .coderabbit.yaml path_instructions — PR-time detection
3. architecture-reviewer agent Pattern Checklist — review-time detection
4. .claude/skills/ — if the procedure is non-trivial
5. docs/STATUS.md Open Issues — if deferred
6. Supporting docs (architecture.md, performance-and-data-correctness.md,
dev-loop.md) — when the *why* needs more than a one-liner
And it makes the threshold explicit: encode if a future author could
repeat the mistake OR re-derive the rule from first principles. Don't
encode trivial style nits; do encode security/perf/concurrency/
distributed-systems patterns. The closing line is the load-bearing
commitment: "A merged fix PR without the corresponding rule is a
half-finished job."
architecture-reviewer agent: added Step 7 to "How to work" — when the
agent identifies a pattern worth encoding, it must propose WHERE
(CLAUDE.md section / .coderabbit.yaml path / Pattern Checklist
category) and the proposed wording. Added a "Rules to encode" optional
section to the output format so the suggestions have a place to land
in the report rather than getting buried.
The point of these two changes together: the compounding loop becomes
mechanical rather than aspirational. Every review-surface finding now
has a clear path from "discovered" to "encoded so we don't discover
it again" — and the agent itself prompts for the encoding so we don't
forget.
Paraphrase sweep: clean. New section is purely additive; no existing
rule wording changed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…in dev-loop md/svg
Round-trips the "Continuous Rule Encoding" meta-rule (introduced in the
preceding commit) into the surfaces a human or AI assistant would actually
read when working in this repo:
.claude/README.md (new): the missing "how to use this directory" guide.
- Directory layout
- Decision tree for picking between agents / commands / skills
- Per-surface deep-dive: what each is, when to use, how to invoke,
how to author, current inventory in this repo
- Hooks + architecture-map sections (briefly; pointers to source)
- "When to add a new extension" table that mirrors the Continuous Rule
Encoding decision matrix from CLAUDE.md
docs/dev-loop.md: adds "Continuous Rule Encoding (the compounding feedback
loop)" section between the 5 stages and the Gaps list. Includes the
encoding-surface table (CLAUDE.md / .coderabbit.yaml / agent / skill /
status / docs) + a tiny ASCII diagram of the loop + an explicit pointer
to .claude/README.md for invocation details. Names the load-bearing rule:
"a merged fix PR without the corresponding .claude/ encoding is a
half-finished job."
docs/dev-loop.svg: two changes.
1. Iteration-loop arrow label updated from "findings → fix → push
→ re-review" to "findings → fix → encode rule in .claude/
→ push → re-review". The encoding step is now visible at a
glance, not buried in prose.
2. New "Continuous Rule Encoding" section at the bottom (180px tall,
purple/AI-LLM color matching the Stage 1 Edit-time block). Shows the
Finding → Encode-in-four-places → Next-cycle-prevented flow
inline. Cross-references .claude/README.md for invocation patterns.
3. SVG viewBox + width + height bumped from 1750 to 1940 to accommodate
the new section. Background rect resized to match.
Why all three: a human reading dev-loop.md gets the prose version. A human
viewing dev-loop.svg gets the visual. A human (or AI) opening the .claude/
directory gets the README explaining what each file in there is for.
Three surfaces, same message, picked up at whichever angle the reader
approaches from.
The architecture-reviewer agent already has Step 7 (suggest encodings)
from the previous commit. Together with this commit, the encoding loop is
now mechanical rather than aspirational at every surface.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… OrderService - Implement tests for GetAllProductsHandler to verify product retrieval and pagination. - Add tests for ReserveStockHandler to ensure stock reservation logic and cache invalidation. - Create tests for SearchProductsHandler to validate product search functionality. - Introduce UpdateProductHandler tests to check product updates and cache management. - Add tests for GetOrderByIdHandler to ensure correct order retrieval and mapping. - Implement GetOrdersByBuyerHandler tests to verify order retrieval by buyer. - Add PaymentFailedHandler tests to ensure correct handling of payment failure events. - Introduce OrderPlacedHandler tests to validate event translation into commands. - Add GetShipmentByOrderHandler tests to ensure shipment retrieval and ownership validation.
…com/emeraldleaf/NextAurora into chore/claude-encode-session-lessons
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
…ession-lessons # Conflicts: # .claude/agents/architecture-reviewer.md Co-authored-by: emeraldleaf <5404190+emeraldleaf@users.noreply.github.com>
Contributor
Resolved and pushed in |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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.Unit/Application/CreateProductHandlerTests.cs`:
- Around line 30-31: Replace the bare phase comments with the required AAA-style
single-line markers including an em-dash and short narrative: in
CreateProductHandlerTests update "// ACT" to something like "// ACT — invoke
_sut.HandleAsync(command, CancellationToken.None) to execute the command" and
update corresponding "// ASSERT" comments to "// ASSERT — verify result contains
expected product details / side-effects" (and apply the same pattern for the
other occurrences in this test file). Ensure all phase labels are all-caps
(ARRANGE, ACT, ASSERT) and include a concise story context on the same line.
In `@tests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cs`:
- Around line 31-32: Replace bare phase comments in the
GetAllProductsHandlerTests unit file with the required narrative AAA format:
change occurrences of "// ACT" and "// ASSERT" around calls like
_sut.HandleAsync(new GetAllProductsQuery(), CancellationToken.None) and
subsequent verification blocks to "// ACT — <what this phase does>" and "//
ASSERT — <what this verifies>" respectively; update the other two test locations
referenced (the blocks around lines with the second ACT/ASSERT and third
ACT/ASSERT) so each comment includes an em-dash and a brief explicit intent
describing the action or expectation for that phase.
In `@tests/CatalogService.Tests.Unit/Application/ReserveStockHandlerTests.cs`:
- Around line 33-37: Replace the bare phase comments in this test so they follow
the required "PHASE — narrative" AAA format: update the lines that currently
read "// ACT" and "// ASSERT" to include same-line em-dash explanations
describing intent and verification (e.g. "// ACT — invoke
ReserveStockHandler.HandleAsync to attempt reservation" and "// ASSERT — result
is true and invariants hold"), and apply the same change for the other phase
markers in this file (around usages of _sut.HandleAsync, command, and result at
the other noted ranges) so every test uses the explicit narrative comments.
In `@tests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cs`:
- Around line 33-34: Replace the loose phase comments in
SearchProductsHandlerTests with strict AAA-style markers: change bare "// ACT"
and "// ASSERT" comments to the all-caps, em-dash style with a short narrative
on the same line (e.g. "// ACT — invoke SearchProductsQuery via
SearchProductsHandler.HandleAsync(...)" and "// ASSERT — verify expected
results/behavior"); update every occurrence in this test (including the other
occurrences noted around the later assertions) so the SearchProductsHandlerTests
class follows the required "// ACT — ..." and "// ASSERT — ..." convention.
In `@tests/CatalogService.Tests.Unit/Application/UpdateProductHandlerTests.cs`:
- Around line 38-41: Replace the bare `// ACT` and `// ASSERT` comments with the
required AAA narrative format (e.g. `// ACT — Run the handler.` and `// ASSERT —
Verify expected invariants.`) in this test class (UpdateProductHandlerTests) so
each phase includes the em-dash and short rationale; specifically update the
comment above the `_sut.HandleAsync(command, CancellationToken.None);` call and
the subsequent assertion markers (and the other similar occurrences in the same
file) to follow the `// ARRANGE — <what/why>`, `// ACT — <what/why>`, `// ASSERT
— <what/why>` pattern.
- Around line 95-103: The test currently asserts an UnauthorizedAccessException
on owner mismatch; change it to follow the null→404 anti-enumeration contract:
call the handler via act and assert it does not throw, then assert the handler
returned null (instead of throwing) and that the stored entity remains unchanged
(product.Name == originalName, product.Price unchanged) and that neither
UpdateAsync nor InvalidateAsync were called; update any related endpoint tests
to map this null result to a 404 response. Use the existing symbols to locate
code: UpdateProductHandlerTests, act, product, originalName, UpdateAsync, and
InvalidateAsync.
In `@tests/OrderService.Tests.Unit/Application/ShipmentDispatchedHandlerTests.cs`:
- Around line 50-62: The test Handle_WhenOrderNotFound_ReturnsWithoutError
currently asserts no exception but doesn't verify no persistence occurred; after
invoking _sut.HandleAsync(EventFor(Guid.NewGuid()), CancellationToken.None) add
an assertion that _repository.UpdateAsync was not called (e.g. using
NSubstitute: _repository.DidNotReceive().UpdateAsync(Arg.Any<Order>(),
Arg.Any<CancellationToken>()) or _repository.Received(0).UpdateAsync(...)) to
enforce the no-op/idempotent path. Ensure you reference the same mocked
_repository and keep the existing GetByIdAsync setup and act invocation
unchanged.
In
`@tests/ShippingService.Tests.Unit/Application/PaymentCompletedHandlerTests.cs`:
- Around line 63-67: Update the test's AAA comments to match the file's em-dash
story style: change the existing "// ACT" and "// ASSERT" markers around the
call to PaymentCompletedHandler.Handle(`@event`) and the result assertion to
include the story text after an em-dash (e.g., "// ACT — when payment completed
is handled" and "// ASSERT — result should not be null"), so the // ACT and //
ASSERT comments follow the same Arrange→Act→Assert narrative convention used
elsewhere in the test file and reference the action
(PaymentCompletedHandler.Handle) and the verification
(result.Should().NotBeNull()).
- Around line 46-69: The test name promises that PaymentCompletedHandler.Handle
always returns a new command instance but currently only calls Handle once;
update the test Handle_AlwaysReturnsNewCommand to invoke
PaymentCompletedHandler.Handle twice with the same PaymentCompletedEvent,
capturing two results (e.g., result1 and result2), then assert both are not
null, have the same OrderId, and are different instances by reference (use a
"not same as" / reference inequality assertion) to verify a new object is
allocated each call.
In `@tests/ShippingService.Tests.Unit/Domain/ShipmentTests.cs`:
- Around line 15-16: Update each bare "// ACT" comment to follow the repo's AAA
pattern by adding an em-dash story explaining what the action does and what will
be verified; e.g., replace the ACT markers surrounding the call to
Shipment.Create(Guid.NewGuid(), Guid.NewGuid(), "FedEx") (and the similar ACT
markers near other Shipment.Create calls) with "// ACT — create a shipment
instance to verify creation succeeds and properties are initialized correctly"
(adjust the verb and verification text to match each test's intent), so the
comment is all-caps "ACT" followed by an em-dash and the narrative explanation.
🪄 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: 7b1cad17-7cf0-431e-825b-44bee80b6aa9
⛔ Files ignored due to path filters (1)
docs/dev-loop.svgis excluded by!**/*.svg
📒 Files selected for processing (33)
.claude/README.md.claude/agents/architecture-reviewer.md.coderabbit.yamlCLAUDE.mddocs/dev-loop.mdtests/CatalogService.Tests.Integration/ProductCachingTests.cstests/CatalogService.Tests.Unit/Application/ContextPropagationMiddlewareTests.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/NotificationService.Tests.Unit/Application/NotificationEventHandlersTests.cstests/NotificationService.Tests.Unit/Application/SendNotificationHandlerTests.cstests/OrderService.Tests.Integration/OrderSagaTests.cstests/OrderService.Tests.Unit/Application/ContextPropagationMiddlewareTests.cstests/OrderService.Tests.Unit/Application/CorrelationIdMiddlewareTests.cstests/OrderService.Tests.Unit/Application/GetOrderByIdHandlerTests.cstests/OrderService.Tests.Unit/Application/GetOrdersByBuyerHandlerTests.cstests/OrderService.Tests.Unit/Application/OutgoingContextMiddlewareTests.cstests/OrderService.Tests.Unit/Application/PaymentCompletedHandlerTests.cstests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cstests/OrderService.Tests.Unit/Application/ShipmentDispatchedHandlerTests.cstests/OrderService.Tests.Unit/Domain/OrderLineTests.cstests/OrderService.Tests.Unit/Domain/OrderTests.cstests/PaymentService.Tests.Unit/Application/ContextPropagationMiddlewareTests.cstests/PaymentService.Tests.Unit/Application/OrderPlacedHandlerTests.cstests/PaymentService.Tests.Unit/Application/PaymentRecoveryJobTests.cstests/ShippingService.Tests.Unit/Application/ContextPropagationMiddlewareTests.cstests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cstests/ShippingService.Tests.Unit/Application/PaymentCompletedHandlerTests.cstests/ShippingService.Tests.Unit/Domain/ShipmentTests.cs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Updated comments in various test files to provide clearer context for actions taken during tests. - Replaced null return value handling with ReturnsNull() for better readability in mocks. - Ensured that all test cases consistently describe the action being performed in the ACT section. - Enhanced assertions to clarify expected outcomes and improve test reliability.
…com/emeraldleaf/NextAurora into chore/claude-encode-session-lessons
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs (1)
61-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNumber and explain the two ASSERT invariants in the idempotency no-op test.
This ASSERT block checks multiple invariants (
NotThrowAsyncandDidNotReceive) but does not number/explain them, which makes the guarded failure mode less explicit.Suggested comment-only patch
- // ASSERT — No throw, no save. + // ASSERT — Two invariants: + // 1) No exception — late benign deliveries should be consumed, not DLQ'd. + // 2) No UpdateAsync call — proves true no-op behavior when the order is missing. await act.Should().NotThrowAsync(); await _repository.DidNotReceive().UpdateAsync(Arg.Any<Order>(), Arg.Any<CancellationToken>());As per coding guidelines: when ASSERT checks multiple invariants in idempotency scenarios, number them and explain why each matters.
🤖 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/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs` around lines 61 - 67, Update the ASSERT block to enumerate and explain the two invariants being checked: 1) that invoking _sut.HandleAsync(EventFor(Guid.NewGuid()), CancellationToken.None) does not throw (use act and await act.Should().NotThrowAsync()) — explain this verifies idempotent handling completes without exceptions, and 2) that _repository.DidNotReceive().UpdateAsync(Arg.Any<Order>(), Arg.Any<CancellationToken>()) — explain this verifies no state changes/saves occur for already-handled events; add short numbered comments immediately above each assertion referencing act and _repository.DidNotReceive to make the guarded failure mode explicit.
🤖 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/OrderService.Tests.Unit/Application/OutgoingContextMiddlewareTests.cs`:
- Around line 72-74: Update the ASSERT comment above the envelope.Headers checks
to enumerate and briefly explain the two invariants: (1) "X-User-Id" must not be
present on envelope.Headers to ensure no user-id header pollution when no user
context is provided, and (2) "X-Session-Id" must not be present to ensure no
session-id leakage; reference the exact assertion lines using envelope.Headers
and the header keys "X-User-Id" and "X-Session-Id" so reviewers can see which
invariant each explanation maps to.
---
Outside diff comments:
In `@tests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs`:
- Around line 61-67: Update the ASSERT block to enumerate and explain the two
invariants being checked: 1) that invoking
_sut.HandleAsync(EventFor(Guid.NewGuid()), CancellationToken.None) does not
throw (use act and await act.Should().NotThrowAsync()) — explain this verifies
idempotent handling completes without exceptions, and 2) that
_repository.DidNotReceive().UpdateAsync(Arg.Any<Order>(),
Arg.Any<CancellationToken>()) — explain this verifies no state changes/saves
occur for already-handled events; add short numbered comments immediately above
each assertion referencing act and _repository.DidNotReceive to make the guarded
failure mode explicit.
🪄 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: a8f83d44-b62c-4ecb-a10d-6edc20723c3d
📒 Files selected for processing (27)
tests/CatalogService.Tests.Unit/Application/ContextPropagationMiddlewareTests.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/NotificationService.Tests.Unit/Application/NotificationEventHandlersTests.cstests/NotificationService.Tests.Unit/Application/SendNotificationHandlerTests.cstests/OrderService.Tests.Integration/OrderSagaTests.cstests/OrderService.Tests.Unit/Application/ContextPropagationMiddlewareTests.cstests/OrderService.Tests.Unit/Application/CorrelationIdMiddlewareTests.cstests/OrderService.Tests.Unit/Application/GetOrderByIdHandlerTests.cstests/OrderService.Tests.Unit/Application/GetOrdersByBuyerHandlerTests.cstests/OrderService.Tests.Unit/Application/OutgoingContextMiddlewareTests.cstests/OrderService.Tests.Unit/Application/PaymentCompletedHandlerTests.cstests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cstests/OrderService.Tests.Unit/Application/ShipmentDispatchedHandlerTests.cstests/OrderService.Tests.Unit/Domain/OrderLineTests.cstests/OrderService.Tests.Unit/Domain/OrderTests.cstests/PaymentService.Tests.Unit/Application/ContextPropagationMiddlewareTests.cstests/PaymentService.Tests.Unit/Application/OrderPlacedHandlerTests.cstests/PaymentService.Tests.Unit/Application/PaymentRecoveryJobTests.cstests/ShippingService.Tests.Unit/Application/ContextPropagationMiddlewareTests.cstests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cstests/ShippingService.Tests.Unit/Application/PaymentCompletedHandlerTests.cstests/ShippingService.Tests.Unit/Domain/ShipmentTests.cs
- Introduced IProductReadStore interface for read operations, separating it from IProductRepository. - Created ProductReadStore class implementing IProductReadStore, projecting to ProductDto directly in EF. - Updated GetAllProductsHandler, GetProductByIdHandler, SearchProductsHandler, and related tests to use IProductReadStore. - Refactored order and shipment handlers to utilize read projections, ensuring no entity materialization occurs on read paths. - Enhanced tests to verify DTOs are returned directly from read store methods, maintaining separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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:
- Line 64: Replace the narrow glob pattern "**/Infrastructure/*Repository.cs" in
the .coderabbit.yaml with the broader pattern
"**/Infrastructure/**/*Repository*.cs" so nested repository files (e.g.,
Infrastructure/Repositories/ProductRepository.cs) are matched; update the
pattern string in the YAML entry to the new value and save the file.
In
`@CatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.cs`:
- Around line 64-81: The SearchAsync method uses case-sensitive Contains;
replace those predicates with PostgreSQL ILIKE via EF.Functions.ILike to make
search case-insensitive: update the Where clause on context.Products in
SearchAsync to use EF.Functions.ILike for p.Name and p.Description with
wildcarded query (e.g., %query%) and ensure Microsoft.EntityFrameworkCore is
imported so EF.Functions is available. Keep the rest of the projection and
paging logic unchanged.
In `@CLAUDE.md`:
- Around line 189-190: The CLAUDE.md note was changed and any paraphrase in the
repo that ends with "See CLAUDE.md" must be audited and updated to reflect the
new EF Core guidance: locate uses of the phrasing (e.g., places referencing
AsNoTracking(), AsNoTrackingWithIdentityResolution(), AsSplitQuery(),
repo.GetX(), Mapper.ToDto(entity), GetByIdAsync, IFooReadStore or
docs/cqrs-data-access.md) and ensure they now recommend projecting into DTOs
inside the IQueryable for reads, using
AsNoTracking()/AsNoTrackingWithIdentityResolution()/AsSplitQuery() where
appropriate, splitting read/write methods (or adding IFooReadStore) instead of
reusing entity-returning GetByIdAsync for queries, and that any "See CLAUDE.md"
link points to the updated section; update the paraphrase text to match the new
guidance and run /check-rules locally to confirm no paraphrase violations
remain.
In `@docs/architecture.md`:
- Around line 500-516: The sentence "Full read/write repository separation
(Interface Segregation) is a future consideration." in the "EF Core Read/Write
Method Split" section is now stale; remove or reword it so the text consistently
states that CQRS read/write separation is the established rule (per guidance) —
locate the paragraph that contains that exact phrase and either delete it or
replace it with a definitive statement like "Full read/write repository
separation (Interface Segregation) is enforced" to avoid conflicting guidance
with the rest of the section.
In `@docs/ef-core.md`:
- Line 567: The TOC entry still references the old "shared-method wrinkle"
anchor; update the table-of-contents link text/anchor to match the renamed
section header "Read/write method split — the hard rule" so the in-page
anchor/slug and link match (e.g., replace the old "shared-method wrinkle"
label/anchor with the new header slug or explicit anchor for "Read/write method
split — the hard rule"). Ensure the TOC link target exactly matches the
generated header id used by your markdown renderer.
In `@docs/performance-and-data-correctness.md`:
- Line 200: Update any internal links that still point to the old anchor
"`#decision-asnotracking-strategy`" so they match the new heading "Decision:
read/write method split (CQRS data access)"; search the markdown for the old
anchor and replace it with the new generated anchor for that heading (e.g.
"`#decision-readwrite-method-split-cqrs-data-access`" or whatever your markdown
renderer generates) so in-page navigation from references (previously pointing
to "`#decision-asnotracking-strategy`") correctly jumps to the "Decision:
read/write method split (CQRS data access)" section.
In `@docs/STATUS.md`:
- Line 5: Update the stale timestamp string "**Last updated:** 2026-05-24" in
docs/STATUS.md to today's date by replacing "2026-05-24" with "2026-05-25" so
the file reflects the current last-updated date.
In `@OrderService/Infrastructure/OrderRepository.cs`:
- Around line 53-59: GetSummariesByBuyerIdAsync currently uses caller-supplied
page and pageSize directly; enforce bounds by validating and normalizing the
inputs inside the method (e.g., ensure page >= 1 and pageSize between 1 and a
server-side cap of 100), clamp pageSize to the cap if it exceeds 100 or set a
sensible default if zero/negative, and recalculate Skip using the sanitized
values before applying Skip/Take on context.Orders so all queries honor the
repo-wide pagination cap and avoid negative/oversized paging.
🪄 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: 38b77871-d13e-4762-b2d7-152a8a008206
📒 Files selected for processing (36)
.claude/agents/architecture-reviewer.md.coderabbit.yaml.github/copilot-instructions.mdCLAUDE.mdCatalogService/CatalogService.Application/Handlers/GetAllProductsHandler.csCatalogService/CatalogService.Application/Handlers/GetProductByIdHandler.csCatalogService/CatalogService.Application/Handlers/SearchProductsHandler.csCatalogService/CatalogService.Application/Interfaces/IProductReadStore.csCatalogService/CatalogService.Application/Mappers/ProductMapper.csCatalogService/CatalogService.Domain/Interfaces/IProductRepository.csCatalogService/CatalogService.Infrastructure/Data/CatalogDbContext.csCatalogService/CatalogService.Infrastructure/DependencyInjection.csCatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.csCatalogService/CatalogService.Infrastructure/Repositories/ProductRepository.csNextAurora.Contracts/DTOs/ShipmentDto.csOrderService/Domain/IOrderRepository.csOrderService/Features/GetOrderById.csOrderService/Features/GetOrdersByBuyer.csOrderService/Features/OrderSummaryMapper.csOrderService/Infrastructure/OrderRepository.csPaymentService/Infrastructure/PaymentRepository.csShippingService/Domain/IShipmentRepository.csShippingService/Endpoints/ShippingEndpoints.csShippingService/Features/GetShipmentByOrder.csShippingService/Infrastructure/ShipmentRepository.csdocs/STATUS.mddocs/architecture.mddocs/cqrs-data-access.mddocs/ef-core.mddocs/performance-and-data-correctness.mdtests/CatalogService.Tests.Unit/Application/GetAllProductsHandlerTests.cstests/CatalogService.Tests.Unit/Application/GetProductByIdHandlerTests.cstests/CatalogService.Tests.Unit/Application/SearchProductsHandlerTests.cstests/OrderService.Tests.Unit/Application/GetOrderByIdHandlerTests.cstests/OrderService.Tests.Unit/Application/GetOrdersByBuyerHandlerTests.cstests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cs
💤 Files with no reviewable changes (2)
- CatalogService/CatalogService.Application/Mappers/ProductMapper.cs
- OrderService/Features/OrderSummaryMapper.cs
…ration tests for ProductReadStore and OrderRepository
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/OrderService.Tests.Integration/OrderReadProjectionTests.cs`:
- Around line 145-148: The test currently pre-filters dtos into buyerAOrders
before asserting count, allowing rows from other buyers to leak in; update the
assertions so they verify the returned dtos are strictly scoped to buyerA (not
just excluding buyerB). Specifically, replace the pre-filter+count check with an
assertion that every dto in dtos has BuyerId == buyerA (e.g., dtos.All(d =>
d.BuyerId == buyerA)) and still assert the total count is 3, keep the order-id
equality check using aNewest.Id, aMiddle.Id, aOldest.Id, and remove or tighten
the dtos.Should().NotContain(d => d.BuyerId == buyerB) check since the
strict-all-BuyerId assertion covers all non-requested buyers. Ensure you update
references to buyerAOrders accordingly in the OrderReadProjectionTests so all
assertions operate on dtos and not the pre-filtered collection.
🪄 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: 1ed79a59-1430-4a3b-a625-5f3616195531
📒 Files selected for processing (10)
.coderabbit.yamlCatalogService/CatalogService.Infrastructure/Repositories/ProductReadStore.csdocs/STATUS.mddocs/architecture.mddocs/ef-core.mddocs/performance-and-data-correctness.mdtests/CatalogService.Tests.Integration/ProductReadStoreTests.cstests/OrderService.Tests.Integration/OrderReadProjectionTests.cstests/OrderService.Tests.Unit/Application/OutgoingContextMiddlewareTests.cstests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs
- OrderRepository.GetSummariesByBuyerIdAsync: clamp page (>= 1) and pageSize (1..100, defaults to 50 on invalid) as defense-in-depth. HTTP endpoint's ClampPaging already enforces these, so this is a no-op under normal call flow. The clamp protects future non-endpoint callers (admin tools, background jobs, gRPC handlers) from oversized or malformed queries. Honors CLAUDE.md "Performance Rules" pagination cap at the repo layer. - OrderReadProjectionTests.GetSummariesByBuyerIdAsync ordering test: rewrote the buyer-scope IDOR assertion to use OnlyContain on the raw projection result instead of pre-filtering with .Where(buyerA). The pre-filter could mask a cross-buyer leak; OnlyContain catches it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@OrderService/Infrastructure/OrderRepository.cs`:
- Around line 64-70: The Skip offset calculation can overflow when (safePage -
1) * safePageSize exceeds int range; in the method in OrderRepository that
builds the query
(context.Orders...Where(...).OrderByDescending(...).Skip(...).Take(...)),
compute the offset using long arithmetic, clamp it to int.MaxValue, then cast to
int before calling Skip. e.g. compute a long offset = (long)(safePage - 1) *
safePageSize; if (offset > int.MaxValue) offset = int.MaxValue; then use
Skip((int)offset). This prevents negative offsets from integer overflow.
🪄 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: f5c5e55b-f048-4b10-b73f-79365ba7f3a4
📒 Files selected for processing (2)
OrderService/Infrastructure/OrderRepository.cstests/OrderService.Tests.Integration/OrderReadProjectionTests.cs
- OrderRepository.GetSummariesByBuyerIdAsync: compute Skip's offset in long arithmetic to prevent int overflow when a non-endpoint caller passes a huge page (e.g. int.MaxValue). (safePage - 1) * safePageSize in int arithmetic would wrap negative; Skip(-N) throws at execution. Return empty when the offset exceeds the addressable result set rather than letting EF translate a too-large OFFSET into a slow no-result query. - STATUS.md "Last updated" bumped 2026-05-24 → 2026-05-25. The disagreement with CodeRabbit was a timezone issue (UTC vs MDT during the late-evening window where the date crosses); CodeRabbit's UTC timestamp is what GitHub records, so matching that here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLAUDE.md "Security Requirements" lists PUT /api/v1/products/{id} as a
reference template for the canonical null/404 anti-enumeration pattern,
but the implementation drifted to throw UnauthorizedAccessException
which mapped to 403. A 403 on owner mismatch leaks existence ("this
product is there, just not yours") and lets an attacker enumerate the
product-ID space. 404 is indistinguishable from "doesn't exist."
Changes:
- UpdateProductHandler.HandleAsync: Task -> Task<bool>. Returns false
on BOTH "product not found" AND "seller mismatch" -- callers cannot
distinguish the two. Returns true on success.
- CatalogEndpoints PUT /products/{id}: awaits the bool, maps false to
404 NotFound, true to 204 NoContent. Endpoint-layer 403 on
JWT-vs-body mismatch stays (different concern: caller lied about
their own identity, not anti-enumeration).
- GlobalExceptionHandler: drop the UnauthorizedAccessException -> 403
case. No production code throws this exception after the handler
refactor -- dead branch.
- UpdateProductHandlerTests: rewrite NotFound and SellerMismatch tests
to assert the false-return contract instead of the throw. Numbered
ASSERT invariants explain the anti-enumeration property.
- ProductAuthorizationTests (new integration suite): IDOR coverage
per CLAUDE.md "Testing" rule. Two tests: (1) PUT by non-owner
returns 404 and leaves the stored row unchanged (the security
invariant); (2) PUT by owner returns 204 and persists the update
(the happy path -- pairs with the IDOR test so a regression that
broke ALL PUTs would still fail visibly).
Resolves CodeRabbit finding on UpdateProductHandlerTests.cs:114.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
May 25, 2026
…stions Triggered by Milan Jovanović's "Solving the distributed cache invalidation problem with Redis and HybridCache" article, which argues that the TTL-shortening mitigation NextAurora currently calls "the right move for ship multi-replica with reasonable consistency" is in fact a band-aid — shorter TTL shrinks the inconsistency window but doesn't eliminate it, and trades L1 hit rate to do so. Changes: - STATUS.md "If we deploy multi-replica" section: relabel options 1-3. Option 1 (TTL drop) explicitly called a band-aid acceptable for Catalog reads, not OK for pricing/permissions/flags. Option 2 added: hand-roll a Redis Pub/Sub backplane (~50-100 lines, reuses existing Redis). Option 3 (FusionCache migration) kept as the heavier "do it right out-of-the-box" path. - docs/code-flows/catalogservice.md: new Open questions section, same shape as the OrderService one (#28). Documents the gap, the two proper fixes, the band-aid's trade-off, and the trigger to act (second replica gets deployed). No code changes — documentation-only. Captures the design question in writing so the decision is informed if/when multi-replica lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
May 25, 2026
* docs: per-service code-flow walkthroughs (Mermaid) Adds one walkthrough per microservice + an index page. Each walkthrough shows which files, classes, and interfaces get touched along the most load-bearing request path in that service, using Mermaid sequenceDiagram (with class+file lane labels), stateDiagram-v2 for state machines, and graph LR for structural relationships. - docs/code-flows.md — index + the saga at a glance - docs/code-flows/orderservice.md — PlaceOrder + 3 saga handlers - docs/code-flows/catalogservice.md — cached GET, IDOR-checked PUT, gRPC ReserveStock - docs/code-flows/paymentservice.md — event consume + cascade + recovery sweeper - docs/code-flows/shippingservice.md — PaymentCompleted consume + IDOR-checked GET - docs/code-flows/notificationservice.md — minimal stateless service (no Domain folder) Mermaid chosen over Excalidraw for this use case: native GitHub markdown rendering (no PNG/SVG sibling files), no Playwright dependency, text-based source easier to maintain. Excalidraw still owns the existing visual set-pieces (nextaurora-architecture.svg, hybridcache-flow.svg, etc.). All 19 Mermaid blocks parser-clean. Gotcha worth knowing for future edits (documented in the index): inside Note over X: <content>, neither ';' nor in-content ':' is allowed — both are Mermaid parser delimiters; use em-dash or comma instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(orderservice): add Open questions on per-aggregate ordering Two-paragraph section at the bottom of the OrderService walkthrough that captures the gap surfaced by Milan Jovanović's "Solving message ordering from first principles" article: - Our per-aggregate ordering is enforced via state guards + RowVersion retry, not bus-level sessions. That matches the "model the workflow, don't fight the queue" pattern. Sessions would be additive insurance (they fix ordering, not duplicate delivery). - The validation is undertested — integration tests don't exercise concurrent same-aggregate processing, which is exactly the "subtle bug that only appears under load" the article warns about. Two cheap additions (a concurrent-event test + a retry-exhaustion metric) would let us measure rather than guess whether bus sessions are needed. No code changes — captures the design question in writing so the decision is evidence-driven if/when it lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: reframe HybridCache backplane mitigation + add catalog Open questions Triggered by Milan Jovanović's "Solving the distributed cache invalidation problem with Redis and HybridCache" article, which argues that the TTL-shortening mitigation NextAurora currently calls "the right move for ship multi-replica with reasonable consistency" is in fact a band-aid — shorter TTL shrinks the inconsistency window but doesn't eliminate it, and trades L1 hit rate to do so. Changes: - STATUS.md "If we deploy multi-replica" section: relabel options 1-3. Option 1 (TTL drop) explicitly called a band-aid acceptable for Catalog reads, not OK for pricing/permissions/flags. Option 2 added: hand-roll a Redis Pub/Sub backplane (~50-100 lines, reuses existing Redis). Option 3 (FusionCache migration) kept as the heavier "do it right out-of-the-box" path. - docs/code-flows/catalogservice.md: new Open questions section, same shape as the OrderService one (#28). Documents the gap, the two proper fixes, the band-aid's trade-off, and the trigger to act (second replica gets deployed). No code changes — documentation-only. Captures the design question in writing so the decision is informed if/when multi-replica lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: address CodeRabbit findings on PR #29 - paymentservice.md "outbox-outside-handler trap": narrow the outside-handler category to truly external execution contexts (BackgroundService, cron, code calling PublishAsync without entering a handler). Admin endpoints routed through bus.InvokeAsync (as Flow 2 shows) DO enter the handler pipeline and get AutoApplyTransactions. The original wording lumped them in with sweepers, contradicting Flow 2 in the same doc. - orderservice.md idempotency boundary: rewrite 4 spots + the Phase 2 sequence diagram to correctly separate the two enforcement layers: * Handlers pre-check Status and return early on duplicate (idempotency — actual behavior in PaymentCompletedHandler/PaymentFailedHandler/ShipmentDispatchedHandler). * Aggregate MarkAsX methods throw InvalidOperationException on invalid transitions (invariant guard, not idempotency — actual behavior in Order.cs:74-86). Original doc said the aggregate no-ops, which is wrong on both counts. The sequence diagram now shows BOTH the handler pre-check Note and the aggregate invariant Note, making the layered defense visible. - catalogservice.md: update stale literal quote. The earlier STATUS.md edit (commit 48035ec) rewrote the phrasing from "right move for ship multi-replica" to "Acceptable for ship multi-replica", and the quote in catalogservice.md was attributing text that no longer exists in STATUS.md. Paraphrased instead of attributing a literal quote so future STATUS.md rewordings don't break the doc again. Plus one Mermaid syntax fix: replaced a semicolon inside a Note over block with em-dash (the doc-wide gotcha). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 25, 2026
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
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.
added tests
Summary by CodeRabbit
New Features
Tests
Documentation
Chores