chore(sonar): burn down csharpsquid smell cluster across API - #389
Merged
Conversation
Behavior-preserving fixes for 11 SonarCloud csharpsquid rules: - S1192 (7): extract repeated string literals to private consts (ResendEmailService heading/intro/footer, ProfileController enabled/disabled via ToggleLabel, NotificationTools update_notifications, OrbitDbContext '[]'::jsonb reuse the existing const). - S3267 (10): simplify filtering loops to LINQ Where/Any/OfType/FirstOrDefault. - S3358 (7): extract nested ternaries to statements/locals. - S108 (3): document intentionally-empty catch blocks (URL-linked WHY). - S1144 (3): delete unused private setters/field. - S2589 (3): drop always-true/false conditions. - S2325 (2): mark helper methods static. - S1172 (1): remove unused cancellationToken param + caller. - S1075 (1): move the Google OAuth token URL to Google:TokenUrl config. - S3260 (1): seal the private UserDatePreferences record. - S4581 (1): use Guid.Empty instead of default(Guid). Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review — #389 chore(sonar): burn down csharpsquid smell cluster across API
Recommendation: APPROVE
Severity counts — Critical (incl. old-client breaks): 0, High: 0, Medium: 0, Low/Info: 1
Summary:
Behavior-preserving SonarCloud csharpsquid smell burndown across 35 files (+105/-118): loop-to-LINQ refactors, extracting repeated string literals to consts, de-nesting ternaries, documenting previously-empty catch blocks with WHY/URL comments, removing dead unused members, marking two methods static, removing unused parameters, moving the Google OAuth token URL from a hardcoded const to IConfiguration, sealing a private record, and replacing default(Guid) with Guid.Empty. All 35 changed files were traced line-by-line against their diffs; every fix checks out as genuinely behavior-preserving with no functional regression.
Security pass:
- GoogleTokenService.cs: _googleTokenUrl is bound from configuration["Google:TokenUrl"]! at construction time, sourced only from appsettings.json/env vars, never from request input. No SSRF/config-injection risk. The null-forgiving ! is consistent with the existing Google:ClientId/Google:ClientSecret reads in the same file.
- Three newly-documented empty catches, none swallow anything security-relevant: ConcurrencyRetry.cs SaveWithRetryAsync retries only when (attempt less than maxAttempts), final-attempt failure still propagates to ConcurrencyExceptionHandler resulting in HTTP 409; GoogleTokenService.cs ParseErrorResponse catches malformed JSON from Google's error body and falls through to the generic TransientFailure/logged-warning path (the auth-relevant invalid_grant/unauthorized_client classification happens before this catch is reached, so nothing is masked); ChatController.cs OperationCanceledException swallow is benign (client disconnect).
- LINQ refactors: SyncControllerMutations.cs ApplyEntityMutationAsync still builds the ownedById predicate per call-site with x.Id == mutation.Id and x.UserId == userId; ownership scoping is preserved, the static extraction didn't drop userId from the predicate.
- NotificationTools.cs: the extracted UpdateNotificationsTool const is just a string label; ClaimsPrincipal user (and thus per-request user-id scoping via McpExecutorBridge/IAgentOperationExecutor) is still forwarded on every call site. No scoping regression.
No Critical/High/Medium findings.
Cross-repo contract:
No endpoint/DTO shape changes in this diff (internal refactor only), the orbit-ui-mobile consumer-parity contract is not implicated. The sibling repo is not checked out in this CI job, so this is asserted from diff content rather than independently cross-checked; noted for transparency, not as a blocking concern.
Verdict:
Clean, low-risk mechanical cleanup. No blocking issues found, approving.
thomasluizon
added a commit
that referenced
this pull request
Jul 13, 2026
…tartup PendingModelChangesWarning crash) (#390) #389's S1144 smell burn-down removed the `private set` from two EF-mapped entity properties -- Report.ReviewedAtUtc and PendingClarification.ResolvedAtUtc -- turning them into read-only auto-properties. EF Core's property-discovery convention does not map read-only auto-properties, so the runtime model dropped both columns while OrbitDbContextModelSnapshot.cs still declared them. The startup DB-migrate step is configured to throw on PendingModelChangesWarning, so every orbit-api deploy since #389 crashes (exit 139) at WebApplicationExtensions.ConfigureOrbitPipeline. Restoring the private setters re-maps both columns with ZERO schema change (no new migration, no snapshot edit): `dotnet ef migrations has-pending-model-changes` returns false. Adding a migration instead would DropColumn both live production columns (data loss), which #389 never intended. The setters are unused from C# (EF sets them via reflection), so a URL-linked WHY note keeps a future S1144 pass from re-removing them and re-breaking the deploy. Refs #389, thomasluizon/orbit-ui-mobile#243 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thomasluizon
added a commit
that referenced
this pull request
Jul 14, 2026
…p ReminderTimes in domain (#396) Resolve two csharpsquid:S1144 "unused private setter" findings that were true false positives (the setters were invoked only by EF via reflection): - Report.ReviewedAtUtc and PendingClarification.ResolvedAtUtc are now read-only properties (=> _backingField) over private backing fields, so no private setter exists for Sonar to flag. - Each column is mapped EXPLICITLY in its IEntityTypeConfiguration block via Property(x => x.Prop).HasField("_field"), so EF convention cannot drop it the way it dropped read-only auto-properties in #389. The relational model is byte-identical: `dotnet ef migrations has-pending-model-changes` reports no pending changes. PendingClarification's one-shot resolve still flips the column atomically at the SQL layer via ExecuteUpdate (unchanged). - The backing fields are written only by EF (materialization / ExecuteUpdate), so CS0649 is suppressed locally with a URL-linked WHY note. Hardening (same PR): the AI-tool path calls Habit.Create/Habit.Update directly, bypassing the FluentValidation MaxReminderTimes (=10) cap. Added HabitInvariants.ValidateReminderTimes (count <= DomainConstants.MaxReminderTimes, already the single source of truth referenced by AppConstants.MaxReminderTimes) and call it from both Habit.Create and Habit.Update, mirroring ValidateScheduledReminders. Added domain tests: ==max passes, >max fails, for both Create and Update. No existing caller legitimately exceeds the cap (HTTP, onboarding, and bulk paths are already validator-capped). Refs thomasluizon/orbit-ui-mobile#243, thomasluizon/orbit-ui-mobile#447 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for 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.
Behavior-preserving burn-down of the SonarCloud csharpsquid smell cluster (orbit-api, key
thomasluizon_orbit-api). Every fix is at source and preserves behavior — no#pragma/NOSONARgaming.Rules fixed (all open instances → 0)
private const(or reuse existing const).Where/Any/OfType/FirstOrDefault.Report.ReviewedAtUtc,PendingClarification.ResolvedAtUtc) + unused field (GoogleCalendarReconnectMessage).TrySaveBatchAsync/ApplyEntityMutationAsyncstatic.cancellationTokenparam fromHandlePostLogin+ caller.Google:TokenUrlconfig (matches the existingGoogle:ClientId/ClientSecretpattern).UserDatePreferencesrecord.Guid.Emptyinstead ofdefault(Guid).Total: 39 issues across 11 rules.
Notes
ReviewedAtUtc,ResolvedAtUtc) remain EF-materializable via their backing fields — the removed setters were never called from C#.S1075config move addsGoogle:TokenUrltoappsettings.json(base, always loaded) + bothGoogleTokenServicetest configs; the value is unchanged (https://oauth2.googleapis.com/token).csharpsquidrules, so the authoritative confirmation is the SonarCloud PR scan (they do not surface in the local build).Verification
dotnet build Orbit.slnx— 0 errors, no new warnings.dotnet test— 5284 passed, 0 failed (Domain 512, Application 2773, Infrastructure 1992, Analyzers 7).Refs thomasluizon/orbit-ui-mobile#243