Uh oh!
There was an error while loading. Please reload this page.
docs: ADR-0032 — exception handling, logging, observability foundation - #2
Merged
Merged
Conversation
Three round-3 nitpicks against the Phase 01 packets 4-6 surface. infra/dapr/README.md: - Phase-assignment ambiguity. The building-blocks table said `ICacheService` / `ISecretProvider` ship in 02a but `IEventBus` in 02b, while a later paragraph claimed all three Dapr-backed implementations ship in 02b. The canonical answer (per phase-02a § Shared Kernel + § Dapr Building Blocks) is that ALL three interfaces AND their Dapr-backed implementations ship in 02a; only the OutboxProcessor (the consuming call site for IEventBus) waits until 02b. Replaced the per-row "(Phase 02a/02b)" hint with a dedicated phase-ownership block and corrected the trailing paragraph. - Sidecar-topology ASCII art replaced with a Mermaid `graph LR` block per CLAUDE.md hard rule (diagrams use Mermaid; text fallback retained for non-Mermaid renderers per the same rule). infra/coturn/turnserver.conf: - The deliberate omission of `external-ip`, the still-absent `rtc.turn_servers` block in livekit.yaml, and the static user/secret pair are all Phase 08c work. Added a "DEFERRED to Phase 08c" header pointing at the three concrete follow-ups (external-ip injection, livekit-side turn_servers wiring, ILiveClassProvider per-session credential minting via `use-auth-secret`). docs/standards/12-infrastructure.md: - MD040: the two service-list fenced code blocks were unmarked. Added the `text` language identifier to both so the standard renders consistently and the markdown linter is happy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codifies the pre-implementation cross-cutting foundation as a single binding contract. Thirteen sub-decisions close the gap between Standards 02 ↔ 09 ↔ 10 and ADR-0016's pipeline-order rule, the Sentry/OTel ambiguity, the Serilog/OTel double-export risk, tenant.id propagation, deployment-mode-aware error tracking, the provider resilience pattern, and controller Result mapping. Key bindings: - 8-step MediatR pipeline (Validation → Logging → AuditLog → TenantContext → Authorization → Transaction → OutboxFlush → Handler). No ExceptionHandlingBehavior. - IExceptionHandler (.NET 8+) as L1; ValidationBehavior returns Result.Fail(validation_failed) and never throws. - DomainException reserved for programmer errors; expected business-rule violations return Result.Fail(business_rule_violation). Roslyn analyzer flags violations. - IProviderResilience<TPort> decorator with Polly v8 ResiliencePipeline (retry + circuit breaker + timeout + bulkhead) per appsettings.Resilience:<port>: section. - Sentry vs OTel error capture partitioned via ShouldCapture(ex): bugs + 5xx provider exceptions go to Sentry; Result.Fail + 4xx provider + cancel do not. - Serilog primary logger; OTLP sink to OTel Collector; AddOpenTelemetry().WithLogging() deliberately not registered alongside. - TenantContextSpanProcessor enriches every span (auto-instrumented + manual) with tenant.id / organization.id / user.id / module / correlation.id. - IErrorTrackingProvider socket branches on DeploymentMode (NoOp / Sentry / LocalFile); air-gapped Self-Hosted gets the local-file tracker. - W3C traceparent threads through HTTP + outbox rows + Hangfire payloads + Hub /api/internal/* envelopes; correlation continues across async boundaries. Changes: - NEW: docs/decisions/0032-exception-handling-logging-and-observability.md - NEW: docs/architecture/33-cross-cutting-concerns.md (conceptual deep dive, diagrams) - NEW: .claude/skills/wire-cross-cutting-foundation/SKILL.md (Phase 02a one-time foundation wiring) - NEW: .claude/skills/add-provider-adapter/SKILL.md (canonical pattern for every new adapter) - Standards 02 § Pipeline Behaviors rewritten to match ADR-0032 + ADR-0016. - Standards 09 gains L1, Sentry/OTel boundary, validation Result-not-throw, DomainException disciplina, provider resilience, controller Result mapping. - Standards 10 gains Serilog primary, IErrorTrackingProvider table, tenant.id auto-enrichment, Hub correlation, outbox/job correlation references. - Standards 20 adds IErrorTrackingProvider + OTLP exporter rows to the composition-root deployment-mode table; forbidden list adds direct Sentry.SentrySdk. - Roadmap phase-02a § Cross-cutting Concerns (Day 1) lists every deliverable + ten new architecture tests; phase-02b cites ADR-0032 for outbox / Hangfire correlation. - Glossary adds Result<T>, Error, Pipeline Behavior, IExceptionHandler, Correlation ID, Telemetry Signal, IErrorTrackingProvider, IProviderResilience<TPort>, TenantContextSpanProcessor, ProviderException.IsClientError, L1/L2/L3 cache. - decisions/README.md active-ADR table gains row 0032. - CLAUDE.md "Things to never do" gains six rules derived from ADR-0032. - add-mediatr-handler skill updated to reference the canonical pipeline order and the bug-only DomainException rule. - skills/README.md catalogue gains the two new skills. No code changes — repository is pre-implementation; only Result<T> and Error records exist in backend/src/LearnStack.SharedKernel/Results/ already. The contract above lands during Phase 02a. ADR: 0032 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…estion Code-review agent flagged eight findings on bb0cf16 (ADR-0032 + cross-cutting concerns). Aksiyon haritası: Major ----- - Major #1: TenantContextSpanProcessor would fail at startup with "Cannot consume scoped service ITenantContext from singleton" if the constructor takes the request-scoped ITenantContext directly. Introduce ITenantContextAccessor (singleton, AsyncLocal<ITenantContext?>-backed, analogous to IHttpContextAccessor). The scoped interface remains for handler-facing code; the singleton accessor is what cross-cutting infrastructure (OTel processor, Serilog enricher, Sentry enricher) reads. The accessor is set at scope start by TenantResolverMiddleware, HubCorrelationMiddleware, Hangfire JobActivator, and the outbox / inbox handler scope. Phase 02a ships both contracts together. Updated: ADR-0032 § Sub-decision 10 + § Implementation Notes (TenantContextSpanProcessor shape), wire-cross-cutting-foundation skill (new Step 4 + accessor pattern), Standards 10 § Span Attributes, Phase 02a roadmap deliverables, Architecture 33 § Tracing Stack, glossary entry. - Major #2: ADR-0032 § Sub-decision 5 originally listed "Hub HTTP clients" among IProviderResilience<TPort>-wired adapters and carried a Resilience.hub: row in the appsettings example. The new add-provider-adapter skill correctly excludes Hub adapters because they have an additional mTLS + signed JWT + HMAC wrapper per ADR-0019. The two documents disagreed. Resolve by removing "Hub HTTP clients" from the adapter list and dropping the appsettings example row — Hub adapters get their resilience inside the ADR-0019 wrapper, defined when the Hub adapter itself lands in Phase 02c. Minor ----- - Minor #3: Standards 09 introductory Mermaid diagram still labelled "Global exception middleware" — replaced with "L1 IExceptionHandler" to match the rewritten section below. - Minor #4: "(200 OK on response = success at runtime)" parenthetical in the Sentry/OTel partition table conflated HTTP status (Result.Fail returns 4xx) with OTel ActivityStatusCode. Reworded to "runtime completed; HTTP response is the appropriate 4xx Problem Details". - Minor #5: Standards 09 listed a custom UnreachableException subclass that collides with System.Diagnostics.UnreachableException (.NET 7+). Dropped the subclass; standardized on the BCL type. Skill snippet qualified. - Minor #6: ADR-0032 was missing the Deciders: line that ADR-0029/30/31 carry. Added "@platform". - Minor #8: Standards 10 § Correlation table omitted organization_id even though sub-decision 10 binds the processor to enrich every span with it. Added the row, cited ADR-0017. - Minor #9: The eight-row Sentry/OTel partition table was duplicated in ADR-0032 § Sub-decision 7 and Standards 09 § Sentry vs OpenTelemetry. Two copies will drift. Kept the authoritative table in Standards 09; ADR-0032 now carries a compact summary and cites the standard. Suggestion ---------- - Suggestion #10: Renamed the architecture test Pipeline_Order_Matches_ADR_0032 → MediatR_Pipeline_Order_Matches_Canonical_Sequence in all five reference sites (ADR-0032, wire-cross-cutting-foundation, glossary, phase-02a roadmap, Standards 02). The ADR citation moves to the test's [Description] attribute at implementation time. Embedding the ADR number in the assembly-level identifier would bake a version into the test name; the Subject_Constraint naming form already used by the rest of the architecture-test set (Modules_Do_Not_Reference_DeploymentMode, Every_TenantOwned_Command_HasAuditCoverage) is the correct pattern. Not addressed (intentional) --------------------------- - Minor #7: The prior commit body (bb0cf16) understated the CLAUDE.md rule count ("six" vs eight added). The body is the durable record; amending it would rewrite history. Noted here for future readers. ADR: 0032 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideIntroduces ADR-0032 and an associated architecture doc to define the cross-cutting contract for exception handling, logging, and observability, then aligns standards, roadmap docs, glossary, skills, and infra readmes around a canonical MediatR pipeline, L1 exception handler, provider resilience pattern, tenant-context tracing enrichment, and deployment-mode-specific error tracking—all without changing runtime code. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR establishes ADR-0032 as the binding cross-cutting contract for LearnStack's exception handling, logging, observability, error tracking, and provider resilience. It documents the canonical architecture across multiple standards, adds comprehensive implementation guidance through two new skills, and integrates these patterns into Phase 02a and 02b roadmaps. ChangesCross-Cutting Exception Handling, Logging, and Observability Architecture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The exception hierarchy now recommends using
System.Diagnostics.UnreachableExceptiondirectly in Standards 09, but ADR-0032 / 33-cross-cutting still listUnreachableExceptiongenerically in the LearnStack hierarchy; consider explicitly naming the BCL type and clarifying that it is not aLearnStackExceptionsubclass to avoid confusion and namespace collisions. - ADR-0032, the standards, and the skills docs all hard-code many analyzer and architecture test identifiers (e.g.
MediatR_Pipeline_Order_Matches_Canonical_Sequence,LearnStackException-DomainExceptionThrow); consider adding a small central index/table of these test/analyzer names and linking to it from the docs so renames or relocations only need to be updated in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- The exception hierarchy now recommends using `System.Diagnostics.UnreachableException` directly in Standards 09, but ADR-0032 / 33-cross-cutting still list `UnreachableException` generically in the LearnStack hierarchy; consider explicitly naming the BCL type and clarifying that it is not a `LearnStackException` subclass to avoid confusion and namespace collisions.
- ADR-0032, the standards, and the skills docs all hard-code many analyzer and architecture test identifiers (e.g. `MediatR_Pipeline_Order_Matches_Canonical_Sequence`, `LearnStackException-DomainExceptionThrow`); consider adding a small central index/table of these test/analyzer names and linking to it from the docs so renames or relocations only need to be updated in one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request establishes a comprehensive cross-cutting foundation for error handling, logging, and observability, primarily codified in the new ADR-0032. Key changes include the implementation of a two-track failure model, a canonical eight-step MediatR pipeline, and a standardized provider-resilience pattern using Polly v8. Review feedback identified opportunities to refine the L1 exception handler by excluding client cancellations from error telemetry, suggested providing a concrete implementation for server-side error detection in the adapter templates, and recommended explicit referencing of the BCL UnreachableException to maintain consistency across documentation.
Comment on lines
+507
to
+508
| Activity.Current?.RecordException(ex); | ||
| Activity.Current?.SetStatus(ActivityStatusCode.Error, ex.GetType().Name); |
There was a problem hiding this comment.
The implementation snippet for LearnStackExceptionHandler contradicts the error capture boundary defined in Sub-decision 7 and Standards 09. For OperationCanceledException, the span status should not be set to Error and the exception should not be recorded, as it represents an expected client disconnection rather than a system failure. Recording the exception typically forces the span status to Error in most OpenTelemetry exporters.
Suggested change
| Activity.Current?.RecordException(ex); | |
| Activity.Current?.SetStatus(ActivityStatusCode.Error, ex.GetType().Name); | |
| if (ex is not OperationCanceledException) | |
| { | |
| Activity.Current?.RecordException(ex); | |
| Activity.Current?.SetStatus(ActivityStatusCode.Error, ex.GetType().Name); | |
| } |
| throw new LiveClassProviderException( | ||
| "provider.quota_exceeded", ex.Message, ex, isClientError: true); | ||
| } | ||
| catch (HttpRequestException ex) when (IsServerError(ex)) |
There was a problem hiding this comment.
The adapter template uses an undefined IsServerError(ex) helper. Since this skill is intended for AI agents to follow, providing the implementation of this helper or using a direct check on the StatusCode property of HttpRequestException (available in .NET 5+) would ensure the generated code is correct and follows the ADR-0032 mapping rules (where null status codes like DNS failures are treated as infrastructure faults).
Suggested change
| catch (HttpRequestException ex) when (IsServerError(ex)) | |
| catch (HttpRequestException ex) when (ex.StatusCode is null || (int)ex.StatusCode >= 500) |
| fault; Sentry). | ||
| - `TenantContextMissingException` — request reached the pipeline without a | ||
| resolved tenant. | ||
| - `UnreachableException` — case branch the type system can't prove |
There was a problem hiding this comment.
To maintain consistency with Standards 09, this reference should explicitly mention System.Diagnostics.UnreachableException to clarify that the BCL exception is preferred over a custom subclass for this specific use case.
Suggested change
| -`UnreachableException` — case branch the type system can't prove | |
| -System.Diagnostics.UnreachableException — case branch the type system can't prove |
PR #2 picked up three medium-severity findings from gemini-code-assist and one matching upper-level note from sourcery-ai (coderabbit rate- limited; no review). All three are snippet corrections — narrow scope, no contract change. - G1: LearnStackExceptionHandler snippet (ADR-0032 § Implementation Notes) called Activity.RecordException + SetStatus(Error) unconditionally, contradicting Sub-decision 7's rule that OperationCanceledException stays out of the error path. Snippet now short-circuits OperationCanceled before the exception-recording branch. Standards 09 and Architecture 33's "SetStatus(Cancelled)" notation also corrected — ActivityStatusCode has only Unset / Ok / Error, so the right value for a client disconnect is "leave Unset" (no RecordException, no SetStatus call). Three files touched. - G2: add-provider-adapter skill's LiveKitClient template used an undefined IsServerError(ex) helper in a `catch (HttpRequestException) when (...)` clause. .NET 5+ exposes HttpRequestException.StatusCode as HttpStatusCode? — null means transport failure (DNS, refused, timeout) which is an infra fault. Snippet now uses `ex.StatusCode is null || (int)ex.StatusCode >= 500`, matching ADR-0032 § Sub-decision 5's IsClientError mapping table. - G3: 33-cross-cutting-concerns.md's exception-hierarchy bullet list still mentioned `UnreachableException` generically. Standards 09 switched to the BCL `System.Diagnostics.UnreachableException` in the earlier review round; Architecture 33 now matches with the same qualified name and the explicit "not a LearnStackException subclass" clarification sourcery requested. Sourcery's second upper-level suggestion — a central index of architecture-test / Roslyn-analyzer identifiers so renames only touch one place — is acknowledged but out of scope for this PR. The catalog will emerge naturally in Phase 02a when the tests are actually written. Ref: PR #2 reviews from gemini-code-assist and sourcery-ai (2026-05-19). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…R-0032 unreachable note Two sourcery upper-level findings on PR #2: 1. ADR-0032 and Architecture 33 listed `UnreachableException` generically while Standards 09 had already switched to the BCL `System.Diagnostics.UnreachableException`. Risk: future readers see the ADR's generic name and add a custom `LearnStackException` subclass, colliding with the BCL type. Resolved by adding an explicit note under ADR-0032 § Sub-decision 4 clarifying that the BCL type is the sanctioned one and is **not** a `LearnStackException` subclass. (Architecture 33 already carried the qualified name from the previous review round.) 2. Architecture-test and Roslyn-analyzer identifiers (`MediatR_Pipeline_Order_Matches_Canonical_Sequence`, `LearnStackException-DomainExceptionThrow`, …) were hard-coded across 5-8 documents each. Rename or relocation forced a multi-file edit with no single source of truth. Resolved by creating `docs/standards/21-architecture-tests-catalogue.md` as the canonical registry: per-identifier section with assertion + source + type + phase, and a documented "rename touches the catalogue first" rule. Catalogue scope in this PR: the 12 architecture tests + 1 Roslyn analyzer introduced by ADR-0032. Earlier-ADR tests (audit, Dapr, tenancy, deployment-mode) are listed as "to be backfilled" — they migrate into the catalogue as their source docs are touched, no rewrite-for-rewrite churn. Cross-link wiring: - Standards 02 / 09 / 10 / Roadmap 02a / 02b / CLAUDE.md each gain a one-paragraph pointer to the catalogue alongside their existing identifier mentions. Eight cite sites in total. - Standards index gains row 21. - ADR-0032 References list gains the catalogue. Naming convention codified in the catalogue: architecture tests use `Subject_Constraint` (no ADR number baked in — the ADR cite belongs in the test's `[Description]` attribute, not the type name); Roslyn analyzer IDs use `LearnStackException-<Topic>`. The two namespaces are disjoint by prefix. Ref: PR #2 sourcery review (2026-05-19). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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)
infra/dapr/README.md (1)
119-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConflicting phase ownership in this document needs to be resolved
Line 119 and Line 120 say the
IEventBus/ICacheService/ISecretProviderimplementations are Phase 02b, but Line 18–23 and Line 86 state they ship in Phase 02a. Please make these sections consistent so phase ownership is unambiguous.Based on learnings "Foundation building blocks (Dapr IEventBus/ICacheService/ISecretProvider, APISIX gateway, audit infrastructure, organization scope, entitlement projection socket, host-to-tenant resolver, architecture tests) ship in Phase 02a".
🤖 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 `@infra/dapr/README.md` around lines 119 - 120, Resolve the conflicting phase assignment for the Dapr interfaces by making all mentions of IEventBus, ICacheService, and ISecretProvider consistently indicate Phase 02a (not 02b); update the sentence at the current Phase 02b reference so it reads that these implementations ship in Phase 02a and ensure any surrounding text referencing phases (the earlier section that lists foundation building blocks and the line that currently says Phase 02b) match this single source of truth for IEventBus, ICacheService, and ISecretProvider.
🧹 Nitpick comments (1)
.claude/skills/add-mediatr-handler/SKILL.md (1)
253-255: ⚡ Quick winClarify
Result.Failmapping boundary (controller vs pipeline).This wording implies the pipeline performs RFC7807 mapping, but
Result<T>.ToActionResult()is an API/controller mapping step. Tightening this sentence avoids implementation drift.Proposed wording tweak
- The pipeline maps `Result.Fail` to RFC 7807 Problem Details automatically via- `Result<T>.ToActionResult()`.+ The API/controller layer maps `Result.Fail` to RFC 7807 Problem Details via+ `Result<T>.ToActionResult()`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/add-mediatr-handler/SKILL.md around lines 253 - 255, The sentence currently implies the pipeline maps Result.Fail to RFC7807, but the mapping is actually performed at the controller/API layer via Result<T>.ToActionResult(); update the wording in SKILL.md to state that Result.Fail values are converted to RFC7807 Problem Details when controllers call Result<T>.ToActionResult(), and reference ADR-0032 § Sub-decision 4 for policy rather than claiming the pipeline performs the mapping; locate the sentence referencing Result.Fail and Result<T>.ToActionResult() and rephrase to clearly separate pipeline behavior from controller/API mapping.
🤖 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/skills/add-provider-adapter/SKILL.md:
- Around line 30-40: The fenced code block in SKILL.md is missing a language
tag; update the opening fence from ``` to ```text (or another appropriate
language) so the block containing ResilientProviderAdapter<TPort>, LiveKitClient
and ProviderException is fenced as ```text ... ``` to satisfy MD040 and your
docs CI.
In `@docs/architecture/33-cross-cutting-concerns.md`:
- Around line 76-103: The two unlabeled fenced code blocks containing the
request/behavior flow (the block starting with "Request ▼ [1] ValidationBehavior
... [8] Handler") and the OTel notes block must be updated to include a language
marker (e.g., ```text or ```mermaid) to satisfy MD040; edit the fenced blocks
that contain those exact snippets and replace the opening ``` with ```text (or
convert to ```mermaid if you want rendering), and also apply the same fix to the
other unlabeled block referenced (the one around lines 206-219) so all
diagrammatic/code fences in this document have explicit language markers.
In `@docs/decisions/0032-exception-handling-logging-and-observability.md`:
- Around line 155-165: The unlabeled fenced code block that contains the
pipeline diagram starting with "Request → ValidationBehavior" must be marked
with an explicit language tag; update that fenced block to begin with ```text
(or ```mermaid if you prefer to render it) so the markdown linter (MD040) and
repository guidelines for diagrams are satisfied.
In `@docs/roadmap/phase-02b-events-auth.md`:
- Around line 45-46: Clarify that the outbox's correlation_id is not a free-form
UUID but must be stored (or converted) into a full W3C traceparent header value;
update the docs around Activity, traceparent and correlation_id to state whether
the outbox row contains the complete traceparent string
(version-trace-id-parent-id-trace-flags) or, if it only stores a correlation_id,
describe the exact deterministic mapping/serialization rule to construct a valid
traceparent (how to derive trace-id, parent-id, version and trace-flags from
correlation_id) so consumers can set Activity.Traceparent correctly and preserve
end-to-end trace continuity.
In `@infra/dapr/README.md`:
- Line 16: The roadmap link in infra/dapr/README.md currently points to
phase-02b-events-outbox-identity.md but the canonical file in this PR is
phase-02b-events-auth.md; update the link target in the README (the line
containing
"[phase-02b](../../docs/roadmap/phase-02b-events-outbox-identity.md)") to
reference ../../docs/roadmap/phase-02b-events-auth.md so the link points to the
renamed file.
---
Outside diff comments:
In `@infra/dapr/README.md`:
- Around line 119-120: Resolve the conflicting phase assignment for the Dapr
interfaces by making all mentions of IEventBus, ICacheService, and
ISecretProvider consistently indicate Phase 02a (not 02b); update the sentence
at the current Phase 02b reference so it reads that these implementations ship
in Phase 02a and ensure any surrounding text referencing phases (the earlier
section that lists foundation building blocks and the line that currently says
Phase 02b) match this single source of truth for IEventBus, ICacheService, and
ISecretProvider.
---
Nitpick comments:
In @.claude/skills/add-mediatr-handler/SKILL.md:
- Around line 253-255: The sentence currently implies the pipeline maps
Result.Fail to RFC7807, but the mapping is actually performed at the
controller/API layer via Result<T>.ToActionResult(); update the wording in
SKILL.md to state that Result.Fail values are converted to RFC7807 Problem
Details when controllers call Result<T>.ToActionResult(), and reference ADR-0032
§ Sub-decision 4 for policy rather than claiming the pipeline performs the
mapping; locate the sentence referencing Result.Fail and
Result<T>.ToActionResult() and rephrase to clearly separate pipeline behavior
from controller/API mapping.
🪄 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
Run ID: 96a3fcb5-6b65-4365-aa12-8e939c9ac56f
📒 Files selected for processing (20)
.claude/skills/README.md.claude/skills/add-mediatr-handler/SKILL.md.claude/skills/add-provider-adapter/SKILL.md.claude/skills/wire-cross-cutting-foundation/SKILL.mdCLAUDE.mddocs/architecture/33-cross-cutting-concerns.mddocs/decisions/0032-exception-handling-logging-and-observability.mddocs/decisions/README.mddocs/glossary.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/roadmap/phase-02b-events-auth.mddocs/standards/02-backend-coding.mddocs/standards/09-error-handling.mddocs/standards/10-observability.mddocs/standards/12-infrastructure.mddocs/standards/20-infrastructure-stack.mddocs/standards/21-architecture-tests-catalogue.mddocs/standards/README.mdinfra/coturn/turnserver.confinfra/dapr/README.md
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…s + dapr README Walked each review finding against current code; all seven were still valid, all minimal scope, all applied. Inline comments --------------- - add-provider-adapter L30-40: ASCII flow diagram fence had no language tag. Added ```text. - 33-cross-cutting-concerns L76 + L206: two ASCII-flow fences without language tags. Added ```text to both. - ADR-0032 L155: pipeline diagram fence without language tag. Added ```text. - phase-02b L41-46: clarified that outbox `correlation_id` is the **full W3C traceparent string** (`00-<32-hex>-<16-hex>-<2-hex>`), not a bare UUID. Consumer rehydrates via `ActivityContext.TryParse(...)` + `_activitySource.StartActivity(name, kind, parentCtx)`. Same clarification mirrored into ADR-0032 § Sub-decision 12 so the binding contract and the roadmap deliverable agree on the wire format. - infra/dapr/README.md L16: link target was `phase-02b-events-outbox-identity.md` (stale name from an earlier rename). Updated to `phase-02b-events-auth.md`. Outside-diff comments --------------------- - infra/dapr/README.md "What does NOT live here" listed the Dapr interface implementations under Phase 02b, contradicting § Phase ownership above which (correctly) places them in Phase 02a. Rewrote the bullet so the document speaks with one voice: interfaces + adapters ship in 02a; only the outbox dispatch path (the sanctioned caller of `IEventBus.PublishAsync`) is 02b. Nitpick ------- - add-mediatr-handler Common-pitfalls: previous wording implied "the pipeline maps Result.Fail to RFC 7807 automatically via Result<T>.ToActionResult()". Mapping actually happens at the controller/API boundary when the endpoint explicitly calls `.ToActionResult()` (Step 7 above) — the pipeline just propagates Result unchanged. Rephrased to separate pipeline behaviour from controller mapping. Ref: PR #2 review batch (2026-05-19/20). Co-Authored-By: Claude Opus 4.7 (1M context) <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 freeto 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.
Summary
handling, logging, and observability before Phase 02a opens — 13
sub-decisions, no code (repo is pre-implementation).
exception-catch gap, plus 11 previously-unwritten implementation
details (
IExceptionHandlerchoice, ValidationBehavior throw-vs-return,DomainExceptionsemantics, Polly v8IProviderResilience<TPort>,Result<T>.ToActionResult()mapping, Sentry-vs-OTel boundary, Serilogprimary + OTLP sink,
TenantContextSpanProcessorviaITenantContextAccessor,IErrorTrackingProviderdeployment-modebranching, Hub
/api/internal/*traceparentpropagation, outbox +Hangfire correlation columns, frontend Sentry correlation handoff).
wire-cross-cutting-foundation,add-provider-adapter)plus
add-mediatr-handlerupdate so Phase 02a engineers don'tre-derive the contract.
Scope
docs/decisions/0032-exception-handling-logging-and-observability.md,docs/architecture/33-cross-cutting-concerns.md,.claude/skills/wire-cross-cutting-foundation/SKILL.md,.claude/skills/add-provider-adapter/SKILL.md.alignment): Standards 02 / 09 / 10 / 20, roadmap phase-02a + phase-02b,
glossary, decisions/README, CLAUDE.md, skills/README,
add-mediatr-handler skill, plus a leftover Dapr README polish (f391b4f)
that was sitting on the branch from PR feat(infra): Phase 01 packets 4-6 + ADRs 0029-0031 backend swap #1's third review round.
Result<T>+Errorrecords exist as live code underbackend/src/LearnStack.SharedKernel/Results/); ADR-0032 lands ascontract for Phase 02a / 02b to implement.
Test plan
Pure-documentation PR. Verification done before this PR was opened:
every relative target resolves.
Decision Drivers / Considered Options (3 rejected alternatives with
concrete "why rejected") / Decision / Context / Consequences
(Positive / Negative / Neutral) / Implementation Notes / References.
ADR-0016 / ADR-0032 / Standards 02.
amended; ADR-0032 cites and extends.
docs/analysis/residual scan — clean (only the hard-rulereference in CLAUDE.md, which is the rule itself).
decisions/README.mdactive-ADR table includes row 0032; reserveddraft slots 0023-0028 untouched.
Subject_Constraintconvention (
MediatR_Pipeline_Order_Matches_Canonical_Sequence, …) —no ADR number baked into a test identifier.
one-line definitions linking back to ADR-0032 / Standards 09 / 10.
intentional skip noted in commit
d6cdba8).To verify after merge (Phase 02a)
LearnStackExceptionHandler : IExceptionHandlerships with theShouldCapture(ex)switch behaviour matching the partition table.8-step list; architecture test
MediatR_Pipeline_Order_Matches_Canonical_Sequenceis green and non-skippable.
ITenantContextAccessor(singleton,AsyncLocal<ITenantContext?>-backed)ships in
LearnStack.SharedKernel; populated at scope start by everysurface listed in ADR-0032 § Sub-decision 10.
LearnStackException-DomainExceptionThrowisreferenced by
Domain+Applicationprojects (Warning inPhase 02a, Error after Phase 03 exit).
IErrorTrackingProvidercomposition-root branching emits theright implementation per
DeploymentMode;LocalFileErrorTrackeris used inSelfHostedAirGapped.Notes for reviewers
one place so the standards and roadmap can cite it instead of restating
the rules. The Considered Options section explains why a single ADR
(Option A) was preferred over the alternatives (multi-ADR split,
amending ADR-0016, deferring to Phase 02a code review).
f391b4f) is included herebecause it never made it onto the merged PR feat(infra): Phase 01 packets 4-6 + ADRs 0029-0031 backend swap #1; it's a Dapr README
clarity polish unrelated to ADR-0032 but small enough to ride along.
cross-cutting concern; Phase 02b references it for outbox / Hangfire
correlation columns. No code change in either phase from this PR.
🤖 Generated with Claude Code
Summary by Sourcery
Document the cross-cutting backend contract for exception handling, logging, and observability and align existing standards, roadmap, glossary, skills, and infra docs with ADR-0032, without changing any runtime code.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit