feat(phase-02a): packet 3 — cross-cutting foundation (ADR-0032) - #8
Merged
Merged
Conversation
Wires the ADR-0032 surface end to end so every later module inherits the same error / logging / observability contract on Day 1: - L1 LearnStackExceptionHandler + ShouldCapture switch (Sentry-vs-OTel boundary per Standards 09); ProblemDetailsFactory + HttpStatusMap + ResultExtensions.ToActionResult lit up. - LearnStackException hierarchy (DomainException, InfrastructureException, ProviderException with IsClientError, TenantContextMissingException) in SharedKernel/Errors/. - Eight-step MediatR pipeline in Application/Pipeline/ — Validation + Logging are full impls; AuditLog ships the try/ExceptionDispatchInfo rethrow shell; TenantContext short-circuits on unresolved context; Authorization / Transaction / OutboxFlush pass-through shells. Registration order encoded in CanonicalBehaviorOrder + asserted by MediatR_Pipeline_Order_Matches_Canonical_Sequence. - New Infrastructure projects: Observability (TenantContextAccessor + TenantContextSpanProcessor), ErrorTracking (NoOp / Sentry / LocalFile trackers + DeploymentMode-aware AddLearnStackErrorTracking), Resilience (Polly v8 IProviderResilience socket with retry → breaker → timeout + the configuration shape Resilience:<portName>:). - LearnStack.Analyzers Roslyn analyzer flags `throw new DomainException(...)` in Domain + Application (warning; escalates to error after Phase 03 exit). Referenced via OutputItemType ="Analyzer" project references; ships release-tracking markdown. - Program.cs rewires through AddLearnStackCrossCuttingFoundation — Serilog primary logger + WriteTo.OpenTelemetry sink (no AddOpenTelemetry().WithLogging() per Sub-decision 8), OTel SDK with AspNetCore + HttpClient + EFCore instrumentation + TenantContextSpanProcessor + OTLP exporter, IErrorTrackingProvider branched on DeploymentMode, MediatR pipeline + AddExceptionHandler. - Architecture tests: pipeline order, IExceptionHandler registered, OTel processor wired, logging via MEL, Sentry not referenced from modules, adapter SDK exceptions stay in their namespace, IErrorTrackingProvider singleton (110 unit + 24 architecture + 1 contract + 1 integration green under CI=true). - Roadmap (docs/roadmap/phase-02a-kernel-tenancy.md) marks Packet 3 ✅ with the full deliverables list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the full review-1 findings from the cross-cutting foundation: Blockers - B1 Sentry DSN reads via the new ISecretProvider socket (SharedKernel/Secrets/) with ConfigurationSecretProvider as the Phase 02a default — Packet 5 swaps in DaprSecretProvider for Vault. Modules never read DSN from IConfiguration directly. (ADR-0032 § Sub-decision 9) - B2 Serilog pipeline gains CorrelationContextEnricher + RedactSensitiveFieldsEnricher (password / token / secret / DSN / JWT / authorization / SSN / TCKN / card-number / IBAN / CVV); LocalFileErrorTracker redacts the same set on AdditionalTags. Stack traces remain (modules must not put secrets in exception messages — Standards 11). Major - M1 ProblemDetailsFactory.For(Exception) routes status via HttpStatusMap.For(Exception) so ProviderException(IsClientError:true) maps to 400 instead of falling through to 503. - M2 L1 handler skips Activity.AddException for client-side ProviderException — SetStatus(Error) only, no exception event. Match the Standards 09 § Sentry vs OpenTelemetry table. - M3 All 14 module Domain + Application csproj files reference LearnStack.Analyzers via OutputItemType="Analyzer"; future `throw new DomainException(...)` in any module trips the analyzer. - M4 Polly bulkhead lit up via Polly.RateLimiting's AddRateLimiter(ConcurrencyLimiterOptions); BulkheadOptions is no longer dead config. Minor - N1 ProblemDetailsFactory.ToCamelCase handles nested paths (Address.Street → address.street) and acronyms (URLValue → urlValue) via JsonNamingPolicy.CamelCase. - N2 ToActionResult() returns ProblemDetailsActionResult that builds the body lazily inside ExecuteResultAsync — Instance + correlationId now populate from HttpContext without the controller threading it. - N3 / A6 LocalFileErrorTracker file names suffix a Guid for unique filenames in same-ms bursts; stackalloc capped at 128 chars so a multi-KB traceparent header cannot blow the stack. - A5 AuditLogBehavior catch filter excludes OperationCanceledException so client disconnects no longer churn warning logs / future audit rows. - A7 MediatRPipelineRegistration.CanonicalBehaviorOrder doc clarifies "7 behaviors + the handler = the 8 canonical steps". - A8 ProblemDetailsFactory Type URL trims the _failed suffix to match the Standards 09 § API Surface example. - A11 New WebApplicationFactory<Program>-based integration tests (LearnStack.Tests.Integration/CrossCuttingFoundationHttpTests) exercise the L1 handler + ValidationBehavior end-to-end via a synthetic test controller. Standards 21 catalogue updated to "unit + integration". - A12 LearnStackExceptionHandler is internal sealed (framework's AddExceptionHandler<T>() instantiates it; tests reach via InternalsVisibleTo). - A13 TenantContextSpanProcessor stringifies Guid tags so wire format is stable across exporters. Suggestions - S1 MediatR_Pipeline_Order_Matches_Canonical_Sequence test asserts a hardcoded behavior sequence; the production list reorder cannot sneak past. - SU1 L1 handler skips body write on cancellation — the client has already disconnected. - SU4 TenantContextBehavior.AllowsUnresolvedContext TODO documents the Packet 7 marker-attribute seam ([AllowsUnresolvedTenantContext]). New architecture test - Modules_Do_Not_Reference_DeploymentMode — catalogue entry existed since ADR-0020 but had no implementation until now. Validation - dotnet build LearnStack.slnx (CI=true) → 0 warning, 0 error. - 142/142 tests green: Unit 111, Architecture 25, Integration 5, Contract 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…token catalog
Addresses the review-2 follow-ups raised on the review-1 commit:
Minor
- N4 SelectSecretProvider helper in CrossCuttingFoundationExtensions
becomes the single composition-root site that picks the
ISecretProvider implementation per DeploymentMode. Both the DI
registration and the local AddLearnStackErrorTracking call read the
same instance. Packet 5's DaprSecretProvider swap now touches one
line, not two — TODO comment anchors the per-mode branches.
Suggestions
- SU5 SensitiveTokenCatalog in SharedKernel/Secrets/ becomes the single
source of truth for the sensitive-property-name token list. Both
RedactSensitiveFieldsEnricher and LocalFileErrorTracker.RedactSensitiveTags
consume SensitiveTokenCatalog.IsSensitive(...), so the Serilog path
and the air-gapped path cannot drift. The catalogue now includes
`vkn` (Vergi Kimlik Numarası — Turkish corporate tax number) next to
`tckn`.
- SU6 RedactSensitiveFieldsEnricher remarks carry a dated TODO naming
the Packet 7+ Roslyn analyzer that should extend LearnStack.Analyzers
to flag string-interpolated `throw new ...Exception($"...{token}...")`
patterns in Domain + Application projects. Runtime redaction covers
logs / OTLP / Sentry tags; the analyzer closes the secrets-in-
exception-messages gap at compile time.
- SU7 HttpStatusMap.For(Exception) carries a rationale comment block
explaining the non-IETF 499 "client closed request" status: matches
Nginx / IIS / Envoy / APISIX behaviour, keeps client disconnects off
the error-budget axis, points at the L1 handler's skip-body
contract. If a future ADR pins a different code, the comment block
is the one seam to change.
Validation
- dotnet build LearnStack.slnx (CI=true) → 0 warning, 0 error.
- 142/142 tests green: Unit 111, Architecture 25, Integration 5,
Contract 1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds shared error, tenancy, resilience, observability, and secret contracts. Adds API Problem Details handling, MediatR behaviors, infrastructure providers, a Roslyn analyzer, composition-root wiring, tests, package updates, and documentation. ChangesCross-cutting foundation
Estimated code review effort: 4 (Complex) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LearnStackApi
participant MediatR
participant LearnStackExceptionHandler
participant ProblemDetailsFactory
participant IErrorTrackingProvider
Client->>LearnStackApi: HTTP request
LearnStackApi->>MediatR: Execute request pipeline
MediatR-->>LearnStackApi: Result or exception
LearnStackApi->>LearnStackExceptionHandler: Handle exception
LearnStackExceptionHandler->>IErrorTrackingProvider: Capture exception when eligible
LearnStackExceptionHandler->>ProblemDetailsFactory: Build ProblemDetails
ProblemDetailsFactory-->>Client: application/problem+json response
Possibly related PRs
Estimated code review effort: 4 (Complex) | ~90 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the cross-cutting foundation for the LearnStack backend, covering exception handling, logging, observability, and resilience as per ADR-0032. Key additions include a centralized L1 exception handler, a canonical eight-step MediatR pipeline with behaviors for validation and logging, and infrastructure for error tracking (Sentry/LocalFile) and provider resilience (Polly v8). A new Roslyn analyzer was introduced to enforce the use of result types over domain exceptions. Feedback focuses on ensuring correlation IDs consistently use the full W3C traceparent format as documented and providing appropriate fallbacks to the trace identifier when the activity context is missing.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
backend/src/LearnStack.Infrastructure.Observability/Serilog/RedactSensitiveFieldsEnricher.cs (1)
51-58: ⚡ Quick winAvoid per-event array allocation in the enricher hot path.
Line 51 allocates a new array for every log event even when no sensitive keys exist. A two-pass lazy list avoids that steady-state allocation.
Proposed refactor
- foreach (var propertyName in logEvent.Properties.Keys.ToArray()) - { - if (SensitiveTokenCatalog.IsSensitive(propertyName)) - { - logEvent.AddOrUpdateProperty( - propertyFactory.CreateProperty(propertyName, RedactedValue)); - } - } + List<string>? sensitiveKeys = null; + foreach (var propertyName in logEvent.Properties.Keys) + { + if (!SensitiveTokenCatalog.IsSensitive(propertyName)) + { + continue; + } + + sensitiveKeys ??= new List<string>(); + sensitiveKeys.Add(propertyName); + } + + if (sensitiveKeys is null) + { + return; + } + + foreach (var propertyName in sensitiveKeys) + { + logEvent.AddOrUpdateProperty( + propertyFactory.CreateProperty(propertyName, RedactedValue)); + }🤖 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 `@backend/src/LearnStack.Infrastructure.Observability/Serilog/RedactSensitiveFieldsEnricher.cs` around lines 51 - 58, The current loop in RedactSensitiveFieldsEnricher uses logEvent.Properties.Keys.ToArray(), causing a new array allocation per event; change it to a two-pass lazy approach in the enricher (e.g., Enrich method): iterate logEvent.Properties.Keys without ToArray and, instead of allocating up-front, only allocate a List<string> when you encounter the first SensitiveTokenCatalog.IsSensitive(propertyName); append subsequent sensitive keys to that list, and after the loop call logEvent.AddOrUpdateProperty for each collected key using propertyFactory.CreateProperty(propertyName, RedactedValue). This avoids the steady-state ToArray allocation while still being safe when mutating properties.
🤖 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 `@backend/src/LearnStack.Api/Common/LearnStackExceptionHandler.cs`:
- Around line 59-64: Wrap the call to errorTracker.CaptureAsync(...) inside a
try/catch within TryHandleAsync so any exception from the error-tracking
provider cannot abort the handler; catch exceptions around BuildCapturedContext
+ errorTracker.CaptureAsync and LogCaptured, log the capture failure with the
logger (including the capture exception and context), but do not rethrow so the
subsequent ProblemDetails response writing always runs; this protects calls to
IErrorTrackingProvider.CaptureAsync and ensures TryHandleAsync completes even if
capture fails.
In `@backend/src/LearnStack.Api/Composition/CrossCuttingFoundationExtensions.cs`:
- Line 181: The TODO comment in CrossCuttingFoundationExtensions.cs uses an
extra token in the parentheses ("// TODO(2026-05-21, `@platform`,
phase-02a-packet-5): ...") which violates the repository rule; update the
comment to the required format by removing the extra token from the parentheses
and placing any extra context after the colon, e.g. change it to "//
TODO(2026-05-21, `@platform`): light up the phase-02a-packet-5" so the TODO
follows the exact "(YYYY-MM-DD, `@owner`): description" pattern.
In `@backend/src/LearnStack.Application/Pipeline/AuditLogBehavior.cs`:
- Line 57: The TODO comments in AuditLogBehavior.cs (inside the AuditLogBehavior
class) use a three-token tuple like "// TODO(2026-05-21, `@platform`,
phase-02a-packet-9): ..." which violates the repo rule; update both TODO
occurrences (the one at the current location and the one around line 74) to the
canonical format "// TODO(YYYY-MM-DD, `@owner`): description" by removing the
extra token and keeping only the date and owner, e.g. "// TODO(2026-05-21,
`@platform`): <description>" so they conform to the required pattern.
In `@backend/src/LearnStack.Application/Pipeline/AuthorizationBehavior.cs`:
- Around line 34-37: The TODO comment in AuthorizationBehavior.cs uses an
invalid signature with an extra metadata token `phase-03`; update the TODO to
follow the required format and move phase info into the body text: change the
comment to use the exact signature pattern // TODO(YYYY-MM-DD, `@owner`): ...
(e.g., // TODO(2026-05-21, `@platform`): resolve the request's [Authorize(Policy)]
attribute — phase-03: call IAuthorizationService.AuthorizeAsync with the
tenant+organization-scoped resource and return
Result.FailFor<TResponse>(forbidden) on deny), ensuring the class/method context
(AuthorizationBehavior) still clearly documents the intended work.
In `@backend/src/LearnStack.Application/Pipeline/OutboxFlushBehavior.cs`:
- Around line 31-34: The TODO comment in OutboxFlushBehavior.cs uses a
non-standard signature containing phase metadata; update it to the
repository-standard format TODO(YYYY-MM-DD, `@owner`): description (e.g.,
TODO(2026-05-21, `@platform`): ...) and remove the phase token from the signature,
moving the phase info into the descriptive text of the comment so the TODO
header only contains the date and owner while the body keeps "phase-02b"
context.
In `@backend/src/LearnStack.Application/Pipeline/TenantContextBehavior.cs`:
- Around line 46-50: Update the TODO comment signatures to the repository
standard by removing extra parameters so they match the format //
TODO(YYYY-MM-DD, `@owner`): description; specifically replace occurrences like
TODO(2026-05-21, `@platform`, phase-02a-packet-7) with TODO(2026-05-21,
`@platform`): and move the "phase-02a-packet-7" / packet/phase references into the
comment body text; apply the same normalization to the other TODO block around
lines 63-70 so both TODO comments in TenantContextBehavior.cs follow the single
date+owner signature and keep the phase/packet details only in the descriptive
text.
In `@backend/src/LearnStack.Application/Pipeline/TransactionBehavior.cs`:
- Around line 34-38: Update the TODO comment in the TransactionBehavior class so
its signature matches the required format: keep only the date and owner inside
the parentheses (e.g., // TODO(2026-05-21, `@platform`): ...) and remove any extra
fields; locate the TODO in the TransactionBehavior.cs file (the comment above
the unit-of-work transaction handling notes) and rewrite it to follow the exact
pattern // TODO(YYYY-MM-DD, `@owner`): description.
In
`@backend/src/LearnStack.Infrastructure.ErrorTracking/ErrorTrackingRegistration.cs`:
- Around line 98-103: The SentrySdk.Init call sets o.TracesSampleRate directly
from options.TracesSampleRate which can throw if outside 0..1; before calling
SentrySdk.Init (in ErrorTrackingRegistration) validate options.TracesSampleRate
is within [0,1] and only assign it when valid (or clamp to the range / skip
setting and log a warning) so that SentrySdk.Init is never passed an
out-of-range value for o.TracesSampleRate.
In `@backend/src/LearnStack.Infrastructure.ErrorTracking/NoOpErrorTracker.cs`:
- Around line 14-17: The NoOpErrorTracker's CaptureAsync method currently
accepts null exception or context silently; update NoOpErrorTracker.CaptureAsync
to perform the same null-guard checks as other IErrorTrackingProvider
implementations by throwing ArgumentNullException for a null exception and/or
null context (and preserve the ValueTask return), so callers get consistent
development-time validation.
In `@backend/src/LearnStack.Infrastructure.ErrorTracking/SentryErrorTracker.cs`:
- Around line 64-69: The code in SentryErrorTracker forwards
context.AdditionalTags verbatim to scope.SetTag, risking leaking secrets/PII;
update SentryErrorTracker to sanitize/redact those tags before calling
scope.SetTag by implementing a helper (e.g., RedactTagValue or
SanitizeAdditionalTags) that either skips known-sensitive keys (password,
secret, token, api_key, ssn, pii, private, key, credential) or masks their
values (e.g., replace with "***REDACTED***" or partial masking) and then iterate
the sanitized map instead of context.AdditionalTags when calling scope.SetTag.
In
`@backend/src/LearnStack.Infrastructure.Observability/Serilog/RedactSensitiveFieldsEnricher.cs`:
- Around line 32-38: Update the TODO comment metadata to the canonical format so
the parser recognizes it: replace the existing TODO signature that includes
extra tokens (e.g. "2026-05-21, `@platform`, phase-02b-or-later") with the
enforced form // TODO(YYYY-MM-DD, `@owner`): description; specifically edit the
TODO near the top of RedactSensitiveFieldsEnricher (the comment mentioning the
Roslyn analyzer) to use only a date and single `@owner` and keep the rest of the
description unchanged.
In `@backend/src/LearnStack.SharedKernel/Errors/DomainException.cs`:
- Around line 22-23: The DefaultError for the DomainException currently uses the
business-rule code "lockey_business_rule_violation" which misclassifies
programmer/invariant bugs; update DefaultError in the DomainException class to
use an internal/programmer error code (e.g., "lockey_internal_error" or
"lockey_programmer_error") via LocalizedMessage so thrown DomainException
clearly represents developer/invariant failures rather than expected
business-rule violations, and ensure any mappings that rely on DomainException
semantics treat this new code as an internal/server error.
In `@backend/src/LearnStack.SharedKernel/Hosting/DeploymentMode.cs`:
- Around line 19-23: The DeploymentMode enum currently defines extra variants
(Development, SelfHostedOnline, SelfHostedAirGapped) which violate the platform
contract; update the enum DeploymentMode to only include the three allowed
values SaaS, Dedicated, and SelfHosted, remove the other members, and refactor
any code referencing the removed members to use SelfHosted (or adjust
composition-root mapping) so all branching on mode happens only at the
composition root and module code uses the normalized DeploymentMode values.
In `@backend/src/LearnStack.SharedKernel/Observability/IErrorTrackingProvider.cs`:
- Around line 38-40: The CapturedContext record currently exposes TenantId,
OrganizationId and UserId as Guid? primitives; change these to the project’s
strongly-typed id value objects (e.g. TenantId, OrganizationId, UserId)
preserving nullability if required (TenantId?, OrganizationId?, UserId?) and
update the IErrorTrackingProvider/CapturedContext definition and any callers
(serialization, mapping, tests, and usages in methods that create
CapturedContext) to construct and accept the strongly-typed ids instead of Guid
values so the public contract uses the domain id types rather than Guid
primitives.
In `@backend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.cs`:
- Around line 35-43: Change the ITenantContext contract to expose strongly-typed
ID value objects instead of raw Guid primitives: replace the TenantId property
type Guid with your TenantId value-object type and replace OrganizationId Guid?
with the nullable OrganizationId value-object type; update the interface symbol
ITenantContext accordingly and add the necessary using/namespace for those value
objects, then update all implementing classes and consumers to return/accept the
new TenantId and OrganizationId types (preserving nullability for
organization-scoped requests).
In `@backend/tests/LearnStack.Tests.Architecture/CrossCuttingFoundationTests.cs`:
- Around line 266-279: The test IErrorTrackingProvider_Is_Singleton only checks
there is one registration but not that the same instance is returned on multiple
resolves; change it to resolve the provider twice and assert they are the same
instance. In the test built by BuildMinimalApiHost(), call
application.Services.GetRequiredService<IErrorTrackingProvider>() twice (and
additionally resolve once from a new scope using
application.Services.CreateScope().ServiceProvider.GetRequiredService<IErrorTrackingProvider>()
to verify cross-scope singleton behavior) and assert object/reference equality
(e.g. Assert.Same or FluentAssertions .BeSameAs) rather than just counting
registrations.
---
Nitpick comments:
In
`@backend/src/LearnStack.Infrastructure.Observability/Serilog/RedactSensitiveFieldsEnricher.cs`:
- Around line 51-58: The current loop in RedactSensitiveFieldsEnricher uses
logEvent.Properties.Keys.ToArray(), causing a new array allocation per event;
change it to a two-pass lazy approach in the enricher (e.g., Enrich method):
iterate logEvent.Properties.Keys without ToArray and, instead of allocating
up-front, only allocate a List<string> when you encounter the first
SensitiveTokenCatalog.IsSensitive(propertyName); append subsequent sensitive
keys to that list, and after the loop call logEvent.AddOrUpdateProperty for each
collected key using propertyFactory.CreateProperty(propertyName, RedactedValue).
This avoids the steady-state ToArray allocation while still being safe when
mutating properties.
🪄 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: b5477076-7159-4ecb-9d30-397f85087018
📒 Files selected for processing (92)
backend/Directory.Build.propsbackend/Directory.Packages.propsbackend/LearnStack.slnxbackend/analyzers/LearnStack.Analyzers/AnalyzerReleases.Shipped.mdbackend/analyzers/LearnStack.Analyzers/AnalyzerReleases.Unshipped.mdbackend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.csbackend/analyzers/LearnStack.Analyzers/LearnStack.Analyzers.csprojbackend/src/LearnStack.Api/Common/HttpStatusMap.csbackend/src/LearnStack.Api/Common/LearnStackExceptionHandler.csbackend/src/LearnStack.Api/Common/ProblemDetailsActionResult.csbackend/src/LearnStack.Api/Common/ProblemDetailsFactory.csbackend/src/LearnStack.Api/Common/ResultExtensions.csbackend/src/LearnStack.Api/Composition/CrossCuttingFoundationExtensions.csbackend/src/LearnStack.Api/LearnStack.Api.csprojbackend/src/LearnStack.Api/Program.csbackend/src/LearnStack.Api/Properties/AssemblyInfo.csbackend/src/LearnStack.Api/appsettings.jsonbackend/src/LearnStack.Application/LearnStack.Application.csprojbackend/src/LearnStack.Application/Pipeline/AuditLogBehavior.csbackend/src/LearnStack.Application/Pipeline/AuthorizationBehavior.csbackend/src/LearnStack.Application/Pipeline/LoggingBehavior.csbackend/src/LearnStack.Application/Pipeline/MediatRPipelineRegistration.csbackend/src/LearnStack.Application/Pipeline/OutboxFlushBehavior.csbackend/src/LearnStack.Application/Pipeline/TenantContextBehavior.csbackend/src/LearnStack.Application/Pipeline/TransactionBehavior.csbackend/src/LearnStack.Application/Pipeline/ValidationBehavior.csbackend/src/LearnStack.Domain/LearnStack.Domain.csprojbackend/src/LearnStack.Infrastructure.ErrorTracking/AssemblyMarker.csbackend/src/LearnStack.Infrastructure.ErrorTracking/ErrorTrackingOptions.csbackend/src/LearnStack.Infrastructure.ErrorTracking/ErrorTrackingRegistration.csbackend/src/LearnStack.Infrastructure.ErrorTracking/LearnStack.Infrastructure.ErrorTracking.csprojbackend/src/LearnStack.Infrastructure.ErrorTracking/LocalFileErrorTracker.csbackend/src/LearnStack.Infrastructure.ErrorTracking/NoOpErrorTracker.csbackend/src/LearnStack.Infrastructure.ErrorTracking/Properties/AssemblyInfo.csbackend/src/LearnStack.Infrastructure.ErrorTracking/SentryErrorTracker.csbackend/src/LearnStack.Infrastructure.Observability/AssemblyMarker.csbackend/src/LearnStack.Infrastructure.Observability/LearnStack.Infrastructure.Observability.csprojbackend/src/LearnStack.Infrastructure.Observability/ObservabilityRegistration.csbackend/src/LearnStack.Infrastructure.Observability/Serilog/CorrelationContextEnricher.csbackend/src/LearnStack.Infrastructure.Observability/Serilog/RedactSensitiveFieldsEnricher.csbackend/src/LearnStack.Infrastructure.Observability/TenantContextAccessor.csbackend/src/LearnStack.Infrastructure.Observability/TenantContextSpanProcessor.csbackend/src/LearnStack.Infrastructure.Resilience/AssemblyMarker.csbackend/src/LearnStack.Infrastructure.Resilience/LearnStack.Infrastructure.Resilience.csprojbackend/src/LearnStack.Infrastructure.Resilience/Properties/AssemblyInfo.csbackend/src/LearnStack.Infrastructure.Resilience/ProviderResilience.csbackend/src/LearnStack.Infrastructure.Resilience/ProviderResilienceRegistration.csbackend/src/LearnStack.SharedKernel/Errors/DomainException.csbackend/src/LearnStack.SharedKernel/Errors/InfrastructureException.csbackend/src/LearnStack.SharedKernel/Errors/LearnStackException.csbackend/src/LearnStack.SharedKernel/Errors/ProviderException.csbackend/src/LearnStack.SharedKernel/Errors/TenantContextMissingException.csbackend/src/LearnStack.SharedKernel/Hosting/DeploymentMode.csbackend/src/LearnStack.SharedKernel/LearnStack.SharedKernel.csprojbackend/src/LearnStack.SharedKernel/Observability/IErrorTrackingProvider.csbackend/src/LearnStack.SharedKernel/Resilience/IProviderResilience.csbackend/src/LearnStack.SharedKernel/Resilience/ResilienceOptions.csbackend/src/LearnStack.SharedKernel/Secrets/ConfigurationSecretProvider.csbackend/src/LearnStack.SharedKernel/Secrets/ISecretProvider.csbackend/src/LearnStack.SharedKernel/Secrets/SensitiveTokenCatalog.csbackend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.csbackend/src/LearnStack.SharedKernel/Tenancy/ITenantContextAccessor.csbackend/src/LearnStack.SharedKernel/Tenancy/UnresolvedTenantContext.csbackend/src/Modules/Audit/LearnStack.Modules.Audit.Application/LearnStack.Modules.Audit.Application.csprojbackend/src/Modules/Audit/LearnStack.Modules.Audit.Domain/LearnStack.Modules.Audit.Domain.csprojbackend/src/Modules/Content/LearnStack.Modules.Content.Application/LearnStack.Modules.Content.Application.csprojbackend/src/Modules/Content/LearnStack.Modules.Content.Domain/LearnStack.Modules.Content.Domain.csprojbackend/src/Modules/Customization/LearnStack.Modules.Customization.Application/LearnStack.Modules.Customization.Application.csprojbackend/src/Modules/Customization/LearnStack.Modules.Customization.Domain/LearnStack.Modules.Customization.Domain.csprojbackend/src/Modules/Education/LearnStack.Modules.Education.Application/LearnStack.Modules.Education.Application.csprojbackend/src/Modules/Education/LearnStack.Modules.Education.Domain/LearnStack.Modules.Education.Domain.csprojbackend/src/Modules/Identity/LearnStack.Modules.Identity.Application/LearnStack.Modules.Identity.Application.csprojbackend/src/Modules/Identity/LearnStack.Modules.Identity.Domain/LearnStack.Modules.Identity.Domain.csprojbackend/src/Modules/Media/LearnStack.Modules.Media.Application/LearnStack.Modules.Media.Application.csprojbackend/src/Modules/Media/LearnStack.Modules.Media.Domain/LearnStack.Modules.Media.Domain.csprojbackend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Application/LearnStack.Modules.Tenancy.Application.csprojbackend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Domain/LearnStack.Modules.Tenancy.Domain.csprojbackend/tests/LearnStack.Tests.Architecture/CrossCuttingFoundationTests.csbackend/tests/LearnStack.Tests.Architecture/LearnStack.Tests.Architecture.csprojbackend/tests/LearnStack.Tests.Integration/CrossCuttingFoundationHttpTests.csbackend/tests/LearnStack.Tests.Integration/LearnStack.Tests.Integration.csprojbackend/tests/LearnStack.Tests.Unit/Api/Common/HttpStatusMapTests.csbackend/tests/LearnStack.Tests.Unit/Api/Common/ResultExtensionsTests.csbackend/tests/LearnStack.Tests.Unit/Application/Pipeline/AuditLogBehaviorTests.csbackend/tests/LearnStack.Tests.Unit/Application/Pipeline/TenantContextBehaviorTests.csbackend/tests/LearnStack.Tests.Unit/Application/Pipeline/ValidationBehaviorTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/ErrorTracking/LocalFileErrorTrackerTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/TenantContextSpanProcessorTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/Resilience/ProviderResilienceTests.csbackend/tests/LearnStack.Tests.Unit/LearnStack.Tests.Unit.csprojdocs/roadmap/phase-02a-kernel-tenancy.mddocs/standards/21-architecture-tests-catalogue.md
Comment on lines
+38
to
+40
| Guid? TenantId, | ||
| Guid? OrganizationId, | ||
| Guid? UserId, |
There was a problem hiding this comment.
Use strongly-typed IDs in CapturedContext.
Line 38-Line 40 exposes tenant/organization/user identifiers as Guid?, which weakens the strong-id boundary in a shared public contract and encourages primitive-id propagation across layers.
As per coding guidelines “C# / .NET 10 backend code must use strongly-typed ids, records, MediatR pipeline, and EF Core with per-module DbContext”.
🤖 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 `@backend/src/LearnStack.SharedKernel/Observability/IErrorTrackingProvider.cs`
around lines 38 - 40, The CapturedContext record currently exposes TenantId,
OrganizationId and UserId as Guid? primitives; change these to the project’s
strongly-typed id value objects (e.g. TenantId, OrganizationId, UserId)
preserving nullability if required (TenantId?, OrganizationId?, UserId?) and
update the IErrorTrackingProvider/CapturedContext definition and any callers
(serialization, mapping, tests, and usages in methods that create
CapturedContext) to construct and accept the strongly-typed ids instead of Guid
values so the public contract uses the domain id types rather than Guid
primitives.
Collaborator
Author
There was a problem hiding this comment.
Deferred, on purpose. The strongly-typed TenantId / OrganizationId Vogen value objects don't exist yet — they land with the Tenancy schema in Packet 6/7; today only UserId exists in SharedKernel. ITenantContext itself exposes raw Guid for the same reason (see the sibling thread on ITenantContext.cs), and CapturedContext deliberately mirrors it. Typing only this record — or mixing a typed UserId with two raw Guids — would be an inconsistent half-step. I'll convert both contracts together when the Tenancy IDs land. Tracked as a Packet 6/7 follow-up.
There was a problem hiding this comment.
@cemililik — Understood, the rationale is sound. Mixing a typed UserId with two primitive Guids would be exactly the kind of inconsistency that makes later refactors harder, so waiting for all three Tenancy IDs to land before converting both CapturedContext and ITenantContext in one go is the right call. I'll drop this flag.
Would you like me to open a GitHub issue to track the Packet 6/7 conversion of CapturedContext and ITenantContext to strongly-typed TenantId/OrganizationId/UserId, or do you already have that covered in your internal backlog?
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/LearnStack PR: 8
File: backend/src/LearnStack.SharedKernel/Observability/IErrorTrackingProvider.cs:38-40
Timestamp: 2026-05-22T10:23:57.718Z
Learning: In the LearnStack C# backend, `CapturedContext` (in `backend/src/LearnStack.SharedKernel/Observability/IErrorTrackingProvider.cs`) and `ITenantContext` (in `backend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.cs`) intentionally expose `TenantId` and `OrganizationId` as raw `Guid?` primitives — NOT as Vogen strongly-typed value objects — because those Vogen IDs (`TenantId`, `OrganizationId`) do not exist yet in SharedKernel (they land in Packet 6/7 with the Tenancy schema). Only `UserId` is typed. Both contracts will be converted to strongly-typed IDs together in Packet 6/7 to avoid an inconsistent half-step. Do not flag raw `Guid` usage for these specific fields until after Packet 6/7 lands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cemililik
Repo: cemililik/LearnStack PR: 6
File: backend/src/LearnStack.SharedKernel/Domain/AuditableEntity.cs:57-100
Timestamp: 2026-05-21T10:55:58.760Z
Learning: In the LearnStack C# backend, Vogen v7 emits VOG009 and blocks comparisons like `by == default(UserId)` at compile time. When validating a `UserId` value object at runtime, compare its underlying value instead: use `by.Value == Guid.Empty` (and equivalent checks) rather than `by == default(UserId)`. Apply this pattern in audit/input validation logic such as `AuditableEntity<TId>.EnsureValidAuditInput(DateTimeOffset at, UserId by)` in `backend/src/LearnStack.SharedKernel/Domain/AuditableEntity.cs`.
Collaborator
Author
There was a problem hiding this comment.
No separate issue needed — this is now tracked in the roadmap (our backlog). docs/roadmap/phase-02a-kernel-tenancy.md records it in two places: Packet 6 introduces the TenantId / OrganizationId Vogen value objects alongside the Tenancy schema, and Packet 7 then converts both ITenantContext and CapturedContext from raw Guid to the strongly-typed IDs in a single pass. There's also a consolidated "Deferred follow-ups carried out of Packet 3" ledger in the Packet 3 status block that names this item and its owning packet. Thanks for offering to track it.
Comment on lines
+35
to
+43
| Guid TenantId { get; } | ||
|
|
||
| /// <summary> | ||
| /// The resolved organization within the tenant, when the request targets | ||
| /// an <c>[OrganizationScoped]</c> resource. <c>null</c> for tenant-wide | ||
| /// requests. | ||
| /// </summary> | ||
| Guid? OrganizationId { get; } | ||
|
|
There was a problem hiding this comment.
Use strongly-typed IDs in tenant context contract.
Line 35 and Line 42 expose raw Guid/Guid? instead of strong ID value objects. Since this interface is shared across pipeline/infra layers, keeping primitives here weakens type safety and makes cross-module misuse easier. Please switch these to strong ID types (e.g., tenant/organization ID value objects) at the contract boundary.
As per coding guidelines: backend/**/*.cs: “C# / .NET 10 backend code must use strongly-typed ids...”.
🤖 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 `@backend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.cs` around lines
35 - 43, Change the ITenantContext contract to expose strongly-typed ID value
objects instead of raw Guid primitives: replace the TenantId property type Guid
with your TenantId value-object type and replace OrganizationId Guid? with the
nullable OrganizationId value-object type; update the interface symbol
ITenantContext accordingly and add the necessary using/namespace for those value
objects, then update all implementing classes and consumers to return/accept the
new TenantId and OrganizationId types (preserving nullability for
organization-scoped requests).
Collaborator
Author
There was a problem hiding this comment.
Deferred, same reasoning as the CapturedContext thread: the TenantId / OrganizationId value objects don't exist until Packet 6/7 (only UserId exists in SharedKernel now). ITenantContext uses raw Guid intentionally so the contract can ship in Packet 3 without a forward dependency on types that aren't defined yet. Converting both ITenantContext and CapturedContext in one pass when the Tenancy IDs land avoids a half-typed intermediate state.
There was a problem hiding this comment.
@cemililik, understood — the raw Guid usage is intentional as a forward-dependency guard while the TenantId/OrganizationId value objects are still undefined. The planned single-pass conversion of both ITenantContext and CapturedContext when the Tenancy IDs land in Packet 6/7 is a clean approach. I'll note this for future reviews.
Would you like me to open a tracking issue for the Packet 6/7 conversion pass so it doesn't get lost?
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/LearnStack PR: 8
File: backend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.cs:35-43
Timestamp: 2026-05-22T10:23:58.274Z
Learning: In LearnStack SharedKernel (C# backend), `ITenantContext.TenantId` and `ITenantContext.OrganizationId` (in `backend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.cs`) intentionally use raw `Guid`/`Guid?` in Packet 3 because the `TenantId` and `OrganizationId` strong-typed value objects do not yet exist (only `UserId` exists in SharedKernel). These will be converted to strongly-typed IDs in one pass alongside `CapturedContext` when the Tenancy ID value objects land in Packet 6/7.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cemililik
Repo: cemililik/LearnStack PR: 6
File: backend/src/LearnStack.SharedKernel/Domain/AuditableEntity.cs:57-100
Timestamp: 2026-05-21T10:55:58.760Z
Learning: In the LearnStack C# backend, Vogen v7 emits VOG009 and blocks comparisons like `by == default(UserId)` at compile time. When validating a `UserId` value object at runtime, compare its underlying value instead: use `by.Value == Guid.Empty` (and equivalent checks) rather than `by == default(UserId)`. Apply this pattern in audit/input validation logic such as `AuditableEntity<TId>.EnsureValidAuditInput(DateTimeOffset at, UserId by)` in `backend/src/LearnStack.SharedKernel/Domain/AuditableEntity.cs`.
Collaborator
Author
There was a problem hiding this comment.
Same as the CapturedContext thread — no GitHub issue needed; it's in the roadmap. Packet 7 in docs/roadmap/phase-02a-kernel-tenancy.md now explicitly carries the single-pass conversion of ITenantContext + CapturedContext to strongly-typed TenantId / OrganizationId (the value objects land in Packet 6), and the Packet 3 block's deferred-follow-ups ledger lists it too. Appreciate the nudge to keep it from getting lost.
…semantics Verified each finding against current code; fixed the still-valid ones, skipped the rest with reasons (below). Fixed - L1 handler wraps IErrorTrackingProvider.CaptureAsync in try/catch — a provider failure (Sentry network blip, full disk) can no longer abort TryHandleAsync; the Problem Details response always writes. New LogCaptureFailed event records the swallowed capture error. - correlationId now uses the full W3C traceparent (Activity.Current.Id) in both the L1 handler's CapturedContext and ProblemDetailsFactory, with a fallback chain to ITenantContext.CorrelationId then HttpContext.TraceIdentifier. Matches the ITenantContext.CorrelationId contract and keeps the Problem Details body + Sentry/LocalFile capture on one handle. - ErrorTrackingRegistration clamps Sentry TracesSampleRate to [0,1] (Math.Clamp) — a mis-typed appsettings value no longer crashes startup via Sentry's range-checked setter. - NoOpErrorTracker null-guards exception + context for parity with the Sentry / LocalFile implementations (a contract bug surfaces in dev, not just prod). - SentryErrorTracker redacts AdditionalTags via SensitiveTokenCatalog before scope.SetTag — the same catalog the Serilog enricher + the air-gapped LocalFileErrorTracker share, so all three external-egress surfaces redact identically. - DomainException default Error is now lockey_internal_error (→ 500), not lockey_business_rule_violation (→ 409). A DomainException reaching L1 is a bug, not a refused business operation (ADR-0032 § Sub-decision 4). - IErrorTrackingProvider_Is_Singleton asserts singleton *lifetime* — resolves twice from root + once from a fresh scope and asserts reference equality, not just registration count. - RedactSensitiveFieldsEnricher uses a two-pass lazy approach: the common no-sensitive-property path now allocates nothing (was a per-event ToArray); the List materialises only on the first match. - Normalized 9 TODO comments to the documented (YYYY-MM-DD, @owner) two-token format (CLAUDE.md / Standards 02 § Comments); phase/packet info moved into the description body. Skipped (with reason) - DeploymentMode enum reduction to {SaaS, Dedicated, SelfHosted}: rejected — Standards 20 § Composition Root + ADR-0020 explicitly mandate the 5-value form (Development + SelfHostedOnline + SelfHostedAirGapped); the split is what lets the composition root pick phone-home vs signed-license without runtime branching. The whole ErrorTracking switch + Standards 20 table depend on it. - Strongly-typed IDs in CapturedContext + ITenantContext: deferred — the TenantId / OrganizationId Vogen value objects do not exist yet (they land with the Tenancy schema in Packet 6/7). ITenantContext itself uses raw Guid for the same reason; typing only CapturedContext (or mixing one typed UserId with two raw Guids) would be inconsistent. Revisit when the Tenancy IDs land. Validation - dotnet build LearnStack.slnx (CI=true) → 0 warning, 0 error. - 142/142 tests green: Unit 111, Architecture 25, Integration 5, Contract 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, provider-error consistency
Verified each finding from the two review passes against current code; fixed
the valid ones, skipped the rest with reasons.
Blocker
- H1 The DomainExceptionThrow analyzer used a hyphenated Roslyn diagnostic id
("LearnStackException-DomainExceptionThrow"), which Roslyn rejects — it
threw AD0001 at report time, so the intended warning never fired and, under
CI's TreatWarningsAsErrors, the first DomainException throw would break the
build. Reproduced empirically. Fixed: diagnostic id is now LS0001 (valid
identifier); the hyphenated string is retained as the human-readable rule
name in the title/help text. LS0001 is listed in WarningsNotAsErrors so a
legitimate aggregate-invariant throw stays a warning in CI until the
Phase 03 escalation. New DomainExceptionThrowAnalyzerTests run the analyzer
over synthetic compilations and assert LS0001 fires (no AD0001). Recorded
as ADR-0032 Amendment 1; Standards 21 naming convention + analyzer entry
+ wiring description (ProjectReference OutputItemType=Analyzer, not
PackageReference) corrected.
Major
- Provider error body/status consistency: HttpStatusMap.For(Exception) now
derives the status from the carried Error.Code for every
LearnStackException instead of special-casing
ProviderException.IsClientError → 400. IsClientError is purely the Sentry
boundary; a bare provider failure is dependency_unavailable → 503, and an
adapter surfacing a provider 4xx passes an explicit Error
(validation_failed → 400). Body code and status can no longer disagree.
Tests assert both. ProviderException doc updated.
- Redaction over-match + nesting: SensitiveTokenCatalog.IsSensitive matches
on word-segment boundaries (camelCase / _ . -) instead of raw substrings,
so ClassName / BusinessName are no longer redacted by the "ssn" token while
Password / ApiKey / SSNToken still are. RedactSensitiveFieldsEnricher
recurses into StructureValue / DictionaryValue / SequenceValue so a
sensitive field nested in a non-sensitive top-level property
(User.Password) is scrubbed; lazy reconstruction keeps clean events
allocation-free. New SensitiveTokenCatalogTests +
RedactSensitiveFieldsEnricherTests.
- OTel naming + air-gapped: AddSource / AddMeter use the documented lowercase
learnstack.* convention (matching the learnstack.mediatr ActivitySource
without relying on case-insensitive wildcard matching). WireSerilog /
WireOpenTelemetry now take DeploymentMode; SelfHostedAirGapped never wires
the network OTLP exporters (no-egress contract), with a dated TODO for the
/var/learnstack/otel/ file target deferred to Phase 11 ops.
Medium
- M1 New Handlers_Return_Result architecture test asserts every
IRequestHandler<,TResponse> has TResponse : IResultBase, so a raw-DTO
handler cannot silently bypass the pipeline (validation / audit /
tenant-context + RLS). Vacuous today, active when handlers land.
Low
- L4 Serilog enrichers are resolved from DI (the singletons registered in
AddLearnStackObservabilityServices) instead of being new()'d in the
pipeline — no dead registrations.
Docs
- Domain_Methods_Do_Not_Throw_For_Expected_Cases marked deferred in
Standards 21 (the LS0001 analyzer already enforces the rule at build time;
the report-walking architecture test lands with module domain code in
Packet 6+). LoggingBehavior activity-name doc, correlationId-as-full-
traceparent in Standards 09/10, and the IMeterFactory.Create example in
architecture 33 reconciled with the code. Analyzer helpLinkUri casing.
Skipped (with reason)
- Resilience pipeline order (retry → breaker → timeout → bulkhead) left as-is
— faithful to ADR-0032 § Sub-decision 5's stated order. Whether the
concurrency limiter should sit outermost (cap total in-flight incl. retries,
per Microsoft.Extensions.Resilience) is an ADR-level question for a future
amendment, not a code defect.
- DeploymentMode 5-value enum: corpus-mandated (Standards 20 + ADR-0020); not
reduced.
- Strongly-typed IDs in CapturedContext / ITenantContext: deferred until the
TenantId / OrganizationId value objects land (Packet 6/7).
Validation
- dotnet build LearnStack.slnx (CI=true) → 0 warning, 0 error.
- Unit 154, Architecture 26, Integration 5, Contract 1 — all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…itch blocks CI's `dotnet format --verify-no-changes` step flagged WHITESPACE on the switch-section braces in RedactSensitiveFieldsEnricher.Redact (the recursive nested-redaction method added in the review-3/4 commit). Whitespace-only; no behavior change. Verified locally with `dotnet format LearnStack.slnx --verify-no-changes` (exit 0). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…packets/phases So the items skipped/deferred during the Packet 3 reviews don't slip between the cracks, each is now recorded where it will actually be done (the roadmap is the backlog — no separate issue tracker needed): - Phase 02a Packet 3: consolidated "Deferred follow-ups" ledger naming every item + its owner. - Phase 02a Packet 6: introduces the TenantId / OrganizationId Vogen value objects; the TransactionBehavior shell lights up with the per-module DbContext. - Phase 02a Packet 7: single-pass conversion of ITenantContext + CapturedContext from raw Guid to strongly-typed TenantId / OrganizationId; [AllowsUnresolvedTenantContext] marker-attribute replaces the AllowsUnresolvedContext stub. - Phase 02a Packet 10: clarifies Domain_Methods_Do_Not_Throw_For_Expected_Cases is deferred here from Packet 3 (needs module domain code; the LS0001 analyzer already enforces the rule meanwhile). - Phase 02b: OutboxFlushBehavior shell lights up with outbox enrolment. - Phase 03: AuthorizationBehavior shell lights up; LS0001 analyzer escalates Warning -> Error (removed from WarningsNotAsErrors) at phase exit. - Phase 11: air-gapped OTLP file target under /var/learnstack/otel/. (The "no secrets in exception messages" analyzer stays tracked via its code TODO + the Packet 3 ledger — it's cross-cutting, not an events/identity concern. The resilience pipeline-order question is logged as a future ADR-0032 amendment.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/RedactSensitiveFieldsEnricherTests.cs (1)
17-18: ⚡ Quick winUse
SensitiveTokenCatalog.RedactedValuedirectly in the test (remove localSensitiveTokenCatalog_RedactedValue).
SensitiveTokenCatalogdefinespublic const string RedactedValue = "***REDACTED***";, so the unit test can referenceSensitiveTokenCatalog.RedactedValueand avoid duplicating the literal (and the misleading intermediate constant) at lines 17-18.🤖 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 `@backend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/RedactSensitiveFieldsEnricherTests.cs` around lines 17 - 18, In RedactSensitiveFieldsEnricherTests replace the two local constants (Redacted and SensitiveTokenCatalog_RedactedValue) with a direct reference to SensitiveTokenCatalog.RedactedValue: remove the duplicate local constant SensitiveTokenCatalog_RedactedValue and change usages of Redacted to use SensitiveTokenCatalog.RedactedValue so the test relies on the canonical public constant from SensitiveTokenCatalog.
🤖 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 `@backend/src/LearnStack.SharedKernel/Secrets/SensitiveTokenCatalog.cs`:
- Around line 71-73: DefaultTokens currently returns the backing HashSet
(SingleWordTokens) via an interface which allows callers to cast and mutate it;
change DefaultTokens to return an immutable snapshot/view instead (e.g., return
ImmutableHashSet.CreateRange(SingleWordTokens) or return
Array.AsReadOnly(SingleWordTokens.ToArray())) so callers cannot alter the
original HashSet; update the DefaultTokens property in the SensitiveTokenCatalog
class to return that immutable collection rather than the raw SingleWordTokens
reference.
---
Nitpick comments:
In
`@backend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/RedactSensitiveFieldsEnricherTests.cs`:
- Around line 17-18: In RedactSensitiveFieldsEnricherTests replace the two local
constants (Redacted and SensitiveTokenCatalog_RedactedValue) with a direct
reference to SensitiveTokenCatalog.RedactedValue: remove the duplicate local
constant SensitiveTokenCatalog_RedactedValue and change usages of Redacted to
use SensitiveTokenCatalog.RedactedValue so the test relies on the canonical
public constant from SensitiveTokenCatalog.
🪄 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: 02d82856-d546-4012-94b2-ffe014b27e54
📒 Files selected for processing (22)
backend/Directory.Build.propsbackend/analyzers/LearnStack.Analyzers/AnalyzerReleases.Unshipped.mdbackend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.csbackend/src/LearnStack.Api/Common/HttpStatusMap.csbackend/src/LearnStack.Api/Composition/CrossCuttingFoundationExtensions.csbackend/src/LearnStack.Application/Pipeline/LoggingBehavior.csbackend/src/LearnStack.Infrastructure.Observability/Serilog/RedactSensitiveFieldsEnricher.csbackend/src/LearnStack.SharedKernel/Errors/ProviderException.csbackend/src/LearnStack.SharedKernel/Secrets/SensitiveTokenCatalog.csbackend/tests/LearnStack.Tests.Architecture/CrossCuttingFoundationTests.csbackend/tests/LearnStack.Tests.Integration/CrossCuttingFoundationHttpTests.csbackend/tests/LearnStack.Tests.Unit/Analyzers/DomainExceptionThrowAnalyzerTests.csbackend/tests/LearnStack.Tests.Unit/Api/Common/HttpStatusMapTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/RedactSensitiveFieldsEnricherTests.csbackend/tests/LearnStack.Tests.Unit/LearnStack.Tests.Unit.csprojbackend/tests/LearnStack.Tests.Unit/SharedKernel/Secrets/SensitiveTokenCatalogTests.csdocs/architecture/33-cross-cutting-concerns.mddocs/decisions/0032-exception-handling-logging-and-observability.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/standards/09-error-handling.mddocs/standards/10-observability.mddocs/standards/21-architecture-tests-catalogue.md
✅ Files skipped from review due to trivial changes (2)
- backend/analyzers/LearnStack.Analyzers/AnalyzerReleases.Unshipped.md
- docs/roadmap/phase-02a-kernel-tenancy.md
…nd adjust test for RedactedValue
Collaborator
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/roadmap/phase-02a-kernel-tenancy.md (1)
350-352: 🔒 Security & Privacy | 🔵 TrivialDefine and test the
SelfHostedAirGappedtelemetry contract.The roadmap claims no network egress and adds a fixed local target, but it does not require a test that proves all network exporters are disabled. It also does not define directory permissions, rotation, retention, disk limits, or write-failure behavior. Without these controls, a new exporter can violate the air-gap guarantee, or unbounded telemetry can exhaust the host disk.
docs/roadmap/phase-02a-kernel-tenancy.md#L350-L352: add a validation that rejects network telemetry exporters inSelfHostedAirGapped.docs/roadmap/phase-11-production-hardening.md#L76-L83: specify creation, permissions, rotation, quotas, retention, and failure behavior for/var/learnstack/otel/.🤖 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/roadmap/phase-02a-kernel-tenancy.md` around lines 350 - 352, Define and test the SelfHostedAirGapped telemetry contract: in docs/roadmap/phase-02a-kernel-tenancy.md lines 350-352, require validation that rejects all network telemetry exporters; in docs/roadmap/phase-11-production-hardening.md lines 76-83, specify creation, permissions, rotation, quotas, retention, and write-failure behavior for /var/learnstack/otel/.
🤖 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/roadmap/phase-02a-kernel-tenancy.md`:
- Around line 344-349: Update the LS0001 roadmap wording in the
AuthorizationBehavior and architecture-test entries to describe it as partial
detection that reports direct DomainException throws, rather than broadly
enforcing the rule. State that it remains a warning until Phase 03 exit and does
not replace the broader report-walking architecture test.
---
Nitpick comments:
In `@docs/roadmap/phase-02a-kernel-tenancy.md`:
- Around line 350-352: Define and test the SelfHostedAirGapped telemetry
contract: in docs/roadmap/phase-02a-kernel-tenancy.md lines 350-352, require
validation that rejects all network telemetry exporters; in
docs/roadmap/phase-11-production-hardening.md lines 76-83, specify creation,
permissions, rotation, quotas, retention, and write-failure behavior for
/var/learnstack/otel/.
🪄 Autofix
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: 63d89ad7-3772-48df-8967-fb0efa71db92
📒 Files selected for processing (6)
backend/src/LearnStack.SharedKernel/Secrets/SensitiveTokenCatalog.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/RedactSensitiveFieldsEnricherTests.csdocs/roadmap/phase-02a-kernel-tenancy.mddocs/roadmap/phase-02b-events-auth.mddocs/roadmap/phase-03-identity-admin.mddocs/roadmap/phase-11-production-hardening.md
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/LearnStack.SharedKernel/Secrets/SensitiveTokenCatalog.cs
- backend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/RedactSensitiveFieldsEnricherTests.cs
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
docs/roadmap/phase-11-production-hardening.md (1)
76-83: 🩺 Stability & Availability | 🔵 TrivialDefine operational controls for the air-gapped file exporter.
Before this deliverable is complete, define rotation, retention, file ownership, permissions, and disk-full behavior for
/var/learnstack/otel/. Unbounded telemetry can fill the volume. Permissive files can expose telemetry context.🤖 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/roadmap/phase-11-production-hardening.md` around lines 76 - 83, Expand the air-gapped OTLP file target deliverable to define operational controls for /var/learnstack/otel/: specify file rotation, retention limits, owner and group, restrictive permissions, and behavior when the disk or volume is full. Keep these controls aligned with the SelfHostedAirGapped telemetry path and ensure they prevent unbounded growth and unauthorized telemetry access.
🤖 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 `@backend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.cs`:
- Around line 91-110: Align LS0001 with the aggregate-invariant policy by
updating InspectThrown to exempt approved aggregate-invariant guard throws, and
add tests covering both exempt guards and still-invalid direct DomainException
constructions; alternatively, if all direct constructions must be rejected,
update LearnStack.Domain.csproj and the ADR-0032 contract consistently and
adjust tests accordingly. Apply the chosen policy at both the analyzer and
project-contract sites.
In `@backend/src/LearnStack.Api/Common/ProblemDetailsFactory.cs`:
- Around line 121-127: Update the projection logic around the ToCamelCase(key)
assignment to merge message entries when multiple detail keys normalize to the
same camel-case key, rather than overwriting the existing value. Preserve all
projected objects in the combined list before storing the result in projected.
In `@backend/src/LearnStack.Infrastructure.ErrorTracking/AssemblyMarker.cs`:
- Line 3: Update every AssemblyMarker declaration across all 37
AssemblyMarker.cs files from file-scoped syntax to a braced empty static class
body, preserving the public static AssemblyMarker type and ensuring all projects
compile.
In
`@backend/src/LearnStack.Infrastructure.ErrorTracking/LocalFileErrorTracker.cs`:
- Around line 91-100: Update the envelope-writing method around File.Create and
JsonSerializer.SerializeAsync to serialize into a temporary file, then move it
to the final path only after serialization completes successfully. In the
existing catch block, delete the temporary file when present before calling
LogWriteFailure, while preserving cancellation and best-effort failure handling.
In `@backend/src/LearnStack.Infrastructure.Observability/AssemblyMarker.cs`:
- Line 7: Replace the semicolon-only AssemblyMarker class declarations with
empty braced class bodies, including the matching AssemblyMarker declarations
elsewhere, so these non-record classes compile.
In `@backend/src/LearnStack.Infrastructure.Resilience/ProviderResilience.cs`:
- Around line 52-54: Update both resilience ShouldHandle predicate builders in
ProviderResilience to include TimeoutRejectedException alongside the existing
handled exceptions, so retry and circuit-breaker policies process provider
timeouts. Add a test using a delegate that observes the supplied cancellation
token and verifies timeout retries, while preserving caller cancellation as
OperationCanceledException rather than treating it as a timeout failure.
In `@backend/src/LearnStack.SharedKernel/Secrets/SensitiveTokenCatalog.cs`:
- Around line 98-108: Update the token matching logic in the sensitive-token
detection method to also recognize each two-word token when the input is
provided as one standalone joined segment, such as “authheader”. Preserve the
existing segmented two-word matching and single-word matching behavior, while
ensuring joined-token matching does not match unrelated substrings.
In `@docs/roadmap/phase-02a-kernel-tenancy.md`:
- Around line 151-153: Align the test totals reported in the earlier validation
summary with the final validation counts: update the LearnStack.Tests.Unit and
LearnStack.Tests.Architecture values to 154 and 26, respectively, or explicitly
label the existing 111 and 25 values as pre-review counts.
- Around line 107-109: Update the resilience pipeline summary in the section
containing `LearnStack.Infrastructure.Resilience/` to include the active
bulkhead policy alongside retry, circuit breaker, and timeout, matching the
pipeline documented later in the file and preserving the existing policy order
where applicable.
In `@docs/standards/10-observability.md`:
- Line 51: Update the observability standard’s correlation_id definition to
require a stable correlation value separate from the W3C traceparent, using the
trace ID or another explicitly preserved value that remains unchanged across
retries and spans. Document traceparent independently for span propagation, and
remove claims that outbox/Hangfire propagation or Problem Details/error-tracker
surfacing are implemented unless they are actually defined.
---
Nitpick comments:
In `@docs/roadmap/phase-11-production-hardening.md`:
- Around line 76-83: Expand the air-gapped OTLP file target deliverable to
define operational controls for /var/learnstack/otel/: specify file rotation,
retention limits, owner and group, restrictive permissions, and behavior when
the disk or volume is full. Keep these controls aligned with the
SelfHostedAirGapped telemetry path and ensure they prevent unbounded growth and
unauthorized telemetry access.
🪄 Autofix
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: eecb2517-ac84-4fcd-8790-76d10b0815ce
📒 Files selected for processing (102)
backend/Directory.Build.propsbackend/Directory.Packages.propsbackend/LearnStack.slnxbackend/analyzers/LearnStack.Analyzers/AnalyzerReleases.Shipped.mdbackend/analyzers/LearnStack.Analyzers/AnalyzerReleases.Unshipped.mdbackend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.csbackend/analyzers/LearnStack.Analyzers/LearnStack.Analyzers.csprojbackend/src/LearnStack.Api/Common/HttpStatusMap.csbackend/src/LearnStack.Api/Common/LearnStackExceptionHandler.csbackend/src/LearnStack.Api/Common/ProblemDetailsActionResult.csbackend/src/LearnStack.Api/Common/ProblemDetailsFactory.csbackend/src/LearnStack.Api/Common/ResultExtensions.csbackend/src/LearnStack.Api/Composition/CrossCuttingFoundationExtensions.csbackend/src/LearnStack.Api/LearnStack.Api.csprojbackend/src/LearnStack.Api/Program.csbackend/src/LearnStack.Api/Properties/AssemblyInfo.csbackend/src/LearnStack.Api/appsettings.jsonbackend/src/LearnStack.Application/LearnStack.Application.csprojbackend/src/LearnStack.Application/Pipeline/AuditLogBehavior.csbackend/src/LearnStack.Application/Pipeline/AuthorizationBehavior.csbackend/src/LearnStack.Application/Pipeline/LoggingBehavior.csbackend/src/LearnStack.Application/Pipeline/MediatRPipelineRegistration.csbackend/src/LearnStack.Application/Pipeline/OutboxFlushBehavior.csbackend/src/LearnStack.Application/Pipeline/TenantContextBehavior.csbackend/src/LearnStack.Application/Pipeline/TransactionBehavior.csbackend/src/LearnStack.Application/Pipeline/ValidationBehavior.csbackend/src/LearnStack.Domain/LearnStack.Domain.csprojbackend/src/LearnStack.Infrastructure.ErrorTracking/AssemblyMarker.csbackend/src/LearnStack.Infrastructure.ErrorTracking/ErrorTrackingOptions.csbackend/src/LearnStack.Infrastructure.ErrorTracking/ErrorTrackingRegistration.csbackend/src/LearnStack.Infrastructure.ErrorTracking/LearnStack.Infrastructure.ErrorTracking.csprojbackend/src/LearnStack.Infrastructure.ErrorTracking/LocalFileErrorTracker.csbackend/src/LearnStack.Infrastructure.ErrorTracking/NoOpErrorTracker.csbackend/src/LearnStack.Infrastructure.ErrorTracking/Properties/AssemblyInfo.csbackend/src/LearnStack.Infrastructure.ErrorTracking/SentryErrorTracker.csbackend/src/LearnStack.Infrastructure.Observability/AssemblyMarker.csbackend/src/LearnStack.Infrastructure.Observability/LearnStack.Infrastructure.Observability.csprojbackend/src/LearnStack.Infrastructure.Observability/ObservabilityRegistration.csbackend/src/LearnStack.Infrastructure.Observability/Serilog/CorrelationContextEnricher.csbackend/src/LearnStack.Infrastructure.Observability/Serilog/RedactSensitiveFieldsEnricher.csbackend/src/LearnStack.Infrastructure.Observability/TenantContextAccessor.csbackend/src/LearnStack.Infrastructure.Observability/TenantContextSpanProcessor.csbackend/src/LearnStack.Infrastructure.Resilience/AssemblyMarker.csbackend/src/LearnStack.Infrastructure.Resilience/LearnStack.Infrastructure.Resilience.csprojbackend/src/LearnStack.Infrastructure.Resilience/Properties/AssemblyInfo.csbackend/src/LearnStack.Infrastructure.Resilience/ProviderResilience.csbackend/src/LearnStack.Infrastructure.Resilience/ProviderResilienceRegistration.csbackend/src/LearnStack.SharedKernel/Errors/DomainException.csbackend/src/LearnStack.SharedKernel/Errors/InfrastructureException.csbackend/src/LearnStack.SharedKernel/Errors/LearnStackException.csbackend/src/LearnStack.SharedKernel/Errors/ProviderException.csbackend/src/LearnStack.SharedKernel/Errors/TenantContextMissingException.csbackend/src/LearnStack.SharedKernel/Hosting/DeploymentMode.csbackend/src/LearnStack.SharedKernel/LearnStack.SharedKernel.csprojbackend/src/LearnStack.SharedKernel/Observability/IErrorTrackingProvider.csbackend/src/LearnStack.SharedKernel/Resilience/IProviderResilience.csbackend/src/LearnStack.SharedKernel/Resilience/ResilienceOptions.csbackend/src/LearnStack.SharedKernel/Secrets/ConfigurationSecretProvider.csbackend/src/LearnStack.SharedKernel/Secrets/ISecretProvider.csbackend/src/LearnStack.SharedKernel/Secrets/SensitiveTokenCatalog.csbackend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.csbackend/src/LearnStack.SharedKernel/Tenancy/ITenantContextAccessor.csbackend/src/LearnStack.SharedKernel/Tenancy/UnresolvedTenantContext.csbackend/src/Modules/Audit/LearnStack.Modules.Audit.Application/LearnStack.Modules.Audit.Application.csprojbackend/src/Modules/Audit/LearnStack.Modules.Audit.Domain/LearnStack.Modules.Audit.Domain.csprojbackend/src/Modules/Content/LearnStack.Modules.Content.Application/LearnStack.Modules.Content.Application.csprojbackend/src/Modules/Content/LearnStack.Modules.Content.Domain/LearnStack.Modules.Content.Domain.csprojbackend/src/Modules/Customization/LearnStack.Modules.Customization.Application/LearnStack.Modules.Customization.Application.csprojbackend/src/Modules/Customization/LearnStack.Modules.Customization.Domain/LearnStack.Modules.Customization.Domain.csprojbackend/src/Modules/Education/LearnStack.Modules.Education.Application/LearnStack.Modules.Education.Application.csprojbackend/src/Modules/Education/LearnStack.Modules.Education.Domain/LearnStack.Modules.Education.Domain.csprojbackend/src/Modules/Identity/LearnStack.Modules.Identity.Application/LearnStack.Modules.Identity.Application.csprojbackend/src/Modules/Identity/LearnStack.Modules.Identity.Domain/LearnStack.Modules.Identity.Domain.csprojbackend/src/Modules/Media/LearnStack.Modules.Media.Application/LearnStack.Modules.Media.Application.csprojbackend/src/Modules/Media/LearnStack.Modules.Media.Domain/LearnStack.Modules.Media.Domain.csprojbackend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Application/LearnStack.Modules.Tenancy.Application.csprojbackend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Domain/LearnStack.Modules.Tenancy.Domain.csprojbackend/tests/LearnStack.Tests.Architecture/CrossCuttingFoundationTests.csbackend/tests/LearnStack.Tests.Architecture/LearnStack.Tests.Architecture.csprojbackend/tests/LearnStack.Tests.Integration/CrossCuttingFoundationHttpTests.csbackend/tests/LearnStack.Tests.Integration/LearnStack.Tests.Integration.csprojbackend/tests/LearnStack.Tests.Unit/Analyzers/DomainExceptionThrowAnalyzerTests.csbackend/tests/LearnStack.Tests.Unit/Api/Common/HttpStatusMapTests.csbackend/tests/LearnStack.Tests.Unit/Api/Common/ResultExtensionsTests.csbackend/tests/LearnStack.Tests.Unit/Application/Pipeline/AuditLogBehaviorTests.csbackend/tests/LearnStack.Tests.Unit/Application/Pipeline/TenantContextBehaviorTests.csbackend/tests/LearnStack.Tests.Unit/Application/Pipeline/ValidationBehaviorTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/ErrorTracking/LocalFileErrorTrackerTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/RedactSensitiveFieldsEnricherTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/Observability/TenantContextSpanProcessorTests.csbackend/tests/LearnStack.Tests.Unit/Infrastructure/Resilience/ProviderResilienceTests.csbackend/tests/LearnStack.Tests.Unit/LearnStack.Tests.Unit.csprojbackend/tests/LearnStack.Tests.Unit/SharedKernel/Secrets/SensitiveTokenCatalogTests.csdocs/architecture/33-cross-cutting-concerns.mddocs/decisions/0032-exception-handling-logging-and-observability.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/roadmap/phase-02b-events-auth.mddocs/roadmap/phase-03-identity-admin.mddocs/roadmap/phase-11-production-hardening.mddocs/standards/09-error-handling.mddocs/standards/10-observability.mddocs/standards/21-architecture-tests-catalogue.md
Comment on lines
+91
to
+110
| private static void InspectThrown(SyntaxNodeAnalysisContext context, ExpressionSyntax expression) | ||
| { | ||
| if (expression is not ObjectCreationExpressionSyntax creation) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var typeInfo = context.SemanticModel.GetTypeInfo(creation, context.CancellationToken); | ||
| var symbol = typeInfo.Type; | ||
| if (symbol is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!IsDomainException(symbol)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| context.ReportDiagnostic(Diagnostic.Create(Rule, creation.GetLocation())); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- analyzer file ---'
sed -n '1,220p' backend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.cs
printf '%s\n' '--- domain project file ---'
cat -n backend/src/LearnStack.Domain/LearnStack.Domain.csproj
printf '%s\n' '--- analyzer-related files and references ---'
git ls-files | grep -E 'DomainExceptionThrow|Analyzer|ADR-0032|Directory.Build.props|LearnStack.Domain' | head -200
printf '%s\n' '--- relevant symbols and diagnostics ---'
rg -n -S 'DomainException|LearnStackException|invariant|aggregate|guard|DomainExceptionThrow|ADR-0032|Phase 02a|Phase 03' \
backend/analyzers backend/src backend/tests backend test tests docs 2>/dev/null | head -300Repository: HodeTech/LearnStack
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- analyzer tests ---'
cat -n backend/tests/LearnStack.Tests.Unit/Analyzers/DomainExceptionThrowAnalyzerTests.cs
printf '%s\n' '--- build properties ---'
cat -n backend/Directory.Build.props
printf '%s\n' '--- ADR section 4 and amendment ---'
sed -n '1,70p' docs/decisions/0032-exception-handling-logging-and-observability.md
sed -n '200,235p' docs/decisions/0032-exception-handling-logging-and-observability.md
sed -n '455,500p' docs/decisions/0032-exception-handling-logging-and-observability.md
printf '%s\n' '--- all direct DomainException constructions and throws ---'
rg -n -S -U 'throw\s+(?:new\s+)?(?:[A-Za-z0-9_.<>?]+\.)?DomainException|new\s+DomainException' \
backend --glob '*.cs' --glob '*.csproj' --glob '*.props' || true
printf '%s\n' '--- project references to the analyzer ---'
rg -n -S 'LearnStack\.Analyzers|OutputItemType="Analyzer"|DomainExceptionThrowAnalyzer' backend --glob '*.csproj' --glob '*.props' --glob '*.cs'Repository: HodeTech/LearnStack
Length of output: 28394
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- DomainException contract ---'
cat -n backend/src/LearnStack.SharedKernel/Errors/DomainException.cs
printf '%s\n' '--- application project contract ---'
cat -n backend/src/LearnStack.Application/LearnStack.Application.csproj
printf '%s\n' '--- analyzer wiring comments for representative projects ---'
for f in \
backend/src/Modules/Media/LearnStack.Modules.Media.Domain/LearnStack.Modules.Media.Domain.csproj \
backend/src/Modules/Media/LearnStack.Modules.Media.Application/LearnStack.Modules.Media.Application.csproj \
backend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Domain/LearnStack.Modules.Tenancy.Domain.csproj \
backend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Application/LearnStack.Modules.Tenancy.Application.csproj
do
echo "--- $f ---"
cat -n "$f"
done
printf '%s\n' '--- referenced architecture test and analyzer configuration ---'
rg -n -S 'Domain_Methods_Do_Not_Throw_For_Expected_Cases|LS0001|WarningsNotAsErrors|dotnet_diagnostic' \
backend tests docs --glob '*.cs' --glob '*.csproj' --glob '*.props' --glob '*.editorconfig' --glob '*.md' | head -250
printf '%s\n' '--- DomainException documentation in focused files ---'
rg -n -S -C 3 'aggregate.invariant|aggregate invariant|invariant guard|every .*throw|outside .*invariant|reserved for programmer' \
backend/src/LearnStack.SharedKernel/Errors/DomainException.cs \
backend/src/LearnStack.Domain \
backend/src/LearnStack.Application \
backend/src/Modules \
docs/decisions/0032-exception-handling-logging-and-observability.md \
docs/standards/09-error-handling.mdRepository: HodeTech/LearnStack
Length of output: 21419
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
analyzer = Path("backend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.cs").read_text()
standards = Path("docs/standards/09-error-handling.md").read_text()
domain_project = Path("backend/src/LearnStack.Domain/LearnStack.Domain.csproj").read_text()
method = re.search(
r"private static void InspectThrown\(.*?\n \}",
analyzer,
re.DOTALL,
)
assert method, "InspectThrown was not found"
body = method.group(0)
assert "context.ReportDiagnostic(Diagnostic.Create(Rule, creation.GetLocation()));" in body
assert "if (!IsDomainException(symbol))" in body
assert not re.search(r"guard|invariant|ContainingMethod|Ancestor|Attribute", body, re.I), \
"InspectThrown unexpectedly contains a guard/invariant exemption"
assert "outside aggregate invariant guards" in standards
assert "outside aggregate" in domain_project and "invariant guards" in domain_project
print("Analyzer reports a matching DomainException construction without an invariant-guard check.")
print("Standards 09 and LearnStack.Domain.csproj document an aggregate-invariant-guard exemption.")
PYRepository: HodeTech/LearnStack
Length of output: 339
Align LS0001 with aggregate-invariant guards
InspectThrown reports every matching DomainException construction. It does not detect aggregate-invariant guards. After Phase 03, valid invariant throws can fail the build.
- Add and test an explicit guard exemption, or update the project and ADR-0032 contract to reject all direct constructions.
📍 Affects 2 files
backend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.cs#L91-L110(this comment)backend/src/LearnStack.Domain/LearnStack.Domain.csproj#L10-L16
🤖 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 `@backend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.cs`
around lines 91 - 110, Align LS0001 with the aggregate-invariant policy by
updating InspectThrown to exempt approved aggregate-invariant guard throws, and
add tests covering both exempt guards and still-invalid direct DomainException
constructions; alternatively, if all direct constructions must be rejected,
update LearnStack.Domain.csproj and the ADR-0032 contract consistently and
adjust tests accordingly. Apply the chosen policy at both the analyzer and
project-contract sites.
| @@ -0,0 +1,3 @@ | |||
| namespace LearnStack.Infrastructure.ErrorTracking; | |||
|
|
|||
| public static class AssemblyMarker; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect no non-record class declarations that terminate directly with ';'.
rg -nP '^\s*(?:public|internal|protected|private)?\s*(?:static\s+)?(?:abstract\s+|sealed\s+)?class\s+\w+(?:<[^>]+>)?\s*;\s*$' backend/srcRepository: HodeTech/LearnStack
Length of output: 4582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- language and project settings ---'
rg -n --glob '*.csproj' --glob '*.props' --glob '*.targets' \
'TargetFramework|LangVersion|EnablePreviewFeatures|CSharp' backend
printf '%s\n' '--- marker declarations and nearby file contents ---'
for f in $(git ls-files 'backend/src/**/AssemblyMarker.cs'); do
printf '\n### %s\n' "$f"
cat -n "$f"
doneRepository: HodeTech/LearnStack
Length of output: 8792
🌐 Web query:
C# language specification class declaration semicolon class body class C;``
💡 Result:
In the C# language specification, the syntactic grammar for a class declaration allows for an optional semicolon after the class body [1][2][3][4]. The relevant production is defined as follows: class_declaration: attributes? class_modifier* 'partial'? 'class' identifier type_parameter_list? class_base? type_parameter_constraints_clause* class_body ';'?; The class_body itself is defined as: class_body: '{' class_member_declaration* '}'; While the grammar allows for the optional semicolon after the class declaration (class_declaration), a construct like 'class C;' is not a valid complete class declaration in C# because the class_body (which contains the braces) is mandatory for defining the class members [1][2]. The semicolon at the end of a class declaration is a trailing character that may appear after the closing brace of the class body (e.g., 'class C { };'), not a replacement for the class body itself [1][2][5].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/classes
- 2: https://github.com/dotnet/csharpstandard/blob/master/standard/classes.md
- 3: https://github.com/dotnet/csharpstandard/blob/draft-v8/standard/classes.md
- 4: https://github.com/dotnet/csharpstandard/blob/standard-v7/standard/classes.md
- 5: https://ecma-international.org/wp-content/uploads/ECMA-334_7th_edition_december_2023.pdf
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path(p) for p in __import__("subprocess").check_output(
["git", "ls-files", "backend/src"], text=True
).splitlines()
if p.endswith("/AssemblyMarker.cs")
]
pattern = re.compile(
r"^\s*(?:(?:public|internal|protected|private)\s+)?"
r"(?:(?:static|abstract|sealed)\s+)*class\s+\w+"
r"(?:\s*<[^>\n]+>)?\s*;\s*$"
)
matches = []
for path in files:
lines = path.read_text(encoding="utf-8").splitlines()
for number, line in enumerate(lines, 1):
if pattern.fullmatch(line):
matches.append((str(path), number, line))
print(f"AssemblyMarker.cs files: {len(files)}")
print(f"Bodyless class declarations ending in ';': {len(matches)}")
for path, number, line in matches:
print(f"{path}:{number}: {line}")
PYRepository: HodeTech/LearnStack
Length of output: 4693
Add a class body to AssemblyMarker.
public static class AssemblyMarker; is invalid C# syntax and prevents compilation. Replace it with a braced empty class. Apply the same fix to all 37 AssemblyMarker.cs files.
🤖 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 `@backend/src/LearnStack.Infrastructure.ErrorTracking/AssemblyMarker.cs` at
line 3, Update every AssemblyMarker declaration across all 37 AssemblyMarker.cs
files from file-scoped syntax to a braced empty static class body, preserving
the public static AssemblyMarker type and ensuring all projects compile.
| /// Reflection seam — gives assembly scanners (architecture tests, | ||
| /// composition-root extension wiring) a non-generic type to anchor against. | ||
| /// </summary> | ||
| public static class AssemblyMarker; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -nP --glob '*.cs' \
'^\s*(?:public|internal|protected|private)?\s*(?:static\s+)?(?:sealed\s+)?class\s+\w+[^{}]*;\s*$' \
backendRepository: HodeTech/LearnStack
Length of output: 4582
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- tool availability ---'
command -v dotnet || true
command -v csc || true
command -v mcs || true
printf '%s\n' '--- target file ---'
cat -n backend/src/LearnStack.Infrastructure.Observability/AssemblyMarker.cs
printf '%s\n' '--- project and language settings ---'
find backend -type f \( -name '*.csproj' -o -name 'Directory.Build.*' -o -name '*.props' -o -name '*.targets' \) -print0 |
sort -z |
xargs -0 rg -n -i 'TargetFramework|LangVersion|EnableDefaultCompileItems|Compile Include|DefineConstants' || true
printf '%s\n' '--- all semicolon-only class declarations ---'
rg -nP --glob '*.cs' '^\s*(?:public|internal|protected|private)?\s*(?:static\s+)?(?:sealed\s+)?class\s+\w+[^{}]*;\s*$' backendRepository: HodeTech/LearnStack
Length of output: 5661
🌐 Web query:
C# language specification class declaration semicolon-only body class C; valid
💡 Result:
A C# class declaration consisting only of the class header followed by a semicolon (e.g., class C;) is not a valid class declaration in C# [1][2]. According to the C# language specification, a class declaration is defined by the following grammar [1][2]: class_declaration: attributes? class_modifier* 'partial'? 'class' identifier type_parameter_list? class_base? type_parameter_constraints_clause* class_body ';'?; While the grammar shows that a class declaration may be optionally followed by a semicolon, the class_body itself is mandatory [1][2]. The class_body is defined as [2]: class_body: '{' class_member_declaration* '}'; Because the class_body must contain the curly braces { and }, a declaration like class C; lacks the required body and is therefore syntactically invalid [1][2]. Note that while a semicolon is permitted after the closing brace of a class declaration (e.g., class C { };), it is optional and not required [3]. Any semicolon appearing there is treated as an empty statement or extra token depending on the context, but it cannot replace the required class body [3].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/classes
- 2: https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/classes.md
- 3: https://stackoverflow.com/questions/6008039/why-mustnt-i-put-a-semicolon-at-the-end-of-class-declaration-in-c-sharp
Replace the semicolon-only class declarations.
A non-record class requires a braced body. Replace this declaration and the matching AssemblyMarker declarations with empty class bodies; otherwise the projects do not compile.
🤖 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 `@backend/src/LearnStack.Infrastructure.Observability/AssemblyMarker.cs` at
line 7, Replace the semicolon-only AssemblyMarker class declarations with empty
braced class bodies, including the matching AssemblyMarker declarations
elsewhere, so these non-record classes compile.
| | `trace_id` | OTel trace context | W3C `traceparent` propagated end to end | | ||
| | `span_id` | OTel | Per operation | | ||
| | `correlation_id` | Per request | Stable across retries; equals trace id at request boundary | | ||
| | `correlation_id` | Per request | Full W3C traceparent (`Activity.Current.Id`, `00-<trace>-<span>-<flags>`); embeds the trace id and is stable across retries. Surfaced on the Problem Details body + error-tracker captures so all three correlate | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect the original correlation value to be preserved when retries create new activities.
rg -n -C 6 \
'correlation_id|CorrelationId|Activity\.Current\.Id|StartActivity|Retry|RetryStrategyOptions|Attempt' \
backend || trueRepository: HodeTech/LearnStack
Length of output: 46557
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- propagation symbols ---'
rg -n -C 5 \
'traceparent|TraceParent|CorrelationId|correlation_id|ActivityContext|Propagator|Propagate|Extract|Inject|Hangfire|outbox|Outbox|StartActivity|ActivitySource' \
backend/src backend/tests docs/standards \
--glob '*.cs' --glob '*.md' --glob '*.json' || true
printf '%s\n' '--- candidate files ---'
git ls-files backend/src backend/tests | rg -i 'outbox|hangfire|job|envelope|message|activity|tenant.*context|correlation' || trueRepository: HodeTech/LearnStack
Length of output: 50378
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- relevant source files ---'
git ls-files backend/src backend/tests docs/architecture docs/decisions | rg -i \
'outbox|hangfire|job|event|envelope|correlation|observab|activity|tenant.*context|context.*tenant' \
| head -200
printf '%s\n' '--- exact propagation references ---'
rg -l -i \
'traceparent|correlation_id|CorrelationId|ActivityContext|Propagator|Inject|Extract|JobActivator|Hangfire|IOutbox|OutboxProcessor|IntegrationEvent' \
backend/src backend/tests docs/architecture docs/decisions \
--glob '*.cs' --glob '*.md' --glob '*.json' \
| sortRepository: HodeTech/LearnStack
Length of output: 5546
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- outbox and context implementation ---'
cat -n backend/src/LearnStack.Application/Pipeline/OutboxFlushBehavior.cs
cat -n backend/src/LearnStack.Application/Pipeline/TenantContextBehavior.cs
cat -n backend/src/LearnStack.Infrastructure.Observability/TenantContextAccessor.cs
cat -n backend/src/LearnStack.SharedKernel/Tenancy/ITenantContext.cs
printf '%s\n' '--- documented correlation and propagation contracts ---'
rg -n -C 8 \
'correlation|traceparent|retry|Hangfire|outbox|envelope|causation' \
docs/architecture/15-event-and-outbox.md \
docs/architecture/33-cross-cutting-concerns.md \
docs/decisions/0032-exception-handling-logging-and-observability.md \
docs/decisions/0006-events-and-outbox.md \
docs/standards/10-observability.md \
| head -400
printf '%s\n' '--- retry implementation ---'
cat -n backend/src/LearnStack.Infrastructure.Resilience/ProviderResilience.csRepository: HodeTech/LearnStack
Length of output: 50377
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
traceparents = [
"00-0123456789abcdef0123456789abcdef-1111111111111111-01",
"00-0123456789abcdef0123456789abcdef-2222222222222222-01",
]
parts = [value.split("-") for value in traceparents]
assert all(len(p) == 4 and p[0] == "00" for p in parts)
assert parts[0][1] == parts[1][1]
assert parts[0][2] != parts[1][2]
print("same_trace_id:", parts[0][1])
print("span_ids_differ:", parts[0][2], "!=", parts[1][2])
print("full_traceparents_equal:", traceparents[0] == traceparents[1])
docs = Path("docs/standards/10-observability.md").read_text()
line = next(
line for line in docs.splitlines()
if "`correlation_id`" in line and "stable across retries" in line
)
print("declared_contract:", line)
source_files = list(Path("backend/src").rglob("*.cs"))
propagation_calls = []
for path in source_files:
text = path.read_text()
if re.search(r"\b(Inject|Extract|Propagate|DistributedContextPropagator|ActivityContext)\b", text):
propagation_calls.append(str(path))
print("source_files_with_explicit_propagation:", propagation_calls)
PYRepository: HodeTech/LearnStack
Length of output: 599
Define a stable correlation ID separately from traceparent.
Activity.Current.Id includes the current span ID. A new span on retry changes correlation_id even when the trace ID remains unchanged. The retry implementation does not define a stable correlation value, and outbox/Hangfire propagation is not implemented.
Carry a stable correlation ID and traceparent separately.
🤖 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/standards/10-observability.md` at line 51, Update the observability
standard’s correlation_id definition to require a stable correlation value
separate from the W3C traceparent, using the trace ID or another explicitly
preserved value that remains unchanged across retries and spans. Document
traceparent independently for span propagation, and remove claims that
outbox/Hangfire propagation or Problem Details/error-tracker surfacing are
implemented unless they are actually defined.
…ve token catalog checks, and update documentation
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.
Summary
Phase 02a Packet 3 — the ADR-0032 surface wired end to end. Three commits: land, fix the review-1 blockers + majors + minors, fix the review-2 follow-ups.
What ships
LearnStackExceptionHandler : IExceptionHandlerregistered viaservices.AddExceptionHandler<T>()+app.UseExceptionHandler().ShouldCapture(ex)drives the Sentry-vs-OTel boundary per Standards 09 (OperationCanceledException+ client-errorProviderExceptionskip capture). Internal sealed; tests reach it viaInternalsVisibleTo.Validation+Loggingreal;AuditLogships the try /ExceptionDispatchInforethrow shell (audit-write lights up in Packet 9);TenantContextshell short-circuits withResult.Fail(tenant_mismatch)until Packet 7's resolver;Authorization/Transaction/OutboxFlushpass-through shells. Order encoded inMediatRPipelineRegistration.CanonicalBehaviorOrderand asserted by a hardcoded-list architecture test.LearnStackExceptionhierarchy —DomainException,InfrastructureException,ProviderException(withIsClientError),TenantContextMissingExceptioninSharedKernel/Errors/. Roslyn analyzerLearnStackException-DomainExceptionThrowships underbackend/analyzers/LearnStack.Analyzers/and is wired into all 14 module Domain + Application csprojs viaOutputItemType=\"Analyzer\"—throw new DomainException(...)in any module trips the analyzer (Warning in Phase 02a, escalates to Error after Phase 03 exit per ADR-0032 § Sub-decision 4).Result<T>.ToActionResult()+ProblemDetailsActionResult— failure path defers the Problem Details body assembly untilExecuteResultAsync, so the sanctioned controller shape(await Send(...)).ToActionResult()populatesInstance+correlationIdfromHttpContextwithout the caller threading it.LearnStack.Infrastructure.Observability—TenantContextAccessor(singletonAsyncLocal<ITenantContext?>),TenantContextSpanProcessor(enriches every span with stringified Guid tags),RedactSensitiveFieldsEnricher+CorrelationContextEnricherfor the Serilog pipeline.LearnStack.Infrastructure.ErrorTracking—NoOpErrorTracker/SentryErrorTracker/LocalFileErrorTrackerselected byDeploymentMode. Sentry SDK referenced only here; architecture testModules_Do_Not_Reference_Sentry_SDK_Directlyenforces. DSN resolves viaISecretProvider.LearnStack.Infrastructure.Resilience— Polly v8IProviderResilience<TPort>with retry → circuit breaker → timeout → bulkhead (Polly.RateLimiting'sAddRateLimiter(ConcurrencyLimiterOptions)); config shapeappsettings.Resilience:<port>:. Hub HTTP clients excluded per ADR-0019.ISecretProvidersocket —SharedKernel/Secrets/ISecretProvider.cs+ConfigurationSecretProviderdefault;SelectSecretProviderhelper in the composition root is the single seam Packet 5 swaps forDaprSecretProvider(Vault).SensitiveTokenCatalogis the shared source of truth for the sensitive-property-name token list (password/token/secret/dsn/jwt/apikey/authorization/tckn/vkn/iban/cardnumber/cvv/ …) so the Serilog enricher and the air-gapped JSON envelope cannot drift.WriteTo.Console(RenderedCompactJsonFormatter)+WriteTo.OpenTelemetry(OTLP gRPC)+ the correlation-context enricher + the redaction enricher. The OTelLoggerProvider(AddOpenTelemetry().WithLogging()) is not registered alongside per ADR-0032 § Sub-decision 8.AspNetCore+HttpClient+EntityFrameworkCoreinstrumentation +TenantContextSpanProcessor+ OTLP exporter.Commits
b1e1306feat — land Packet 3 surface6023f67fix — review-1 (B1 Sentry DSN via ISecretProvider; B2 Serilog enricher pair + LocalFile redaction; M1 ProblemDetailsFactory status path; M2 handler skips AddException for client provider errors; M3 analyzer wired into all 14 module csprojs; M4 Polly bulkhead via RateLimiter; N1 camelCase nested + acronyms; N2 lazy ProblemDetails HttpContext binding; N3 LocalFile filename uniqueness + stackalloc cap; A5 AuditLog filters OperationCanceledException; A7 7+1 comment; A8 type URL drops_failed; A11 WebApplicationFactory integration tests; A12 handler internal sealed; A13 OTel Guid stringification; S1 hardcoded pipeline order; SU1 skip body on cancellation; SU4[AllowsUnresolvedTenantContext]seam doc; + newModules_Do_Not_Reference_DeploymentModetest)a194b77fix — review-2 (N4 single ISecretProvider instance viaSelectSecretProvider; SU5SensitiveTokenCatalogshared catalog incl.vkn; SU6 Roslyn analyzer follow-up TODO + roadmap note; SU7 499 rationale comment)Standards / ADR cross-links
DeploymentMode.MediatR_Pipeline_Order_Matches_Canonical_Sequence,IExceptionHandler_Registered_AtStartup,OTel_Pipeline_Includes_TenantContextSpanProcessor,Logging_Goes_Through_Microsoft_Extensions_Logging,Modules_Do_Not_Reference_Sentry_SDK_Directly,Adapters_Wrap_Provider_Exceptions,IErrorTrackingProvider_Is_Singleton,Modules_Do_Not_Reference_DeploymentMode,TenantContextSpanProcessor_DoesNotThrow_When_Context_Missing,ValidationBehavior_DoesNotThrow_ValidationException— last one upgraded to "unit + integration");LearnStackException-DomainExceptionThrowanalyzer entry shipped.Test plan
dotnet build LearnStack.slnx /p:CI=true(TreatWarningsAsErrors) — 0 warnings, 0 errorsdotnet test LearnStack.Tests.Unit— 111/111 greendotnet test LearnStack.Tests.Architecture— 25/25 green (incl. the new cross-cutting catalogue entries +Modules_Do_Not_Reference_DeploymentMode)dotnet test LearnStack.Tests.Integration— 5/5 green (WebApplicationFactory-based L1 handler + ValidationBehavior end-to-end)dotnet test LearnStack.Tests.Contract— 1/1 greendocs/analysis/residual scan over changed files — cleanNotes for the reviewer
SelectSecretProvideris the Packet 5 seam. AddingDaprSecretProviderfor SaaS / Dedicated / SelfHostedOnline is a one-method edit; the DI registration and the localAddLearnStackErrorTrackingcall both consume the same instance returned by the helper.SensitiveTokenCatalogis the single source of truth for the redaction substring list — Serilog enricher and the air-gappedLocalFileErrorTrackerboth read it. Adding a token lights up both surfaces.AddExceptionvsRecordException: the code uses the .NET 9+Activity.AddExceptionAPI; the ADR's Implementation Notes still referenceRecordException. The handler comment names the change explicitly so the next reader doesn't second-guess.Title = messageKey(lockey_*) — matches the Standards 09 § API Surface example. A future LocalizedMessage → human-readable text projector (Phase 02b, Accept-Language binding) can compose a localised Title alongside; the wire shape stays stable across locales for support-handoff debugging in the meantime.Result.Fail(business_rule_violation, ...).TenantContextBehavior.AllowsUnresolvedContextis=> falsefor now; the dated TODO names the Packet 7 marker-attribute seam ([AllowsUnresolvedTenantContext]) so tenant-provisioning + platform-admin commands can opt in once those land.throw new ...Exception($\"...{token}...\")patterns inDomain+Applicationprojects. Runtime redaction covers logs / OTLP / Sentry tags; the analyzer would close the secrets-in-exception-messages gap at compile time. TODO comment in RedactSensitiveFieldsEnricher.cs anchors it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation