refactor(api): dedup AI tools/validators/schedulers/auth to ~0% (#243)#404
Conversation
Behavior-preserving duplication burn-down for orbit-api (part of #243, "Duplicated code → 0%"). Same outputs, error strings, and ordering throughout; verified by the full unit suite (5454 green). Extracted shared logic: - Bulk validators: BulkLog/BulkSkip command validators (66.7% dup each) now derive from BulkHabitCommandValidatorBase<TCommand,TItem> over new IBulkHabitCommand/IBulkHabitItem marker interfaces. - Auth commands: VerifyCode/GoogleAuth share AuthUserProvisioning .FindOrCreateUserAsync (race-safe find-or-create); GoogleAuth keeps its welcome-email side effect, VerifyCode its caller-side welcome. - Bulk commands: BulkLog/BulkSkip handlers share BulkHabitLoader .LoadHabitsWithRecentLogsAsync (habit map + windowed logs). - Goal tools: DeleteGoal/UpdateGoal share GoalToolHelpers (goal-id parse, owner-scoped lookup, not-found guard), mirroring HabitToolHelpers. - Goal queries: GetGoalById/GetGoalDetail share GoalDetailLoader .BuildGoalDetailAsync (load + project GoalDetailDto). - Chat tools: Calendar/Notification tools share ChatToolMediator.RunAsync (the duplicated mediator-run-to-ToolResult helper). - AI habit tools: Bulk/Log/Skip tools share HabitToolHelpers schema builders; Log/Skip reuse the existing parse/not-found guards. - ChatController: ProcessChat/ProcessChatStream share ParseChatInputsAsync. - MCP HabitTools: BulkLog/BulkSkip share ExecuteBulkHabitOperationAsync. Justifiably excluded (sonar.cpd.exclusions, rationale in workflow): - CreateHabitTool/UpdateHabitTool: declarative LLM-facing JSON parameter schemas — overlapping fields but per-tool field sets and descriptions in a single anonymous object that cannot be composed without CPD-flagged mirror call sites or worse readability. - AgentCatalogService.UserDataCatalog: declarative user-data catalog of unique rows sharing a constructor shape (parity with its existing S107/S1192/CA1861 suppressions). - Slip/ProactiveCheckin schedulers: residual dup is per-class [LoggerMessage] source-generator partial-method boilerplate. Refs thomasluizon/orbit-ui-mobile#243 (duplication → 0) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #404 — thomasluizon/orbit-api
Recommendation: APPROVE
Summary
This is a behavior-preserving duplication burn-down: 27 files touched, extracting shared logic into 7 new internal helper types (AuthUserProvisioning, ChatToolMediator, GoalToolHelpers, GoalDetailLoader, BulkHabitLoader, IBulkHabitCommand/IBulkHabitItem, BulkHabitCommandValidatorBase) and updating call sites in auth commands, AI/MCP tools, goal queries, and bulk habit commands/validators. Every changed file was read in full (not just diff hunks) and each extraction diffed against the pre-refactor code it replaces. Owner-scoping predicates (UserId/goalId/habitId filters), the race-safe unique-violation retry in the new user-provisioning path, validation rules (empty/size-cap/per-item checks), and JSON tool schemas are all reproduced exactly, just parameterized. No endpoint routes, DTOs, MCP tool names, or [Authorize] annotations changed.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
[Info] New internal helper classes have no direct unit tests, but are already exercised end-to-end through existing handler/tool tests
· dimension: Backend hard rules (#13, Tests)
· location: src/Orbit.Application/Auth/Commands/AuthUserProvisioning.cs, src/Orbit.Application/Goals/Queries/GoalDetailLoader.cs, src/Orbit.Application/Habits/Commands/BulkHabitLoader.cs, src/Orbit.Application/Chat/Tools/Implementations/ChatToolMediator.cs, GoalToolHelpers.cs
· issue: These are pure extractions of previously-tested code into shared internal statics; no new behavior was introduced.
· risk: None currently — GoogleAuthCommandHandlerTests.cs:162 and VerifyCodeCommandHandlerTests.cs:143 already cover the DbUpdateException unique-violation race path (both construct a FakeUniqueViolationException and assert the retry), and the goal/bulk-habit/tool tests cover the rest indirectly. PR body confirms 5454/5454 tests still pass.
· fix: No action required now; if these helpers grow additional branches later, add direct unit tests at that point rather than relying only on caller coverage.
· reference: tests/CLAUDE.md "Every new feature needs... unit tests"
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — reviewed extraction of AuthUserProvisioning.FindOrCreateUserAsync, GoalToolHelpers.FindGoalAsync, BulkHabitLoader.LoadHabitsWithRecentLogsAsync, BulkHabitCommandValidatorBase, ChatController.ParseChatInputsAsync, and HabitTools.ExecuteBulkHabitOperationAsync. Owner-scoping predicates, the DbUpdateException unique-violation retry, [Authorize]/rate-limit/size-limit attributes, and validator rules all preserved exactly. No new logging of secrets/PII, no new raw SQL. No findings. |
| contract-aligner | N/A — no DTO, Controller route, or packages/shared type changed (internal refactor only; MCP tool names bulk_log_habits/bulk_skip_habits unchanged, ChatController route unchanged). Not verifiable in CI (orbit-ui-mobile not checked out), but diff scope confirms no consumer-facing surface changed. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — CI runs Build as a separate required check; PR description reports dotnet build Orbit.slnx 0 errors |
| Tests (dotnet) | N/A — CI runs Tests as a separate required check; PR description reports 5454 passed, 0 failed |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A — no
apps/*UI files in this diff (backend-only repo). - Dimension 9 (Parity) / Dimension 10 (i18n): N/A — frontend-only dimensions owned by the orbit-ui-mobile side; this diff has no mobile/web surface.
- Dimension 14 (FEATURES.md parity): N/A — pure refactor, no user-facing feature added/changed/removed; all AI-tool and MCP-tool names, descriptions, and schemas are byte-identical to before.
- Cross-model
/second-opinionstep: not triggered — no Critical findings survived to invoke it. - All 27 changed files were read in full and given a verdict; nothing was skipped for size.
What's good
- Each extraction was verified against SonarCloud's
/api/duplications/showper the PR body, and thesonar.cpd.exclusionsadditions are all real, justified files (verified:CreateHabitTool.cs,UpdateHabitTool.cs,AgentCatalogService.UserDataCatalog.cs,SlipAlertSchedulerService.cs,ProactiveCheckinSchedulerService.csall exist). AuthUserProvisioningcleanly merges two previously near-identical, separately race-safe implementations while preserving each caller's behavioral difference via parameters (GoogleAuth's inside-flow welcome email fired onIsNewvs VerifyCode's caller-side one andemail.Split('@')[0]name derivation).IBulkHabitCommand<TItem>/IBulkHabitItemis a minimal, well-scoped generic contract — exactly the "third real use" bar the DRY rule asks for.- No narration comments introduced; new types carry proper
/////** */XML-doc.
Recommendation
Approve as-is. Low-risk, thoroughly self-verified refactor with no behavior, contract, or security changes detected.
There was a problem hiding this comment.
Code Review: PR #404 — thomasluizon/orbit-api
Recommendation: APPROVE
Summary
This PR's own content diff (27 files, the behavior-preserving SonarCloud duplication burn-down) is byte-for-byte identical to what was already reviewed in a prior pass, which reported no Critical/High findings. The only new commit since then, 965620c (merge from main), merges in unrelated main-branch commits (PR #403's StripeRetryPolicy/HabitScheduleService bounded-query fix) and does not touch this PR's own diff surface.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
The new internal helper types (AuthUserProvisioning, GoalDetailLoader, BulkHabitLoader, ChatToolMediator, GoalToolHelpers) have no direct unit tests but are pure extractions already exercised end-to-end via existing handler/tool tests (5454/5454 passing per PR body). No action required.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS (from prior full pass; diff unchanged) |
| contract-aligner | N/A — no DTO/route/contract surface touched |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — CI runs as separate required check |
| Tests (dotnet) | N/A — CI runs as separate required check |
Deferred — N/A dimensions
- Cross-repo dimensions (contract drift, mobile-side
contract-aligner) — not verifiable in this CI job; siblingorbit-ui-mobilerepo is not checked out. No contract surface touched anyway. - Parity / i18n / FEATURES.md — N/A, backend-only internal refactor.
What's good
Owner-scoping predicates, the race-safe unique-violation retry, validator rules, and JSON tool schemas were reproduced exactly across all seven extractions, with zero route/DTO/[Authorize] changes.
|
Drives orbit-api SonarCloud duplicated_lines_density 0.1% -> 0.0% (#404 deferred these files to avoid a concurrent-edit conflict). Real extractions (genuinely-shared logic): - HabitScheduleService: the identical active-days filter + frequency-unit switch tail of IsHabitDueOnDate and IsHabitHistoricallyDueOnDate is extracted into one private MatchesFrequency helper (self-dup ~24 lines). - LevelDefinitions.SyncLevel(User): the GetLevelForXp + SetLevel level-sync idiom, previously copy-pasted in 6 places (both accept commands, SendCheer, ChallengeProgressService, JoinChallenge, and GamificationService.UpdateLevel), is unified into one canonical helper; UpdateLevel is deleted. Justified exclusion (irreducible parallel mirror): - AcceptAccountabilityPairCommand / AcceptFriendRequestCommand: after the shared level-sync is extracted, the only residual is the BuildRequesterNotification parallel mirror -- a null-guard + isPortuguese ternary skeleton emitting DISTINCT localized copy for two independent accept events in separate bounded contexts. CPD normalizes the differing string literals so the skeletons match; a shared builder would couple the two contexts without improving readability. Added to sonar.cpd.exclusions with rationale, consistent with the existing #404 exclusions. Behavior-preserving: build 0 errors, all tests green, no EF model changes, openapi.json unchanged. Refs thomasluizon/orbit-ui-mobile#243 (duplication -> 0) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
Behavior-preserving duplication burn-down for orbit-api, driving SonarCloud
duplicated_lines_densityfrom 1.0% toward ~0% (part of thomasluizon/orbit-ui-mobile#243, "Duplicated code → 0%"). Same outputs, error strings, and execution order throughout.Each culprit was verified against SonarCloud's authoritative
/api/duplications/showbefore deciding extract vs. exclude.Extracted (real shared logic)
BulkHabitCommandValidatorBase<TCommand,TItem>+IBulkHabitCommand/IBulkHabitItemAuthUserProvisioning.FindOrCreateUserAsync(race-safe find-or-create)BulkHabitLoader.LoadHabitsWithRecentLogsAsyncGoalToolHelpers(parse + owner lookup + not-found guard)GoalDetailLoader.BuildGoalDetailAsyncChatToolMediator.RunAsync(removed 3 copy-pasted private helpers)HabitToolHelpers.BulkHabitActionSchema/SingleHabitDateSchema; Log/Skip reuse existing parse/not-found guardsProcessChat/ProcessChatStreamParseChatInputsAsyncHabitToolsBulkLog/BulkSkipExecuteBulkHabitOperationAsyncGoogleAuth keeps its inside-flow welcome email (now fired on
IsNew); VerifyCode keeps its caller-side welcome. Orphanedusings removed as they fell out.Justifiably excluded (
sonar.cpd.exclusions, rationale inline in the workflow)UserDataFieldDescriptorconstructor shape (parity with the file's existingS107/S1192/CA1861suppressions).ScheduledServiceBase; residual dup is per-class[LoggerMessage]source-generator partial-method boilerplate the compiler requires in each class.Verification (all foreground)
dotnet build Orbit.slnx— 0 errors (warnings 10 → 7, from removed orphaned usings)dotnet test Orbit.slnx— 5454 passed, 0 faileddotnet ef migrations has-pending-model-changes— false (no entity/model touched)src/Orbit.Api/openapi.json— unchanged (checksum identical)//narration added (ORBIT0001 clean)Before / after duplication
Before:
duplicated_lines_density= 1.0% (project). This PR removes the extracted pairs from the count and excludes the three irreducible mirrors; the PR's SonarCloud run should show the drop.Refs thomasluizon/orbit-ui-mobile#243 (duplication → 0)