chore(sonar): burn down 32 Roslyn/analyzer smells across API + tests - #394
Conversation
Clears every open external_roslyn / analyzer smell in the #243 SonarCloud-to-zero campaign (the roslyn-b slice), all behavior-preserving: - CA1068 (3): move CancellationToken to the last parameter on IUserStreakService.RecalculateAsync and AiCompletionClient.CompleteText/JsonAsync; update every call site (production + tests). - CA1873 (4): hoist the expensive log argument into a local inside an IsEnabled guard (AdminController, WaitlistController, MinimumVersionMiddleware, AiUsageSummaryService). - CA1862 (3): StringComparison.OrdinalIgnoreCase in SyncControllerMutations; the two EF query predicates (SetHandleCommand, FriendGraphService) keep ToLower() under a scoped, URL-linked suppression because EF Core cannot translate string.Equals(StringComparison). - CA2016 (3): forward the CancellationToken in IdempotencyBehaviorDbTests handlers. - CA2263 (3): use the generic TypeAssertions.Be<T>() overload in job-enqueue tests. - CA1822 (1): mark ProcessUserChatCommand.BuildResponseCards static. - CA1869 (1): cache JsonSerializerOptions in GetPublicProfileQueryHandlerTests. - SYSLIB1045 (1): [GeneratedRegex] partial method in McpToolsDoNotDispatchCommandsTests. - CS0618 (1): GetDeclaredQueryFilters() replaces obsolete GetQueryFilter(). - xUnit1042 (7): TheoryData<> member-data return types across OrbitDbContextTests, SyncChangesIndexConfigurationTests, ScheduledJobRegistryTests. - ASP0015 (2): Headers.AcceptLanguage property in SubscriptionControllerTests. - ASP0025 (1): AddAuthorizationBuilder in ServiceCollectionExtensions. - RS1038 (2): split code-fix providers into a new Orbit.Analyzers.CodeFixes assembly so the analyzer assembly no longer references Microsoft.CodeAnalysis.Workspaces. Verified: dotnet build 0 errors + 0 target analyzer warnings, dotnet test green (5284), EF has-pending-model-changes = false. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #394 — chore(sonar): burn down 32 Roslyn/analyzer smells
Scope: 58 changed files (chore/api-sonar-roslyn-b → main)
Recommendation: APPROVE
Summary
Mechanical, behavior-preserving SonarCloud/Roslyn cleanup: 32 analyzer smells (CA1068, CA1873, CA1862, CA2016, CA2263, CA1822, CA1869, SYSLIB1045, CS0618, xUnit1042, ASP0015, ASP0025, RS1038) across production code, tests, and a new Orbit.Analyzers.CodeFixes project split out to satisfy RS1038. The riskiest change — the IUserStreakService.RecalculateAsync / AiCompletionClient.CompleteText/JsonAsync parameter reorder (CancellationToken moved to last) — was traced across every call site in src/ and tests/, not just the diff: all are consistent, and the three Orbit.Infrastructure/Services/Ai*SuggestionService.cs callers outside the diff already used named cancellationToken: arguments, so the reorder doesn't silently break them. No DTO, Controller route, or packages/shared contract surface changed.
Findings
Critical: None
High: None
Medium: None
Low / Info
- The CA1862 pragma suppressions in
SetHandleCommand.cs:26-33andFriendGraphService.cs:22-28are correctly justified with a WHY-URL note (https://github.com/dotnet/efcore/issues/1222), satisfying the comment-policy exception (CLAUDE.md dimension 4 /ORBIT0001). Directory.Build.props's analyzer-reference condition correctly excludes bothOrbit.Analyzersand the newOrbit.Analyzers.CodeFixesfrom self-referencing; every consuming project (includingtests/Orbit.Analyzers.Tests) picks up both — no dangling reference.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — no auth/authz semantic change from the AddAuthorizationBuilder() migration, no new data exposure from IsEnabled(LogLevel.Information) guards, no injection risk from .ToLower() EF predicates (translates to Postgres lower(), not raw SQL) |
| contract-aligner | N/A — diff touches no DTO, Controller route, or packages/shared type; sibling orbit-ui-mobile repo not checked out in this job |
Validation
Build / Unit Tests / SonarCloud run as separate required CI checks on this PR (per workflow instructions, not re-executed in this review session). PR body states dotnet build Orbit.slnx: 0 errors, 0 target analyzer warnings, 5284 tests passed / 0 failed (author-reported).
What's good
- The parameter reorder is fully swept — every call site in the repo (diff and non-diff) is consistent.
- CA1862 workarounds are properly justified with an upstream-issue WHY-URL rather than silently suppressed.
- Test changes mirror production changes 1:1.
- The
Orbit.Analyzers.CodeFixesproject split cleanly removes theMicrosoft.CodeAnalysis.Workspacesdependency from the shipped analyzer assembly.
Recommendation
Clean to merge. No Critical/High/Medium findings.


Clears every open
external_roslyn/ analyzer smell in the #243 SonarCloud-to-zero campaign (the failed roslyn-b night-run slice). All changes are behavior-preserving; locations were derived from an authoritativedotnet buildwith the target rules elevated (SonarCloud's line numbers were partly stale, and the build surfaced one extra xUnit1042 SonarCloud had missed).Rules cleared (32 total)
CancellationTokenmoved to last param onIUserStreakService.RecalculateAsync+AiCompletionClient.CompleteText/JsonAsync; every call site updated (production + tests)IsEnabledguard (AdminController,WaitlistController,MinimumVersionMiddleware,AiUsageSummaryService)StringComparison.OrdinalIgnoreCaseinSyncControllerMutations; the two EF-query predicates (SetHandleCommand,FriendGraphService) keepToLower()under a scoped, URL-linked suppression because EF Core cannot translatestring.Equals(StringComparison)to SQL (efcore#1222)CancellationTokeninIdempotencyBehaviorDbTestshandlersTypeAssertions.Be<T>()overload in job-enqueue testsProcessUserChatCommand.BuildResponseCardsstaticJsonSerializerOptionsinGetPublicProfileQueryHandlerTests[GeneratedRegex]partial method inMcpToolsDoNotDispatchCommandsTestsGetDeclaredQueryFilters()replaces obsoleteGetQueryFilter()TheoryData<>member-data return types (OrbitDbContextTests,SyncChangesIndexConfigurationTests,ScheduledJobRegistryTests)Headers.AcceptLanguageproperty inSubscriptionControllerTestsAddAuthorizationBuilderinServiceCollectionExtensionsOrbit.Analyzers.CodeFixesassembly so the analyzer assembly no longer referencesMicrosoft.CodeAnalysis.WorkspacesVerification (foreground)
dotnet build Orbit.slnx: 0 errors, and (with the target rules elevated to warning) 0 remaining target analyzer warnings.dotnet test Orbit.slnx: green — 5284 passed / 0 failed (incl. the 7 analyzer/code-fix tests that validate the RS1038 split).dotnet ef migrations has-pending-model-changes: false (model untouched).//narration added (ORBIT0001); the two suppression WHY notes link an upstream URL.Refs thomasluizon/orbit-ui-mobile#243