chore(api): quiet routine background-service logs to Debug; stop leaking AI exception detail#362
Conversation
…ing AI exception detail (#243) Reclassifies routine per-op progress and idempotency-guard log lines in the background services from Information to Debug so production logs carry only low-frequency business/lifecycle signal, per the CLAUDE.md logging-level policy: - AccountDeletionService: "Processing N scheduled deletions" -> Debug - ResendEmailService: "Skipping test email" / "Email sent" -> Debug - StreakFreezeAutoActivationService: freeze-already-activated + concurrent-skip idempotency guards -> Debug (genuine "freeze activated" stays Information) - HabitDueDateAdvancementService: "Advanced DueDate for N bad habits" -> Debug - OAuthController: "OAuth API key created" -> Debug - ProactiveCheckinSchedulerService: "already recorded; skipping push" -> Debug AiRetryLoggingPolicy no longer logs the raw exception message (which can carry implementation/network detail); it keeps the safe classification already present (exception type name / HTTP status + retriable flag). Adds a focused test asserting the safe format is logged without the raw exception message. Behavior-preserving observability change. Full suite green (4972 tests). Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #362 — chore(api): quiet routine background-service logs to Debug; stop leaking AI exception detail
Scope: PR #362 in thomasluizon/orbit-api
Recommendation: APPROVE
Summary
Behavior-preserving observability cleanup: routine per-op / idempotency-guard logs move from Information to Debug across five background services and one controller, and AiRetryLoggingPolicy stops logging the raw exception message (which could carry connection strings / internal detail), keeping only the safe exception-type/HTTP-status classification. A new unit test locks in the safe-classification behavior. The change matches the CLAUDE.md logging-level policy for every service except one debatable call site in OAuthController. No control-flow change; all 8 changed files were reviewed and given a verdict.
Findings
Critical: None.
High: None.
Medium:
[MEDIUM] "OAuth API key created" log demoted to Debug breaks the established audit-visibility pattern for API-key grants
· dimension: 13 (Backend hard rules — logging) / 12 (Security — data exposure/observability, defense-in-depth)
· location: src/Orbit.Api/Controllers/OAuthController.cs:325-326
· issue: LogOAuthApiKeyCreated (fired after a successful /oauth/token exchange that mints a new write-scope ApiKey with AgentScopes.ClaudeDefaultScopes for an external OAuth client such as Claude.ai) is moved from Information to Debug. Since appsettings.Production.json sets Logging:LogLevel:Default to Information, this event is now completely absent from production logs. The sibling event for user-initiated key creation, ApiKeysController.cs:74 (LogApiKeyCreated, "API key created {KeyId} by user {UserId}"), is untouched by this PR and stays at Information.
· risk: Every other path that grants a user's account write-scope API access remains observable in prod, but the OAuth-flow path (arguably higher-risk since it hands a key to a third-party client rather than the user's own tooling) goes dark — a blind spot for incident response / anomaly detection on external-client key grants.
· fix: Keep LogOAuthApiKeyCreated at Information (or Warning, given it grants external write access), consistent with ApiKeysController.LogApiKeyCreated. It's a low-frequency, per-user, per-integration event — it fits the CLAUDE.md "Information" bucket (business/lifecycle event), not the "routine per-operation success" Debug bucket the rest of this PR correctly targets.
· reference: root CLAUDE.md "Logging levels" hard rule; orbit-api/CLAUDE.md hard rule #13
Low / Info:
[INFO] All other Information → Debug demotions match the CLAUDE.md logging policy exactly — praise. Call sites: AccountDeletionService.cs:66 (processing-count progress), HabitDueDateAdvancementService.cs:108 (advanced-count progress), ProactiveCheckinSchedulerService.cs:197 (idempotency-guard hit), ResendEmailService.cs:153,182,249,272 (per-delivery send/skip), StreakFreezeAutoActivationService.cs:263,270 (idempotency-guard hits). Each is explicitly named in the PR's own rationale table and matches the policy's Debug bucket. The genuinely business-relevant events in the same files (Deleted deactivated account, stale-record cleanup summary, Auto-activated streak freeze, Sent proactive check-in) are correctly left at Information.
[INFO] AiRetryLoggingPolicy exception-detail removal (src/Orbit.Infrastructure/AI/AiRetryLoggingPolicy.cs:42-47) is a net security improvement with no diagnosability loss — the retained fields (exception type name or HTTP status, attempt number, retriable flag) are sufficient to diagnose retry storms without echoing raw exception text that can carry connection strings/hostnames/tokens. Covered by the new Send_TransportThrows_LogsSafeClassificationWithoutRawExceptionMessage test, which asserts the exact safe-format regex and that a planted secret string never appears.
Subagents
security-reviewer: PASS (no Critical/High; the one observability regression above is captured as a Medium finding)
contract-aligner: N/A — no DTO, Controller route, or packages/shared surface touched (pure log-level + one log-field change)
Validation
Build (dotnet): N/A — CI adaptation: Build runs as a separate required check in this workflow
Tests (dotnet): N/A — CI adaptation: Unit Tests run as a separate required check in this workflow; PR body reports 4,972 tests / 0 failures locally, including the new regression test
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop) — N/A, no apps/* files in this diff.
- Dimension 9 (Parity web↔mobile) — N/A, backend-only diff; not verifiable from this repo by design.
- Dimension 10 (i18n) — N/A, no new user-facing strings.
- Dimension 11 (Contract drift + backward-compat) — N/A, no DTO/schema/route change; Phase 5 guard has no candidates to classify.
- Dimension 14 (FEATURES.md parity) — N/A, pure observability change with no user-facing feature-surface change (PR body confirms no control-flow/behavior change).
- Phase 6 second-opinion — not applicable, no Critical finding survived to need cross-model corroboration.
- All 8 changed files were read in full and given a verdict — nothing deferred there.
What's good
- Every demoted log site matches the CLAUDE.md Debug-vs-Information policy precisely, with the PR body's own rationale table making the intent auditable call-site by call-site.
- Genuine business/lifecycle events were correctly left untouched at Information — this wasn't a blanket demotion.
- The AI-retry exception-detail fix closes a real data-exposure gap while preserving full diagnosability via the safe classification.
- The new regression test pins the safe-format contract with a regex assertion plus a planted secret string that must never appear.
- Narrow, single-purpose diff: no scope creep, no dead code left behind.
Recommendation
Approve. Address the Medium finding (revert OAuthController.LogOAuthApiKeyCreated to Information, or bump to Warning given it grants external write access) as a fast follow — it does not block this PR, since the rest of the diff is a correct, well-tested application of the logging policy.
Note: an earlier connectivity-check comment was posted on this PR while validating the review-posting mechanism; it carries no findings and can be disregarded — this review is authoritative.



What
Behavior-preserving observability cleanup (Batch 5 code-quality slice) applying the CLAUDE.md logging-level policy — Information is reserved for low-frequency business/lifecycle events; routine per-op progress and idempotency-guard hits belong at Debug.
Information → Debug
AccountDeletionServiceProcessing {Count} scheduled account deletionsResendEmailServiceSkipping email to test account/Email sentStreakFreezeAutoActivationServiceStreak freeze already activated; skipping+skipped due to concurrent updateHabitDueDateAdvancementServiceAdvanced DueDate for {Count} bad habitsOAuthControllerOAuth API key created(EventId 2)ProactiveCheckinSchedulerServicealready recorded; skipping pushGenuine business/lifecycle events are kept at Information:
Deleted deactivated account, stale-record cleanup summary,Auto-activated streak freeze,Sent proactive check-in. Each lowered call-siteIsEnabledguard was updated toLogLevel.Debugto match its message.AI retry log — stop leaking exception detail
AiRetryLoggingPolicyno longer logs the rawexception?.Message(which can carry connection strings / internal detail). It keeps the safe classification already present — exception type name (orHTTP {status}) plus the retriable flag. The trailingDetail: {Detail}field is removed.Tests
Added
Send_TransportThrows_LogsSafeClassificationWithoutRawExceptionMessage— asserts each retry log matches the safeAI attempt N failed (Type); retriable: boolformat and never contains the raw exception message. Full suite green: 4,972 tests, 0 failures.Scope
Pure observability change, no control-flow/behavior change. Touches only this slice's owned files.
StreakGoalSyncServiceintentionally untouched (owned by another slice).Refs thomasluizon/orbit-ui-mobile#243