fix(security): log the error code instead of EncinaError.Message in EF Core, Marten and GraphQL (#1328) - #1397
Conversation
…d GraphQL Replace the free-text error.Message with error.GetEncinaCode() in the four Log.cs call sites named in #1328 (TransactionPipelineBehavior, DomainEventDispatcherInterceptor, EventPublishingPipelineBehavior, GraphQLMediatorBridge), plus one sibling found by the same search (InlineProjectionRelay's InlineProjectionFailedAfterSave log). Rename each LoggerMessage template placeholder from the message name to an error-code name, keeping EventIds unchanged. Grant Encina.Marten and Encina.GraphQL InternalsVisibleTo access to Encina so they can call the internal GetEncinaCode() extension, matching Encina.EntityFrameworkCore's existing access. Add a regression test per fixed call site that drives the failure path with a sentinel-bearing EncinaError and asserts the sentinel never reaches the log while the error code does.
…mplate for naming consistency (#1328)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: dlrivada/Encina/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dlrivada/Encina/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (17)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughLos registros modificados en EF Core, GraphQL y Marten usan códigos de error de Encina en lugar de mensajes libres. Se añaden pruebas que verifican el código registrado y la ausencia del texto centinela. ChangesRegistro de errores en integraciones
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The identified EF Core, GraphQL, and Marten logs use error codes rather than free-text error messages. No issue in the supplied evidence currently prevents merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 18.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 14 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CodeRabbit reported "Review limit reached" when the review was requested (2026-09-26 ~13:50 CEST). Per the pr-cycle procedure the merge does not wait for it: this PR already had an adversarial-reviewer pass before opening (no blocker or major; one minor fixed, one filed as #1396). The maintainer will request the CodeRabbit review again once the limit resets; any thread it opens blocks the auto-merge until answered. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…CRAP under 10 CI's crap-gate job flagged Handle (complexity 12, CRAP 12.42) after #1328's one-line error.GetEncinaCode() change pulled it into the gate. Extract the new-transaction path into ExecuteInNewTransactionAsync, the commit/rollback Match into CommitOrRollbackAsync, and the shared rollback-if-active check into RollbackIfActiveAsync. No behavior change; existing tests pass unmodified.
…ng CRAP under 10 CI's crap-gate job flagged Handle (complexity 18, CRAP 18.82) after #1328's one-line error.GetEncinaCode() change pulled it into the gate. Extract the pending-notification LINQ query into GetPendingNotifications and the per-event publish-and-log-on-failure logic into PublishEventAsync. No behavior change; existing tests pass unmodified.
…r 10 CI's crap-gate job flagged DispatchDomainEventsAsync (complexity 18, coverage 65%, CRAP 31.63) after #1328's one-line error.GetEncinaCode() change pulled it into the gate. Extract the INotification resolution check into TryResolveNotification, the per-event publish/try-catch into DispatchSingleEventAsync, the failed-publish logging and StopOnFirstError handling into HandlePublishFailure, and the post-dispatch entity clearing into ClearEntities. No behavior change; existing tests pass unmodified.
The crap-gate job measures the compiler-generated async state machine (<Handle>d__5.MoveNext), which folds every await point of one async method into a single complexity score (#1355: MoveNext attribution). With the publish-loop's await still inside Handle, its MoveNext stayed at complexity 10 (CRAP 10.29). Move the loop into PublishPendingEventsAsync so each async method's state machine covers fewer branches. No behavior change.
|
The first CI run failed the blocking crap-gate: the #1328 one-line fixes pulled three long pre-existing methods into the gate (CRAP 31.6, 18.8, 12.4). Four behaviour-preserving splits follow (61a2545, 61869a9, c10c435, eb7b327): |
…as the review-bot fallback (#1447) (#1458) * docs(plans): implementation plan for pr-reviewer agent (#1447) * feat(agents): add pr-reviewer agent definition (#1447) * feat(hooks): wire pr-reviewer into spawn/publish/path-ownership hooks (#1447) * test(hooks): add pr-reviewer regression cases to Test-Hooks.ps1 (#1447) * docs(pr-cycle): trigger pr-reviewer instead of adversarial-reviewer when CodeRabbit is unavailable (#1447) * docs(engineering): sync pr-reviewer agent into routing table and history (#1447) Adds pr-reviewer to the Claude subagent tier table in ai-task-routing.md (kept verbatim in sync with .claude/agents/README.md) and records it as item 15 of the 2026-09-23 agent system review in AI-DEVELOPMENT-MODEL.md. * docs(knowledge): record pr-reviewer implementation and #1397/#1399 dry-run comparison (#1447) * fix(agents): wire enforce-path-ownership on pr-reviewer's Bash|PowerShell matcher too * docs(knowledge): record the self-review finding and fix (#1447) * docs(knowledge): link PR #1458 in the #1447 record --------- Co-authored-by: dlrivada <3762783+dlrivada@users.noreply.github.com>
1 participant
Summary
EncinaError.Messagemust never reach a log (AGENTS.md §3, "only the error code or the exception type is recorded"). Five call sites in Encina.EntityFrameworkCore, Encina.Marten and Encina.GraphQL logged it, and the sibling search found a sixth in Marten's inline projection relay.What changed
error.GetEncinaCode():TransactionPipelineBehavior.cs(EF Core);DomainEvents/DomainEventDispatcherInterceptor.cs(EF Core);EventPublishingPipelineBehavior.cs(Marten);Projections/InlineProjectionRelay.cs(Marten, found by the sibling search);GraphQLMediatorBridge.cs, query and mutation paths (GraphQL).[LoggerMessage]templates in each package'sLog.csand in Marten'sProjectionLog.csuse{ErrorCode}instead of the message placeholder. EventIds are unchanged. The deadSkippingOutboxStorageDueToErrortemplate was renamed the same way for consistency.src/Encina/Encina.csproj:InternalsVisibleTofor Encina.Marten and Encina.GraphQL.GetEncinaCode()is internal to core, and EF Core already had this access.EncinaErrorwhose message carries the sentinelSENTINEL-do-not-log-4f2a, then asserts that no log entry contains the sentinel and that the error code is logged.changelog.d/1328-error-message-logs.security.mdand the knowledge recorddocs/knowledge/issues/1328.md.Verification
Encina.slnx: 0 warnings, 0 errors.dotnet format --verify-no-changesclean on the changed files.changelog-fragments --checkandknowledge-records --checkOK.error.Message,Activity.SetStatusandSetTag, plus the health checks. No remaining sink was found. The remaining hits put the message into a thrown exception, which is the ROP boundary, or into a newEncinaErrorreturned asLeft. TwoActivitySource.Failedhelpers that take a message have no caller.error.Message, which contains the sentinel, so every test fails on the old code by construction. The reviewer reached the same conclusion independently.Cross-cutting checklist (ADR-018)
Security fix at existing log call sites. Structured logging is integrated: the templates now carry the error code only, and EventIds are unchanged. The other eleven functions are not applicable, because no new entity, store, behavior or integration is added.
Fixes #1328
Summary by CodeRabbit