feat(messaging): adopt RabbitMQ, remove Azure Service Bus (closes #148)#159
Merged
Conversation
Official Wolverine emulator recipe confirms feat-branch Approach B and contradicts the 'emulator has no management API' rule in CLAUDE.md:230 + architecture.md:399. Tracked on #148.
…Provision docs (#148) The local saga never advanced past Placed because subscription listeners were registered with a combined "{topic}/{sub}" string instead of ListenToAzureServiceBusSubscription("{sub}", c => c.TopicName = "{topic}"), which silently mis-registers the listener so it never attaches — and fails against real Azure too, not just the emulator. Fixed across Order/Payment/Shipping/Notification. Keep Wolverine AutoProvision=false locally. Decompiling Wolverine 6.8.0 shows AzureServiceBusSubscription.InitializeAsync calls the management-API path (SetupAsync -> SubscriptionExistsAsync) ONLY when AutoProvision is true; with it false the listener binds over AMQP with no management call, so the emulator's subscription-admin HTTP 500 is never reached. An earlier "Approach B" attempt on this branch re-enabled AutoProvision + injected a management connection string — reverted here, because it reintroduced the 500. Correct the docs that claimed the emulator has no management API: it does (on the emulatorhealth endpoint, port 5300), but its subscription admin endpoints return 500 while topics/queues return 200. Updated CLAUDE.md, docs/architecture.md, docs/how-it-works.md, docs/full-saga-deployment-plan.md. Live end-to-end verification is currently blocked by an Aspire DCP container-creation timeout on the external-drive-backed Docker (unrelated to this change); the fix rests on the decompiled control-flow proof plus the stubbed-transport integration tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Azure Service Bus (#148) Make the async-messaging broker swappable via Messaging:Transport. RabbitMQ is the new default (local dev, CI, Hetzner deploy); Azure Service Bus is opt-in for Azure. Why: the ASB *emulator* never actually ran the saga locally — its subscription admin endpoints return HTTP 500, and Wolverine's system queues can't auto-provision against it (BrokerResource.Setup calls the management API for every endpoint regardless of AutoProvision). RabbitMQ runs as one clean container, AutoProvisions against the live broker, and the full saga flows locally — verified end-to-end (order -> Shipped in seconds). This also gives dev/prod parity (deployed is RabbitMQ per deployment-plan D3). - Directory.Packages.props: WolverineFx.RabbitMQ + Aspire.Hosting.RabbitMQ. - Each service Program.cs branches on Messaging:Transport (default rabbitmq). RabbitMQ maps ASB topics -> fanout exchanges, subscriptions -> queues bound to them. - AppHost provisions RabbitMQ (WithManagementPlugin) by default, ASB emulator when selected; injects Messaging__Transport (+ Wolverine__AutoProvision=false for ASB). - Integration test factories pinned to Messaging:Transport=azureservicebus so the ASB stub path is unchanged (OrderService slice green locally: 11/11). - Docs: CLAUDE.md transport rules, architecture.md messaging section, deployment-plan D3 updated to RabbitMQ-everywhere default. Carries the ASB subscription-listener API fix from #148 (correct against real Azure). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
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:
WalkthroughThis PR replaces Azure Service Bus with RabbitMQ via Wolverine across local orchestration, service transports, tests, tracing, architecture documentation, and repository guidance. It adds fanout exchange and consumer-queue topology, AMQP test stubs, Wolverine tracing, and tombstone auditing. ChangesRabbitMQ transport migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
OrderService/Program.cs (1)
34-36: ⚡ Quick winFail fast on unsupported
Messaging:Transportvalues.Line 34 currently falls back to RabbitMQ for any unrecognized value. A typo silently selects the wrong transport path; add explicit validation (
rabbitmq/azureservicebus) and throw on invalid input.Proposed guard
- var transport = builder.Configuration["Messaging:Transport"] ?? "rabbitmq"; - if (string.Equals(transport, "azureservicebus", StringComparison.OrdinalIgnoreCase)) + var transport = (builder.Configuration["Messaging:Transport"] ?? "rabbitmq").Trim(); + var useAzureServiceBus = string.Equals(transport, "azureservicebus", StringComparison.OrdinalIgnoreCase); + var useRabbitMq = string.Equals(transport, "rabbitmq", StringComparison.OrdinalIgnoreCase); + if (!useAzureServiceBus && !useRabbitMq) + { + throw new InvalidOperationException( + $"Unsupported Messaging:Transport '{transport}'. Allowed values: rabbitmq, azureservicebus."); + } + + if (useAzureServiceBus)🤖 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 `@OrderService/Program.cs` around lines 34 - 36, The transport variable assignment at line 34 silently defaults to "rabbitmq" for any unrecognized configuration value, which can mask typos or invalid configuration. Add explicit validation after retrieving the transport configuration value to verify it is either "rabbitmq" or "azureservicebus", and throw a descriptive exception if the value is not one of these supported options, ensuring that invalid configuration values fail fast rather than defaulting silently to RabbitMQ.tests/OrderService.Tests.Integration/OrderApiFactory.cs (1)
107-112: ⚡ Quick winAdd one integration startup path that exercises the default RabbitMQ branch.
Line 107 pins this factory to ASB, and the same pin exists in the Payment/Shipping factories. With RabbitMQ now the default transport, there’s no integration-host coverage for the default branch wiring. Add at least one integration slice that boots with
Messaging:Transport=rabbitmqto catch default-path startup regressions early.🤖 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.Integration/OrderApiFactory.cs` around lines 107 - 112, The current integration factories (OrderApiFactory, Payment factory, and Shipping factory) all pin the transport to Azure Service Bus, leaving the default RabbitMQ transport branch untested during integration startup. Create at least one additional integration startup factory that exercises the RabbitMQ default path by setting "Messaging:Transport" to "rabbitmq" instead of "azureservicebus" when building the integration host. This ensures the default transport branch wiring is covered by integration tests and regressions in the default startup path are caught early.
🤖 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 227-231: CLAUDE.md exceeds its own 500-line hard limit enforced at
line 207, and the new messaging rules violate the file's guidance that rules
longer than 6 lines should be condensed. Condense the ASB subscription naming
rule (starting around line 227) to approximately 2 lines, keeping only the
headline, naming convention `{consumer}-{source-events}-sub`, and the critical
point about separate arguments in ListenToAzureServiceBusSubscription (never
combined `{topic}/{sub}`), then reference supporting docs for details. Condense
the messaging transport rule (around line 230) to approximately 3 lines, keeping
only the config option `Messaging:Transport`, the two choices (RabbitMQ as
default for local dev/CI/Hetzner with full saga flow, and Azure Service Bus as
opt-in for Azure), and references to docs/architecture.md and
docs/full-saga-deployment-plan.md for emulator limitations, fanout/queue
mappings, and AutoProvision details. Move the expanded explanations currently in
lines 227-230 into supporting documentation files to bring CLAUDE.md back under
the 500-line limit while maintaining the headline-plus-reference pattern.
In `@Directory.Packages.props`:
- Line 19: The Aspire.Hosting.RabbitMQ package and other Aspire.Hosting.*
packages in lines 16-21 are pinned to version 13.3.0, while Aspire.AppHost.Sdk
is on version 13.4.0, creating a version mismatch. Update all Aspire.Hosting.*
package versions from 13.3.0 to 13.4.0 to align with the Aspire.AppHost.Sdk
version and maintain consistency across the Aspire hosting packages.
In `@docs/architecture.md`:
- Around line 399-406: The Deployment section (around line 566) contains
contradictory messaging transport guidance that conflicts with the earlier
Messaging transport section (lines 399-406). Update the Deployment section to
align with the established guidance that RabbitMQ is the default transport for
all environments (local dev, CI, and Hetzner deployment) and that Azure Service
Bus is opt-in specifically for Azure deployments. Ensure the messaging transport
discussion in the Deployment section consistently reflects that ASB should only
be used when targeting real Azure infrastructure, not as the universal standard
across every environment.
In `@docs/full-saga-deployment-plan.md`:
- Around line 160-172: The document section at lines 160-172 establishes a new
decision to use RabbitMQ as the default messaging transport across all
environments (dev, CI, and deployed), with Azure Service Bus as opt-in
configuration. However, the remaining D3 follow-on sections throughout the
document still reference the old environment-specific transport split. Locate
all subsequent D3 implications, prerequisites, and related sections that
describe the previous approach (where different environments used different
transports), and update them to align with the new RabbitMQ-everywhere policy
and its implications for dev/prod parity and configuration simplicity.
---
Nitpick comments:
In `@OrderService/Program.cs`:
- Around line 34-36: The transport variable assignment at line 34 silently
defaults to "rabbitmq" for any unrecognized configuration value, which can mask
typos or invalid configuration. Add explicit validation after retrieving the
transport configuration value to verify it is either "rabbitmq" or
"azureservicebus", and throw a descriptive exception if the value is not one of
these supported options, ensuring that invalid configuration values fail fast
rather than defaulting silently to RabbitMQ.
In `@tests/OrderService.Tests.Integration/OrderApiFactory.cs`:
- Around line 107-112: The current integration factories (OrderApiFactory,
Payment factory, and Shipping factory) all pin the transport to Azure Service
Bus, leaving the default RabbitMQ transport branch untested during integration
startup. Create at least one additional integration startup factory that
exercises the RabbitMQ default path by setting "Messaging:Transport" to
"rabbitmq" instead of "azureservicebus" when building the integration host. This
ensures the default transport branch wiring is covered by integration tests and
regressions in the default startup path are caught early.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3205f254-0b10-4e41-b057-94e523bf145d
📒 Files selected for processing (19)
.claude/audits/INDEX.mdCLAUDE.mdDirectory.Packages.propsNextAurora.AppHost/AppHost.csNextAurora.AppHost/NextAurora.AppHost.csprojNotificationService/NotificationService.csprojNotificationService/Program.csOrderService/OrderService.csprojOrderService/Program.csPaymentService/PaymentService.csprojPaymentService/Program.csShippingService/Program.csShippingService/ShippingService.csprojdocs/architecture.mddocs/full-saga-deployment-plan.mddocs/how-it-works.mdtests/OrderService.Tests.Integration/OrderApiFactory.cstests/PaymentService.Tests.Integration/PaymentApiFactory.cstests/ShippingService.Tests.Integration/ShippingApiFactory.cs
Follows the swappable-transport commit: drops the ASB branch everywhere. Per the codebase's anti-carry-debt rule, a second transport wiring that runs nowhere (the local emulator can't run the saga; there's no Azure deployment) is speculative coupling, not optionality. RabbitMQ is the broker in every environment; the transport stays swappable (Wolverine abstracts it — re-adding ASB is a ~5-line block per service, preserved in git history + decision-plan D3). - 4 services Program.cs: RabbitMQ unconditional (AutoProvision gated so it's off for tests). - AppHost: single RabbitMQ container; removed the broker selection + ASB branch. - Removed WolverineFx.AzureServiceBus + Aspire.Hosting.Azure.ServiceBus packages. - Integration test factories now stub RabbitMQ (fake amqp:// string), dropping the fake-ASB connection string + gitleaks:allow markers entirely. OrderService slice green (11/11). - Docs: CLAUDE.md, architecture.md, deployment-plan D3 → RabbitMQ-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
emeraldleaf
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
NextAurora.AppHost/AppHost.cs (1)
4-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the stale topology header comment.
Line 5 still says
Service Bus emulator, but the infrastructure block now provisions RabbitMQ. Keep the AppHost topology header aligned with the actual broker.Suggested comment fix
-// every container (Postgres, SQL Server, Redis, Service Bus emulator, Keycloak, App Insights), +// every container (Postgres, SQL Server, Redis, RabbitMQ, Keycloak, App Insights),As per coding guidelines,
AppHost.csis the canonical source of system topology.🤖 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 `@NextAurora.AppHost/AppHost.cs` around lines 4 - 5, Update the topology header comment in AppHost.cs at the beginning of the file to accurately reflect the current infrastructure. The comment currently mentions "Service Bus emulator" but this should be replaced with "RabbitMQ" to match the actual message broker implementation that is now provisioned in the infrastructure block. Ensure the comment header remains the authoritative documentation of all containers and services in the local-development topology.Source: Coding guidelines
docs/project-decisions.md (4)
1142-1142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the Dapr comparison table to reference RabbitMQ instead of Azure Service Bus.
Line 1142 in the Dapr decision section's table entry reads: "Pub/sub | Wolverine + Azure Service Bus + transactional outbox". This should be updated to "Wolverine + RabbitMQ + transactional outbox" to stay consistent with the transport migration, even though this section is explaining why Dapr is not adopted.
Proposed fix for Dapr comparison table
-| **Pub/sub** | Wolverine + Azure Service Bus + transactional outbox (§13) | Covered — and *better-integrated* (outbox in the EF `SaveChanges`) | +| **Pub/sub** | Wolverine + RabbitMQ + transactional outbox (§13) | Covered — and *better-integrated* (outbox in the EF `SaveChanges`) |🤖 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 1142, In the Dapr comparison table entry for Pub/sub messaging at line 1142 of the docs/project-decisions.md file, update the text to replace "Azure Service Bus" with "RabbitMQ" so it reads "Wolverine + RabbitMQ + transactional outbox" instead of "Wolverine + Azure Service Bus + transactional outbox" to maintain consistency with the transport migration.
607-627:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate section 12.3 header and content to reflect RabbitMQ instead of Azure Service Bus.
The section header still names "Azure Service Bus" but the PR switches the transport to RabbitMQ. The content describes async event workflows generically (which applies to both transports), but the header is now misleading. Update the title to "12.3 RabbitMQ (Wolverine) — async event-driven workflows" to align with the transport change.
Proposed fix for section title
-### 12.3 Azure Service Bus (Wolverine) — async event-driven workflows +### 12.3 RabbitMQ (Wolverine) — async event-driven workflows🤖 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` around lines 607 - 627, The section header "12.3 Azure Service Bus (Wolverine) — async event-driven workflows" references the old transport but the PR has switched to RabbitMQ. Update the section 12.3 header to replace "Azure Service Bus" with "RabbitMQ" to accurately reflect the transport choice while keeping the rest of the header and content unchanged, as the explanation of async event-driven workflows remains valid for both transports.
1059-1060:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the crib sheet to reflect RabbitMQ instead of Azure Service Bus.
Lines 1059–1060 in the "Walk me through the architecture" section state: "...and Azure Service Bus for asynchronous workflow events." This should be updated to "...and RabbitMQ for asynchronous workflow events." The crib sheet is a key quick-reference; stale transport names will mislead readers.
Proposed fix for crib sheet
-> NextAurora is a .NET 10 microservices platform with 5 backend services — Catalog, Order, Payment, Shipping, Notification. Each is independently deployable with its own database. Catalog and Shipping run on Postgres; Order and Payment on SQL Server; Notification is stateless. Cross-service communication is gRPC for synchronous queries (Order calls Catalog to validate products) and **RabbitMQ** for asynchronous workflow events.🤖 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` around lines 1059 - 1060, In the "Walk me through the architecture" section of the project decisions crib sheet, locate the sentence describing cross-service asynchronous communication and replace the reference to "Azure Service Bus" with "RabbitMQ" to accurately reflect the current message broker implementation. This is a documentation accuracy fix to ensure the quick-reference guide remains a reliable source of truth for readers about the actual infrastructure choices in the NextAurora platform.
756-757:⚠️ Potential issue | 🟠 MajorReplace stale Azure Service Bus instrumentation with correct Wolverine source across all configuration and documentation.
The
"Azure.Messaging.ServiceBus"source at line 756 is no longer used since transport is now RabbitMQ. Replace it with"Wolverine"(not"Wolverine.RabbitMQ"), which is the correct instrumentation source for Wolverine-based message processing.However, this is incomplete. Per FILE-MOVE DISCIPLINE, audit for all references to the stale instrumentation:
NextAurora.ServiceDefaults/Extensions.cs—AddSource("Azure.Messaging.ServiceBus")docs/architecture.md— references to Azure.Messaging.ServiceBus tracingdocs/observability.md— instrumentation table and span diagramsdocs/event-driven-observability.md— ActivitySource registration exampleAll must be updated to reference
"Wolverine"instead or removed if no longer applicable.🤖 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` around lines 756 - 757, Replace the stale instrumentation source "Azure.Messaging.ServiceBus" with "Wolverine" in the AddSource method call in docs/project-decisions.md at line 756, since the transport has been migrated from Azure Service Bus to RabbitMQ. Additionally, perform a comprehensive audit across all files (NextAurora.ServiceDefaults/Extensions.cs, docs/architecture.md, docs/observability.md, and docs/event-driven-observability.md) to locate any remaining references to "Azure.Messaging.ServiceBus" instrumentation or tracing and replace them with "Wolverine" or remove them if they are no longer applicable to the current RabbitMQ-based messaging implementation.
🤖 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 227-229: Update the CLAUDE.md document to remove outdated
references to Azure Service Bus that conflict with the new RabbitMQ baseline
transport guidance. Specifically, find and update the section around line 233
that still references "Async events (Azure Service Bus)" and the section around
line 284 that prescribes fake ASB connection-string literals to reflect that
RabbitMQ is now the canonical messaging transport across all environments (local
dev, CI, and production). After making these updates, run the `/check-rules`
command locally to refresh paraphrases and ensure consistency throughout the
document per the coding guidelines for CLAUDE.md changes.
In `@docs/dev-loop.md`:
- Line 392: The documentation has conflicting transport technology
specifications. Line 392 states Wolverine uses RabbitMQ for cross-service
events, but the Aspire stack section around line 390 still references a Service
Bus emulator and Gap 1 section (lines 441-454) still tracks Azure Service Bus
wire coverage. Update the Aspire stack section to replace the Service Bus
emulator reference with RabbitMQ, and update the Gap 1 section to replace all
Azure Service Bus coverage tracking with RabbitMQ coverage tracking to achieve
consistency across all three sections of the document.
In `@NextAurora.AppHost/AppHost.cs`:
- Around line 37-44: Update the architecture diagram artifacts to reflect the
RabbitMQ broker addition made in the AppHost class. Either modify the
docs/nextaurora-architecture.svg and docs/nextaurora-architecture.excalidraw
files to illustrate the RabbitMQ topology change shown in the AppHost.cs
messaging configuration, or add a tracking issue reference in the PR description
to document the deferred diagram updates in accordance with your coding
guidelines that require all three topology artifacts (architecture.md, SVG, and
Excalidraw) to be updated together.
In `@NotificationService/Program.cs`:
- Around line 25-31: Update the docs/event-catalog.md file to reflect the
current RabbitMQ queue configuration from Program.cs. Replace any references to
the outdated queue name "notify-sub" with the three new queue names that are now
being bound: "notify-orders", "notify-payments", and "notify-shipping".
Additionally, correct the documentation entry for "send-notification" to
accurately reflect that it is a RabbitMQ queue (not a Service Bus queue as
currently documented). Ensure the event catalog documentation matches the actual
queue bindings and listeners configured in the BindExchange and
ListenToRabbitQueue method calls.
In `@tests/ShippingService.Tests.Integration/ShippingApiFactory.cs`:
- Around line 71-73: The comment block describing the AutoProvision disable
logic currently uses Azure Service Bus terminology "topics/subscriptions" but
this path now provisions RabbitMQ. Replace the misleading ASB terms with the
correct RabbitMQ terminology "exchanges/queues" in the comment to accurately
describe what broker-provisioning is being disabled and avoid confusion during
test-host debugging.
---
Outside diff comments:
In `@docs/project-decisions.md`:
- Line 1142: In the Dapr comparison table entry for Pub/sub messaging at line
1142 of the docs/project-decisions.md file, update the text to replace "Azure
Service Bus" with "RabbitMQ" so it reads "Wolverine + RabbitMQ + transactional
outbox" instead of "Wolverine + Azure Service Bus + transactional outbox" to
maintain consistency with the transport migration.
- Around line 607-627: The section header "12.3 Azure Service Bus (Wolverine) —
async event-driven workflows" references the old transport but the PR has
switched to RabbitMQ. Update the section 12.3 header to replace "Azure Service
Bus" with "RabbitMQ" to accurately reflect the transport choice while keeping
the rest of the header and content unchanged, as the explanation of async
event-driven workflows remains valid for both transports.
- Around line 1059-1060: In the "Walk me through the architecture" section of
the project decisions crib sheet, locate the sentence describing cross-service
asynchronous communication and replace the reference to "Azure Service Bus" with
"RabbitMQ" to accurately reflect the current message broker implementation. This
is a documentation accuracy fix to ensure the quick-reference guide remains a
reliable source of truth for readers about the actual infrastructure choices in
the NextAurora platform.
- Around line 756-757: Replace the stale instrumentation source
"Azure.Messaging.ServiceBus" with "Wolverine" in the AddSource method call in
docs/project-decisions.md at line 756, since the transport has been migrated
from Azure Service Bus to RabbitMQ. Additionally, perform a comprehensive audit
across all files (NextAurora.ServiceDefaults/Extensions.cs,
docs/architecture.md, docs/observability.md, and
docs/event-driven-observability.md) to locate any remaining references to
"Azure.Messaging.ServiceBus" instrumentation or tracing and replace them with
"Wolverine" or remove them if they are no longer applicable to the current
RabbitMQ-based messaging implementation.
In `@NextAurora.AppHost/AppHost.cs`:
- Around line 4-5: Update the topology header comment in AppHost.cs at the
beginning of the file to accurately reflect the current infrastructure. The
comment currently mentions "Service Bus emulator" but this should be replaced
with "RabbitMQ" to match the actual message broker implementation that is now
provisioned in the infrastructure block. Ensure the comment header remains the
authoritative documentation of all containers and services in the
local-development topology.
🪄 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: e85a8a85-2974-408b-80c2-7fedb0ad8852
📒 Files selected for processing (19)
CLAUDE.mdDirectory.Packages.propsNextAurora.AppHost/AppHost.csNextAurora.AppHost/NextAurora.AppHost.csprojNotificationService/NotificationService.csprojNotificationService/Program.csOrderService/OrderService.csprojOrderService/Program.csPaymentService/PaymentService.csprojPaymentService/Program.csShippingService/Program.csShippingService/ShippingService.csprojdocs/architecture.mddocs/dev-loop.mddocs/full-saga-deployment-plan.mddocs/project-decisions.mdtests/OrderService.Tests.Integration/OrderApiFactory.cstests/PaymentService.Tests.Integration/PaymentApiFactory.cstests/ShippingService.Tests.Integration/ShippingApiFactory.cs
💤 Files with no reviewable changes (6)
- PaymentService/PaymentService.csproj
- ShippingService/ShippingService.csproj
- NotificationService/NotificationService.csproj
- NextAurora.AppHost/NextAurora.AppHost.csproj
- OrderService/OrderService.csproj
- Directory.Packages.props
…Q swap From the architecture-reviewer pass on PR #159 (0 must-fix, 4 should-consider): - ServiceDefaults/Extensions.cs: replace the now-dead OTel AddSource("Azure.Messaging.ServiceBus") with AddSource("Wolverine") (verified: Wolverine's ActivitySource) so saga message spans actually appear in the dashboard again; refresh the stale comment. - Directory.Packages.props: remove the orphaned Azure.Messaging.ServiceBus PackageVersion (no remaining PackageReference). - docs/full-saga-deployment-plan.md D3: collapse the two dated updates into one; drop the "selected via Messaging:Transport" claim — no such config switch exists (RabbitMQ is unconditional). Reconciles doc with code. - .claude/architecture-map.md: messaging lines → RabbitMQ fanout-exchange/per-consumer-queue model. - .coderabbit.yaml: encode the transport-removal sweep rule (orphaned OTel AddSource + orphaned PackageVersion) so the next transport swap catches both at review time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
Jun 18, 2026
…ure-reviewer pass Encodes the lesson from the ASB→RabbitMQ swap (PR #159): the "stay present / review significant changes" principle existed in CLAUDE.md but had no trigger at the ship moment, so it got skipped — the review (run only after a prompt) then caught a dead OTel trace source. Principle alone proved skippable, so add the mechanical catch. - CLAUDE.md: explicit rule — architecturally-significant changes get an architecture-reviewer pass before the PR opens; findings addressed or deferred in the PR body before merge. Defines "significant" concretely (transport/dep change, 3+ services, cross-cutting pattern, Domain aggregate, or /feature-spec significance). - .claude/scripts/remind-architecture-review.sh: PreToolUse(Bash) hook that surfaces the reminder when the command is `gh pr create`. Non-blocking (significance is a judgment call; a command-pattern deny could never let a PR through). Smoke-tested both branches. - .claude/settings.json: register the hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…159 - CLAUDE.md: project overview + "Async events" bullet + durability consume-side clause now say RabbitMQ; the fake-connection-string/gitleaks rule rewritten for the amqp:// reality (no markers needed today; compressed the high-entropy-fake convention for future fixtures). - docs/architecture.md: AWS section reframed ("Why SNS+SQS (not self-managed RabbitMQ) on AWS"), 1:1 topology mapping + Phase 0/1 swap steps now map FROM RabbitMQ. - docs/dev-loop.md: Aspire stack row + Gap 1 → RabbitMQ (real-wire E2E gap now framed as a RabbitMQ Testcontainer; manual live verification noted). - docs/event-catalog.md: matrix rewritten to fanout exchanges + {consumer}-{source-events} queues (old names were doubly stale), per-event Topic→Exchange labels, RabbitMQ command queue + observability headers + DLQ sections. - docs/full-saga-deployment-plan.md: topology risk callout updated (verified live). - docs/nextaurora-architecture.{excalidraw,svg,png}: re-labeled (RabbitMQ band, exchange:/ queue: boxes, saga-walk steps) and re-rendered via rebuild-diagrams.sh (doc-and-diagram pairing for the AppHost topology change). - ShippingApiFactory: AutoProvision comment says exchanges/queues. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Addressed the review in 318d506 — 7 of 9 findings fixed, 2 declined with evidence: Fixed
Declined CI note: this branch will fail NU1903 ( |
# Conflicts: # NextAurora.AppHost/AppHost.cs
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NextAurora.AppHost/AppHost.cs (1)
4-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the remaining AppHost topology comment from Service Bus to RabbitMQ.
The transport block now creates RabbitMQ, but the file header still lists “Service Bus emulator” as a spun-up container.
Proposed fix
-// every container (Postgres, SQL Server, Redis, Service Bus emulator, Keycloak, App Insights), +// every container (Postgres, SQL Server, Redis, RabbitMQ, Keycloak, App Insights),As per path instructions,
NextAurora.AppHost/AppHost.csis the canonical source of the system topology.Also applies to: 39-46
🤖 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 `@NextAurora.AppHost/AppHost.cs` around lines 4 - 6, The AppHost topology header comment still mentions the Service Bus emulator even though the transport block now provisions RabbitMQ; update the summary text in AppHost.cs so the listed containers match the actual topology, and make sure any remaining Service Bus references in the surrounding AppHost comment block are replaced with RabbitMQ to stay consistent with the transport setup.Source: Path instructions
🤖 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 `@docs/dev-loop.md`:
- Around line 448-455: The RabbitMQ Testcontainer wording is overstating the
cost savings by implying the wire test only needs a single container; update the
phrasing in the dev-loop docs around the RabbitMQ Testcontainer claim so it says
RabbitMQ removes the broker/emulator sidecar, while still acknowledging the saga
harness provisions real service DBs. Keep the existing intent, but reword the
section near the “Pragmatic solution” text to reflect the accurate runtime cost
estimate.
In `@docs/event-catalog.md`:
- Around line 136-140: The `messages.abandoned` description is inconsistent
between the DLQ section and `NextAuroraMetrics`, so align both to the same
transport-neutral wording. Update the `messages.abandoned` docstring/metric
description in `NextAuroraMetrics` and the DLQ note in the event catalog so they
both describe the counter as a generic abandoned/dead-lettering signal rather
than Service Bus-specific or RabbitMQ-specific behavior.
In `@frontend/.vscode/tasks.json`:
- Around line 8-9: The VS Code task marked by isBackground in tasks.json should
not use an empty problemMatcher, because that prevents VS Code from knowing when
the dev server is ready. Update the task definition to use a background-aware
problem matcher that detects startup and completion for the dev server task so
it can safely serve as a dependsOn prerequisite for other tasks. Use the task
configuration around isBackground and problemMatcher as the place to adjust this
behavior.
- Line 7: The frontend dev task command is binding Vite to 127.0.0.1, which may
not match the backend CORS origin documented as localhost. Update the task in
tasks.json to use the same host as the allowed browser origin, and verify the
npm run dev command in the frontend task aligns with the README and backend CORS
configuration. Use the command entry in the frontend dev task as the place to
make the host binding consistent.
In `@NextAurora.ServiceDefaults/Extensions.cs`:
- Around line 84-91: The ActivitySource documentation is stale: the tracing
setup in Extensions now registers Wolverine instead of
Azure.Messaging.ServiceBus, so update the ActivitySource table in
docs/observability.md to match the current configuration. Use the existing
observability and tracing references around the AddSource calls in Extensions
and replace the messaging source entry with Wolverine so the docs reflect the
actual spans being emitted.
---
Outside diff comments:
In `@NextAurora.AppHost/AppHost.cs`:
- Around line 4-6: The AppHost topology header comment still mentions the
Service Bus emulator even though the transport block now provisions RabbitMQ;
update the summary text in AppHost.cs so the listed containers match the actual
topology, and make sure any remaining Service Bus references in the surrounding
AppHost comment block are replaced with RabbitMQ to stay consistent with the
transport setup.
🪄 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: 56f7a9af-c6b1-4db6-a1f8-1aae6ec9f672
⛔ Files ignored due to path filters (1)
docs/nextaurora-architecture.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
.claude/architecture-map.md.coderabbit.yamlCLAUDE.mdDirectory.Packages.propsNextAurora.AppHost/AppHost.csNextAurora.ServiceDefaults/Extensions.csNotificationService/NotificationService.csprojOrderService/OrderService.csprojPaymentService/PaymentService.csprojShippingService/ShippingService.csprojdocs/architecture.mddocs/dev-loop.mddocs/event-catalog.mddocs/full-saga-deployment-plan.mddocs/nextaurora-architecture.excalidrawdocs/project-decisions.mdfrontend/.vscode/tasks.jsontests/ShippingService.Tests.Integration/ShippingApiFactory.cs
This was referenced Jul 12, 2026
Closed
Closed
…ontrol, sweep-to-zero Ultracode review of #159 found 25 real issues (2 durability bugs, 2 dead observability artifacts, ~20 doc-drift surfaces). This commit encodes the lessons + installs controls; the code bugs are tracked (#168 topology race, #169 durable inbox, #170 dead-end queue, #171 dead metric/source, #68 rescoped to failure-injection). Controls (new): - Tombstone audit: .claude/tombstones.txt + check-tombstones.sh + CI step — removed identifiers (transport APIs, old queue names) fail the build outside the allowlist. The compiler catches stale identifiers in code; this catches them in prose. - Paraphrase hook upgrade: AI-instruction surfaces (.github/copilot-instructions.md, .coderabbit.yaml) are ALWAYS listed on CLAUDE.md edits, marker or not — copilot instructions drifted invisibly because the marker convention was opt-in. Encodings: - CLAUDE.md: identifier-move discipline (grep-to-zero completion criterion + tombstones); fanout-lazy-topology trap (#168); per-direction Wolverine durability trap (#169) with the consume-side claim honestly qualified until both land; comments-link-durable-docs rule (STATUS.md is rolling state). - .coderabbit.yaml: **/Program.cs messaging checks (topology completeness, durable inbox, name consts) + docs/** tombstone/dead-signal checks. - architecture-reviewer: new **/Program.cs messaging section. Sweep-to-zero (the tombstone audit passing IS the completion proof): - 30+ files de-drifted: README front page, how-it-works (incl. the section the earlier sweep edited to a *different* removed-API form), BRD, demo-deployment, code-flows ×5, observability docs ×4, context-propagation, project-decisions (version table + Q&A), ef-core, event-replay, dotnet-10-features, copilot-instructions (+ See CLAUDE.md markers), test READMEs ×2, 15 tier-1 teaching comments across services/tests, dev-loop + transactional-outbox diagram pairs re-rendered, dangling STATUS.md pointers repointed to dev-loop Gap 1 + #68, STATUS.md refreshed. Drift RCA (5 escape routes × control): .claude/audits/2026-07-02-ultracode-review-pr159.md Verified: tombstone audit clean; build 0 warn/0 err; Order+Payment unit suites 68/68. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 17
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.Integration/OrderSagaTests.cs (1)
250-264: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not describe this as a real RabbitMQ/redelivery test.
OrderApiFactorydisables external RabbitMQ transports, soPublishMessageAndWaitAsyncexercises Wolverine’s in-process consumer pipeline; it does not validate RabbitMQ serialization, headers, broker delivery, or actual redelivery. Change the comments to say “in-process Wolverine dispatch” and reserve real-wire coverage for the planned RabbitMQ Testcontainer test.🤖 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.Integration/OrderSagaTests.cs` around lines 250 - 264, Update the comments around the two PublishMessageAndWaitAsync calls in OrderSagaTests to describe both dispatches as in-process Wolverine consumer-pipeline execution, not real RabbitMQ or broker redelivery. Remove claims about real RabbitMQ behavior and note that wire-level serialization, headers, and redelivery belong to the planned RabbitMQ Testcontainer coverage.
🤖 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/scripts/check-claude-md-refs.sh:
- Around line 38-48: Move the no-match early exit in the script so it occurs
after the ai_surfaces loop has populated the list. Exit only when both matches
and ai_surfaces are empty; otherwise continue building and emitting the
reminder, ensuring .github/copilot-instructions.md and .coderabbit.yaml are
evaluated even without marker matches.
In @.claude/scripts/check-tombstones.sh:
- Around line 36-38: Update the diagnostic output pipeline in the
tombstone-check loop to avoid SIGPIPE under pipefail by replacing the
sed-to-head pipeline with a bounded sed command such as sed -n '1,30p'. Preserve
the existing indentation and 30-hit output limit while allowing subsequent
patterns to be processed.
- Around line 32-41: Update the git grep handling in the tombstone audit loop to
capture its exit status separately from the matched output. Treat status 0 as a
violation when hits exist, status 1 as no matches, and any other
status—including invalid regex errors—as an audit failure with an appropriate
error message; preserve the existing violation reporting.
In @.github/copilot-instructions.md:
- Line 297: Add a language identifier, preferably text, to the opening fence in
the affected Markdown fenced block so it satisfies markdownlint MD040 while
preserving the block’s contents.
- Line 303: Update the outgoing-event flow description near
WolverineEventPublisher to identify the enlisted transactional API as
IMessageContext or IDbContextOutbox, rather than IMessageBus.PublishAsync.
Clarify that the outbox boundary is established through that API before dispatch
to RabbitMQ, avoiding any implication that a plain bus publish is atomic.
In `@CLAUDE.md`:
- Around line 239-240: Qualify the no-loss statement in the “Async events”
guidance to describe the intended post-fix guarantee rather than current
behavior. Explicitly preserve the documented caveats that lazily declared
topology (`#168`) and buffered listeners (`#169`) currently weaken consume-side
durability to publish-side-only, while retaining the outbox and durable-queue
explanation for the post-fix state.
In `@docs/architecture.md`:
- Line 435: Update the three stale documentation references at the context
propagation and dispatcher descriptions: replace “Service Bus” with “RabbitMQ”
or the established broker terminology, while preserving the existing identifier
and logging details. Search the remaining Markdown documentation for additional
unintended “Service Bus” references using the provided command and update any
equivalent stale references consistently.
In `@docs/BRD.md`:
- Around line 147-148: Update the REL-02 reliability requirement and its
corresponding entry to explicitly state the durability assumptions:
transactional outbox usage, durable RabbitMQ messaging, and consumer replay
support. Retain the at-least-once delivery behavior while tying the no-data-loss
guarantee to these mechanisms.
In `@docs/code-flows.md`:
- Line 77: Update the arrow legend in the Phase 1 diagram documentation to
describe solid arrows as synchronous or in-process calls and database
operations, rather than limiting them to HTTP/gRPC. Keep the dashed-arrow
RabbitMQ description unchanged.
In `@docs/how-it-works.md`:
- Line 405: Update the “Retries and DLQ” documentation to remove the claim that
exhausted retries surface as the unreliable messages.abandoned metric. Either
wire that metric before retaining the statement, or document the Wolverine
dead-letter store as the destination instead.
- Line 662: Update the “Change which events a service consumes” guidance to
point handlers to each service’s VSA `Features/` directory instead of
`{Service}.Application/Handlers/`, while retaining the
`{Service}.Api/Program.cs` references for `opts.ListenToRabbitQueue(...)` and
`rabbit.BindExchange(...).ToQueue(...)`.
In `@docs/observability.md`:
- Around line 88-93: Update the trace tree example so NotificationService’s
OrderPlaced handler is a sibling of the PaymentService branch under the initial
order-events fanout, not nested beneath the later payment-events send. Preserve
the existing PaymentService handler and payment-events branch structure.
In `@docs/project-decisions.md`:
- Line 1147: Update the transport-swap example in the deployment paragraph to
replace the incorrect WolverineFx.AzureServiceBus reference with
WolverineFx.RabbitMQ, while preserving the existing migration to
WolverineFx.AmazonSqs and the surrounding explanation.
In `@docs/STATUS.md`:
- Around line 17-19: Update the Last updated entry in docs/STATUS.md to today’s
date. If this change is for PR `#159`, revise the in-flight merge-train wording to
remove `#159` from the next slot while preserving the surrounding status details.
In `@README.md`:
- Line 161: Update the ordered-list marker in the “Verify it's working” README
section from 5. to 1. to satisfy the markdownlint MD029 requirement.
- Around line 273-279: Update the README identifier description to state that
CorrelationId is established consistently, while UserId and SessionId are
propagated only when available from their respective headers or claims. Adjust
the introductory wording and table descriptions without changing the documented
propagation mechanisms.
- Line 21: Update the testing-strategy statement in README.md to describe
handler tests as integration tests using real infrastructure. Reserve unit tests
for pure domain logic and validation, and remove the wording that presents
mocked infrastructure as the approach for handler coverage.
---
Outside diff comments:
In `@tests/OrderService.Tests.Integration/OrderSagaTests.cs`:
- Around line 250-264: Update the comments around the two
PublishMessageAndWaitAsync calls in OrderSagaTests to describe both dispatches
as in-process Wolverine consumer-pipeline execution, not real RabbitMQ or broker
redelivery. Remove claims about real RabbitMQ behavior and note that wire-level
serialization, headers, and redelivery belong to the planned RabbitMQ
Testcontainer coverage.
🪄 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: db193035-213a-4944-8335-437e404eb4cc
⛔ Files ignored due to path filters (2)
docs/dev-loop.svgis excluded by!**/*.svgdocs/transactional-outbox.svgis excluded by!**/*.svg
📒 Files selected for processing (53)
.claude/agents/architecture-reviewer.md.claude/scripts/check-claude-md-refs.sh.claude/scripts/check-tombstones.sh.claude/tombstones-allowlist.txt.claude/tombstones.txt.coderabbit.yaml.github/copilot-instructions.md.github/workflows/ci.ymlCLAUDE.mdCatalogService/Infrastructure/Data/CatalogDbContext.csNextAurora.AppHost/AppHost.csNextAurora.ServiceDefaults/Extensions.csNextAurora.ServiceDefaults/Messaging/ContextPropagationMiddleware.csNextAurora.ServiceDefaults/Metrics/NextAuroraMetrics.csNextAurora.ServiceDefaults/Middleware/CorrelationIdMiddleware.csOrderService/Domain/Order.csOrderService/Features/PaymentCompletedHandler.csOrderService/Features/PaymentFailedHandler.csOrderService/Features/ShipmentDispatchedHandler.csPaymentService/Domain/IPaymentGateway.csPaymentService/Domain/Payment.csPaymentService/Features/ProcessPayment.csPaymentService/Infrastructure/Data/PaymentDbContext.csREADME.mddocs/BRD.mddocs/STATUS.mddocs/architecture.mddocs/code-flows.mddocs/code-flows/notificationservice.mddocs/code-flows/orderservice.mddocs/code-flows/paymentservice.mddocs/code-flows/shippingservice.mddocs/context-propagation.mddocs/demo-deployment.mddocs/dev-loop.mddocs/dotnet-10-features.mddocs/ef-core.mddocs/event-driven-observability.mddocs/event-replay.mddocs/how-it-works.mddocs/observability-and-context-propagation.mddocs/observability.mddocs/performance-and-data-correctness.mddocs/project-decisions.mddocs/transactional-outbox.excalidrawtests/CatalogService.Tests.Integration/README.mdtests/OrderService.Tests.Integration/OrderApiFactory.cstests/OrderService.Tests.Integration/OrderSagaTests.cstests/OrderService.Tests.Integration/README.mdtests/OrderService.Tests.Unit/Domain/OrderTests.cstests/PaymentService.Tests.Integration/PaymentApiFactory.cstests/PaymentService.Tests.Unit/Application/OrderPlacedHandlerTests.cstests/ShippingService.Tests.Integration/ShippingApiFactory.cs
…sweep Control-script hardening (both flagged Major): - check-claude-md-refs.sh: AI-instruction surfaces list is built BEFORE the no-match early-exit, so copilot-instructions/.coderabbit.yaml are surfaced even with zero markers. - check-tombstones.sh: git grep exit >= 2 (invalid regex) now FAILS the audit instead of silently disabling the pattern (verified with an injected bad regex); sed -n '1,30p' replaces the pipefail-fragile `| head`. Docs/canon corrections: - CLAUDE.md: "already can't lose a message" opening now matches the qualified publish-side-only reality (#168/#169). - copilot-instructions.md: outbox paraphrase names the enlisted APIs (IMessageContext / IDbContextOutbox) instead of implying IMessageBus.PublishAsync is transactional; MD040 fence language. - architecture.md ×2, BRD REL-02, code-flows legend, how-it-works (dead-metric promise → management UI + #171; Application/Handlers → Features/), observability trace tree (Notification consumes order-events as a sibling, not nested under payment-events), project-decisions AWS-swap examples now start from WolverineFx.RabbitMQ (both spots — incl. the fix dropped as a MISS in the previous round). Tombstone audit clean; CLAUDE.md 324 lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Round-3 findings all addressed in 8aebb5e — including both Major script bugs (early-exit before the AI-surfaces block; invalid tombstone regex silently disabling the audit — now fails loudly, verified with an injected bad pattern) and the outbox-enlistment wording in copilot-instructions.md (now names IMessageContext/IDbContextOutbox as the transactional APIs). The CLAUDE.md no-loss claim is reworded to match the #168/#169 qualification. Nice catch on the dropped project-decisions fix — that one was logged as a MISS in my batch edit and never followed up. |
Owner
Author
|
@coderabbitai resolve |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
docs/how-it-works.md (1)
657-657: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale handler path inconsistent with the updated line 662.
Line 662 was correctly updated to say "the service's
Features/folder," but line 657 in the same table still says{Service}.Application/Handlers/. This is the same stale-path issue the past review flagged for line 662 — the fix should have been applied consistently.🔧 Proposed fix
-| Add a handler for a command/event | `{Service}.Application/Handlers/` (Wolverine discovers by convention — no interface to implement) | +| Add a handler for a command/event | `{Service}.Features/` (Wolverine discovers by convention — no interface to implement) |🤖 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` at line 657, Update the handler location in the “Add a handler for a command/event” table row to reference the service’s `Features/` folder, matching the updated guidance elsewhere in the table; leave the Wolverine convention note unchanged.
🤖 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 `@docs/architecture.md`:
- Line 435: Update the architecture documentation’s remaining stale “Service
Bus” references, including the context propagation description near
CorrelationIdMiddleware and the background dispatcher flush description, to use
RabbitMQ or the broker consistently. Run the provided ripgrep search across
Markdown documentation and replace any additional non-exempt occurrences, while
preserving legitimate Azure Service Bus, explanatory, and historical references.
---
Outside diff comments:
In `@docs/how-it-works.md`:
- Line 657: Update the handler location in the “Add a handler for a
command/event” table row to reference the service’s `Features/` folder, matching
the updated guidance elsewhere in the table; leave the Wolverine convention note
unchanged.
🪄 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: 72a465b1-8462-46c3-a4a9-b0fda2e0b468
📒 Files selected for processing (10)
.claude/scripts/check-claude-md-refs.sh.claude/scripts/check-tombstones.sh.github/copilot-instructions.mdCLAUDE.mddocs/BRD.mddocs/architecture.mddocs/code-flows.mddocs/how-it-works.mddocs/observability.mddocs/project-decisions.md
✅ Action performedComments resolved and changes approved. |
emeraldleaf
added a commit
that referenced
this pull request
Jul 12, 2026
…hase 3) (#167) * feat(frontend): saga timeline + narrator on order detail (epic #130 Phase 3) The centerpiece feature: the order-detail page now renders a live Placed → Paid → Shipped timeline off the polled order status, with a narrator panel explaining what the backend just did at each hop (202 + transactional outbox → RabbitMQ fanout → idempotent consumers → choreography, no orchestrator). Unblocked by the saga actually flowing locally on RabbitMQ (PR #159). - features/orders/saga.ts: OrderStatus union mirroring the backend enum, step model with teaching-grade narration copy, deriveStepStates + isSagaSettled. - api/orders.ts: status typed as OrderStatus; orderByIdQuery polls every 2s via dynamic refetchInterval while the saga is in flight and stops once it settles (Shipped/Delivered/Cancelled/PaymentFailed). Polling lives in the query definition, not component timers. - components/SagaTimeline.tsx: pure-render timeline + narrator panel; aria-current marks the live step; failure branch (PaymentFailed/Cancelled) renders loud, not stuck. - Tests (11 new, AAA-narrated per canon): saga helper table tests pin the full visual contract per status + the polling lifecycle; RTL component tests cover in-flight, settled, and failure states by role/label. typecheck + eslint clean; vitest 24/24. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(frontend): Cancelled renders honestly — no failure glyph on a step the status can't blame CodeRabbit findings on #167: Cancel() is legal from both Placed and Paid, and the DTO carries only the current status — so PROGRESS[Cancelled]=1 rendering Paid as 'failed' misstated a cancel-before-payment. Cancelled now renders Placed complete + remaining steps pending; the terminal panel (Saga stopped: Cancelled) carries the story. Adds the missing Cancelled error-path tests (derivation table + timeline render), per the frontend canon's error-path coverage rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
Jul 13, 2026
…g the very drift it audits The review of this branch caught a structural flaw in MY OWN control, plus a half-done deletion. Both Must-fixes applied. MUST-FIX 1 — the tombstone allowlist was FILE-scoped, so it silently exempted files from every FUTURE tombstone: docs/architecture.md was allowlisted for the ASB patterns (#159). That exemption then covered the send-notification and messages.abandoned tombstones added months later — so `check-tombstones.sh` reported "clean" while architecture.md still documented BOTH deleted artifacts as live (a publishable SendNotificationCommand row; messages.abandoned as the DLQ alarm). The control that exists to make "sweep to zero" mechanical was returning green on a sweep that wasn't. Found by architecture review, not by the control. FIX: tombstones are now GROUPED ([group-name] headings) and allowlist entries are `<group> <path>` — an exemption is scoped to ONE group, never the whole audit. Rewritten check-tombstones.sh enforces it. The fixed control immediately surfaced 6 hidden hits, all now swept (architecture.md ×6, performance-and-data-correctness.md). MUST-FIX 2 — #171 deleted 1 of 5 dead counters; the whole class was dead: NextAuroraMetrics was registered via AddSingleton but NEVER INJECTED anywhere — every real counter is created inline (`new Meter("NextAurora")`) in the handler that emits it. So all five of its counters were declared-and-never-incremented, i.e. the exact defect #171 exists to eliminate, in the file the PR was already editing. Deleted the class + its registration. Also (should-consider): - Extensions.cs now names the meter explicitly: Wolverine's is `Wolverine:{ServiceName}`, so AddMeter("Wolverine*") is LOAD-BEARING — a literal AddMeter("Wolverine") collects nothing. (Reviewer runtime-verified this against Wolverine 6.8.0 + OTel 1.15.3.) - Re-flowed two tier-1 comments left mid-sentence by the surgical edits; fixed a doubled `---` in event-catalog.md; framed the project-decisions OTel snippet as as-of-that-decision; noted the topology guard's direct-queue branch is now a forward-guard. Rules encoded (all three the review asked for): - CLAUDE.md: allowlist exemptions are group-scoped, never whole-audit. - architecture-reviewer: new "metrics / Meter / Counter" checklist section — a declared-but-never-incremented instrument is a DEFECT (it reads as a working alarm and never fires); prefer the framework's own instruments. - .coderabbit.yaml: **/Metrics/*.cs dead-instrument rule + the Wolverine meter-name rule for ServiceDefaults (don't "tidy" the wildcard). Verified: build 0 err; tombstone + messaging-topology audits clean; 145/145 tests (integration 40 — Order 11, Payment 6, Shipping 4, Catalog 19; unit 105). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
Jul 19, 2026
…etrics class + trace source (#171) (#174) * chore: remove dead artifacts — send-notification queue (#170), abandoned-counter + emitter-less trace source (#171) Closes #170. Closes #171. Both were found by the ultracode review of #159: things the docs presented as live that nothing actually used. Canon says speculative/dead coupling gets deleted (same rule that retired IRecipientResolver), so both go. #170 — send-notification was dead-end wiring: - NextAurora.Contracts.Commands.SendNotificationCommand had ZERO references — no handler, no publisher. The working path is unaffected: event handlers return SendNotificationRequest and Wolverine cascades it IN-PROCESS to SendNotificationHandler; it never touched the queue. - Worse than dead: event-catalog.md told any service it could publish there, and such a message would arrive unhandleable and be silently discarded. - Removed: the contract + Commands/ folder, MessagingQueues.SendNotification, the listener, the catalog row + command section, the architecture/README/observability entries, and the diagram box. #171 — two artifacts documented as live, incremented/emitted by nothing: - NextAuroraMetrics.MessagesAbandoned: declared, never .Add()-ed (the ASB processors that fed it died in the Wolverine migration). An operator alerting on it got a permanently-zero signal while Wolverine dead-lettered silently. - .AddSource("NextAurora.Messaging"): registered with no emitter. - ROOT CAUSE + real fix: only the "NextAurora" meter was registered, so Wolverine's OWN metrics were never collected. Now .AddMeter("Wolverine*") — wolverine-dead-letter-queue (the real DLQ alarm), -execution-failure, -messages-sent/-received, inbox depth. The dead counter is deleted and REPLACED by telemetry that actually fires. Identifier-move discipline applied: removed names tombstoned; swept to zero (the audit is the completion criterion, not memory). Documented a newly-found blind spot in the audit — line-broken identifiers in .excalidraw JSON evade the regex (found via the rendered PNG, not the grep), so diagram hits need a visual check too. Verified: build 0 warn/0 err; tombstone + messaging-topology audits clean; integration 40/40 (Order 11, Payment 6, Shipping 4, Catalog 19); unit 57/57; both diagrams re-rendered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: architecture-review findings — the tombstone allowlist was hiding the very drift it audits The review of this branch caught a structural flaw in MY OWN control, plus a half-done deletion. Both Must-fixes applied. MUST-FIX 1 — the tombstone allowlist was FILE-scoped, so it silently exempted files from every FUTURE tombstone: docs/architecture.md was allowlisted for the ASB patterns (#159). That exemption then covered the send-notification and messages.abandoned tombstones added months later — so `check-tombstones.sh` reported "clean" while architecture.md still documented BOTH deleted artifacts as live (a publishable SendNotificationCommand row; messages.abandoned as the DLQ alarm). The control that exists to make "sweep to zero" mechanical was returning green on a sweep that wasn't. Found by architecture review, not by the control. FIX: tombstones are now GROUPED ([group-name] headings) and allowlist entries are `<group> <path>` — an exemption is scoped to ONE group, never the whole audit. Rewritten check-tombstones.sh enforces it. The fixed control immediately surfaced 6 hidden hits, all now swept (architecture.md ×6, performance-and-data-correctness.md). MUST-FIX 2 — #171 deleted 1 of 5 dead counters; the whole class was dead: NextAuroraMetrics was registered via AddSingleton but NEVER INJECTED anywhere — every real counter is created inline (`new Meter("NextAurora")`) in the handler that emits it. So all five of its counters were declared-and-never-incremented, i.e. the exact defect #171 exists to eliminate, in the file the PR was already editing. Deleted the class + its registration. Also (should-consider): - Extensions.cs now names the meter explicitly: Wolverine's is `Wolverine:{ServiceName}`, so AddMeter("Wolverine*") is LOAD-BEARING — a literal AddMeter("Wolverine") collects nothing. (Reviewer runtime-verified this against Wolverine 6.8.0 + OTel 1.15.3.) - Re-flowed two tier-1 comments left mid-sentence by the surgical edits; fixed a doubled `---` in event-catalog.md; framed the project-decisions OTel snippet as as-of-that-decision; noted the topology guard's direct-queue branch is now a forward-guard. Rules encoded (all three the review asked for): - CLAUDE.md: allowlist exemptions are group-scoped, never whole-audit. - architecture-reviewer: new "metrics / Meter / Counter" checklist section — a declared-but-never-incremented instrument is a DEFECT (it reads as a working alarm and never fires); prefer the framework's own instruments. - .coderabbit.yaml: **/Metrics/*.cs dead-instrument rule + the Wolverine meter-name rule for ServiceDefaults (don't "tidy" the wildcard). Verified: build 0 err; tombstone + messaging-topology audits clean; 145/145 tests (integration 40 — Order 11, Payment 6, Shipping 4, Catalog 19; unit 105). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: build the 6th-surface ADR (OKL) and point canon at its tracked path The 6th surface — a fail-closed, cross-repo knowledge layer — is built (the OKL), so the tracked ADR replaces the gitignored docs/post-ideas/ draft the canon had been pointing at (a dangling 404, itself the doc-orphan drift the method polices). - docs/decisions/2026-07-17-sixth-surface-okl.md: the decision record - CONTEXT.md: 6th-surface note "not built" -> "built as the OKL", ref repointed - .claude/commands/grid-infographic.md: ref repointed to the tracked path * fix(loop): harden tombstone script + refresh STATUS — CodeRabbit on #174 - check-tombstones.sh: git grep -e "$pattern" (a tombstone starting with '-' is no longer parsed as a flag); guard "${excludes[@]}" so an exemption-less group doesn't trip set -u on bash 3.2 (macOS default); read -r grp pth instead of set -- (handles spaces, drops the shellcheck disable). Verified: clean run + a leading-hyphen pattern in an empty-exemption group matches without crashing. - STATUS.md: Last-updated date + #170/#171/#174 state. Declined the LanguageTool 'exactly'->'precisely' nit on the ADR (subjective, no correctness or consistency impact). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: STATUS reworded — my own refresh reintroduced the tombstoned NextAuroraMetrics literal The tombstone CI audit correctly caught it (I edited STATUS after the last local audit run and didn't re-verify). A living status doc doesn't earn a permanent allowlist — reworded to 'business-metrics holder class'. Both audits re-run clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Joshua Dell <joshuadell@Joshuas-MacBook-Air.local>
emeraldleaf
added a commit
that referenced
this pull request
Jul 19, 2026
…ure-reviewer pass (#160) * chore(dev-loop): gate architecturally-significant PRs on an architecture-reviewer pass Encodes the lesson from the ASB→RabbitMQ swap (PR #159): the "stay present / review significant changes" principle existed in CLAUDE.md but had no trigger at the ship moment, so it got skipped — the review (run only after a prompt) then caught a dead OTel trace source. Principle alone proved skippable, so add the mechanical catch. - CLAUDE.md: explicit rule — architecturally-significant changes get an architecture-reviewer pass before the PR opens; findings addressed or deferred in the PR body before merge. Defines "significant" concretely (transport/dep change, 3+ services, cross-cutting pattern, Domain aggregate, or /feature-spec significance). - .claude/scripts/remind-architecture-review.sh: PreToolUse(Bash) hook that surfaces the reminder when the command is `gh pr create`. Non-blocking (significance is a judgment call; a command-pattern deny could never let a PR through). Smoke-tested both branches. - .claude/settings.json: register the hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(audits): log vibe-coding audit No action — article already encoded more rigorously in the repo’s AI workflow rules. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
Jul 19, 2026
…ed after #175 (#176) #175 bumped six WolverineFx.* packages to 6.17.3 but left WolverineFx.RabbitMQ at 6.8.0 — a 9-minor skew within one framework whose core + transport adapters ship in lockstep (an adapter that far behind core references APIs that may have changed; unsupported combination). Root cause is a dependabot timing artifact, not a config bug: the `wolverine` group pattern (`WolverineFx*`) does cover RabbitMQ, but the package only entered the manifest via #159 AFTER #175 was opened, so its group snapshot missed it. - Directory.Packages.props: WolverineFx.RabbitMQ 6.8.0 → 6.17.3 (whole family now aligned). - CLAUDE.md Package Management: encode "all WolverineFx.* move together" (parallel to the existing Aspire SDK/runtime lockstep rule) — a build passes on skew, so it's a grep-Directory.Packages.props check, and any newly-added WolverineFx.* must match the family. - Also swept a pre-group-scoped-audit tombstone leak: the WaitFor bullet still said "Service Bus emulator" → RabbitMQ / SQL Server. Verified: build 0 errors across the version jump (proves API compat); tombstone + messaging- topology audits clean. Integration tests deferred to CI (local Docker down); build-clean rules out API breakage, and CI runs the RabbitMQ-transport suites. Co-authored-by: Claude Fable 5 <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
1 participant
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
Replaces Azure Service Bus with RabbitMQ as NextAurora's async messaging transport, in every environment (local dev, CI, Hetzner deploy). Wolverine abstracts the broker, so the saga, handlers, outbox, and idempotency are unchanged — only the per-service transport block + AppHost wiring.
RabbitMQ maps the saga's pub/sub onto fanout exchanges (
order-events,payment-events,shipping-events) with a queue per consumer bound to each;AutoProvision()declares it all against the live broker.Why (the journey, in two commits)
Verified
Shippedin seconds end-to-end (Payment + Shipping consumed and re-published). The thing the ASB emulator never did.dotnet buildclean (0 warnings/errors).Cleanups this unlocked
amqp://guest:guest@localhost:5672— the fake-ASB connection string + everygitleaks:allowmarker is gone (that whole apparatus existed only for ASB's high-entropy key).WolverineFx.AzureServiceBus+Aspire.Hosting.Azure.ServiceBus.Docs
CLAUDE.md (transport rule + dropped the dead ASB subscription-naming rule), architecture.md (messaging section, topology diagram, summary table), deployment-plan D3.
Deferred follow-up (noted, not blocking): a residual docs sweep of the AWS-migration planning section in architecture.md and the voiced-answer appendix in project-decisions.md (both still use ASB as the illustrative "from" transport — historical/hypothetical, not current-state claims). Plus the RabbitMQ Testcontainer real-wire saga test (STATUS.md gap).
Notes
/check-rulesaudit: CLAUDE.md transport rule reframed to RabbitMQ-only; the ASB subscription-naming rule removed (dead). Inline rule-paraphrase comments end withSee CLAUDE.md.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests