chore(nuclear-review): behavior-preserving structural review — orbit-api

[thomasluizon]

Thermo-nuclear code-quality review — orbit-api

Behavior-preserving structural review: split giant files, deduplicate boilerplate, move logic to its canonical layer. No behavior, public-API, route, status-code, DB-schema, EF-mapping, DI-graph/order, or serialized-output change. Technique bias: C# partial class (identical compiled type → zero behavior risk) + static helpers for pure logic — never new injected services, reflection, or new MediatR commands.

Paired with the orbit-ui-mobile review PR (chore/nuclear-review-ui) — cross-link added once it opens.

Applied

File	What	Before → After
Chat/Commands/ProcessUserChatCommand.cs	Handler split across 6 same-type partial files (Context / Ai / Tools / ToolResults / Persistence / Accumulator)	1227 → 275 (+6 partials, 101–331)
Habits/Queries/GetHabitScheduleQuery.cs	21 pure filter/projection helpers → static HabitScheduleFilters	752 → 391 (+370)
Api/Controllers/HabitsController.cs	14 request records → companion partial HabitsControllerRequests	684 → 556 (+136)
Chat/Tools/Implementations/PlatformTools.cs	9 tool classes → one file each	437 → 0 (9 files)
Api/Mcp/Tools/HabitTools.cs, GoalTools.cs	Cross-file dedup (GetUserId, JSON opts, line-formatting) → static McpToolHelpers	629 → 546 / 278 → 263 (+100)
Infrastructure/Services/AgentOperationExecutor.cs	Response construction → static AgentOperationResponseFactory	446 → 386 (+142)
Api/Extensions/ServiceCollectionExtensions.cs	Split into 4 same-type partial files by area; registration/middleware order unchanged	723 → 239 (+AiServices 175, Infrastructure 248, BackgroundJobs 86)
Gamification/Services/GamificationService.cs	6 pure achievement checks → static AchievementChecks	605 → 456 (+164)
Domain/Entities/Habit.cs, Goal.cs	Validation guards → static HabitInvariants / GoalInvariants (Goal: deduped Create/Update triplet)	530 → 450 (+94) / Goal +25
Api/Controllers/SyncController.cs	4 duplicated per-entity mutation methods → 1 generic ApplyEntityMutationAsync (partial; Expression<Func> keeps SQL server-side)	497 → 420 (+62)
5× Infrastructure/Services/*SchedulerService.cs	Identical periodic loop → abstract ScheduledServiceBase; per-service log text / EventIds / intervals preserved	−~40 each (+49 base)

44 files, +3295 / −3077.

Deferred — complete ledger

Every remaining top-inventory offender, each with a concrete, verified reason:

File / area	LOC	Why deferred
Services/AgentCatalogService.Capabilities.cs	959	Data-heavy declarative capability catalog (static product declarations, no algorithmic logic). Already cleanly split into partials by main commit 449357d; splitting static data further adds no structural value.
Persistence/OrbitDbContext.cs	705	EF model snapshot must stay byte-identical. Moving inline config to IEntityTypeConfiguration<T> is standard, but the encryption service threading through nullable params is a runtime-parameterized invariant — safe only behind a dedicated migration + snapshot-diff verification task, outside this pass's envelope.
Domain/Models/AgentContracts.cs	402	Data-heavy record/DTO declarations encoding the single unified agent-authorization contract; size is domain breadth, not a structural smell. Splitting reduces cohesion (negative ROI).
ai-services (AiController 443, AiIntentService 436, AiSummaryService 363, AiRetrospectiveService 253)	—	The targeted "duplicated prompt helpers" do not exist: StripMarkdownFences / CapToSentence each have one definition in AiSummaryService and are already shared by reference; extracting them would delete them from the type the tests reference (public-surface break) = premature abstraction. Left unchanged.
Api/Controllers/AuthController.cs	440	Under the 700 threshold + security boundary. Genuinely-shared pieces were already factored out (BuildOperationResponse, RecordDirectAuthAuditAsync); remaining per-action divergence (400-vs-401 status, value presence, audit args) means a generic helper would hide a security-relevant status decision behind a parameter. Left unchanged.
Api/Extensions/WebApplicationExtensions.cs	475	Already under 700 and well-factored; splitting would be churn with no LOC benefit.
Domain/Entities/User.cs	475	Email validation is bound to a source-gen [GeneratedRegex] partial on the type (moving it changes the generated surface); the remaining guards are single-caller inline (no duplication to DRY).

Verification

• dotnet build Orbit.slnx -c Debug: 0 errors (pre-existing warnings only — CS9057 analyzer-version mismatch + one CS8602 in a test file).
• dotnet test: 3724 passed, 0 failed (Application 2134 · Domain 391 · Infrastructure 1199) — identical to the clean-main baseline.
• dotnet ef migrations has-pending-model-changes --project src/Orbit.Infrastructure --startup-project src/Orbit.Api: "No changes have been made to the model since the last migration."

🤖 Generated with Claude Code

[claude]

Code Review: PR #253

Scope: PR #253 — chore(nuclear-review): behavior-preserving structural review — orbit-api
Recommendation: APPROVE

Summary

This PR is a large behavior-preserving structural refactor: 44 files, +3295/−3077, zero EF migrations, zero public-API changes. It splits giant files using partial class, extracts pure static helper classes, and deduplicates identical background-service loops into ScheduledServiceBase. The approach is sound — partial class is the lowest-risk decomposition technique in C# because the compiler merges the parts into a single compiled type. All authorization, validation, and domain guards are preserved verbatim in the extracted helpers. No Critical or High findings.

Findings

Critical

None.

High

None.

Medium

[MEDIUM] ParseGuidCsv throws on malformed GUID input rather than returning a user-facing error
· dimension: Security — Error handling (#12)
· location: src/Orbit.Api/Mcp/Tools/McpToolHelpers.cs:30-32
· issue: value.Split(',', …).Select(Guid.Parse) throws FormatException on any malformed GUID in a CSV argument. The callers (bulk_delete_habits, bulk_log_habits, bulk_skip_habits, link_goals_to_habit) do not catch this, so a bad GUID in an MCP session propagates as an unhandled exception rather than a clean tool error the model can self-correct on.
· risk: Any AI-supplied or user-supplied malformed GUID surfaces as a 500/unhandled exception rather than a clean error message. Not process-crashing — the MCP SDK serializes it — but it breaks the clean-error contract the rest of the tool layer follows and is worth fixing before more callers are added.
· fix: Replace Guid.Parse with Guid.TryParse and throw ArgumentException with a message:

public static List<Guid> ParseGuidCsv(string value)
{
    var results = new List<Guid>();
    foreach (var part in value.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries))
    {
        if (!Guid.TryParse(part, out var g))
            throw new ArgumentException($"'{part}' is not a valid GUID.");
        results.Add(g);
    }
    return results;
}

· reference: orbit-api CLAUDE.md "No workarounds / Root-cause every bug"; rubric dimension 12 — Error handling

Low / Info

None — signal gate applied; no actionable Low findings survive.

Subagents

Agent	Verdict
security-reviewer	PASS (1 medium, 3 low — folded above; no critical/high)
contract-aligner	N/A — no DTO, route, or packages/shared change in this diff

Validation

Check	Result
Build (dotnet)	N/A — skipped per --skip-build flag; PR body reports 0 errors
Tests (dotnet)	N/A — skipped per --skip-build flag; PR body reports 3724 passed, 0 failed

What's good

• Technique choice is correct. partial class is zero-behavioral-risk for decomposing large C# files — the compiler produces the same IL as if the code were in one file. Consistent, principled use throughout.
• ScheduledServiceBase is the right abstraction. Five scheduler services had identical ExecuteAsync scaffolding. The base class eliminates the duplication without introducing any new injected dependencies or reflection.
• HabitInvariants / GoalInvariants correctly scoped as internal static. Keeps domain invariants inside the entity assembly; Application and Infrastructure cannot call them directly, preserving the "all mutations through entity methods" invariant.
• AchievementChecks is pure static with no I/O. Extracting the six achievement-check methods makes GamificationService testable in isolation.
• AgentOperationResponseFactory eliminates a genuine DRY violation. Five near-identical response-construction blocks replaced with one-line factory calls.
• Authorization ownership correctly preserved. SyncControllerMutations.cs and HabitsControllerRequests.cs carry valid WHY-with-URL notes explaining the [Authorize] omission; the primary partials still carry the class-level attribute. All four ApplyEntityMutationAsync call sites include && h.UserId == userId.
• Deferred items are well-documented. The PR body contains a complete ledger of every file excluded with concrete reasons — exactly the right level of transparency for a refactor of this scope.

Recommendation

Approve and merge. The one Medium finding (ParseGuidCsv throwing on malformed GUID) is a pre-existing pattern the refactor preserved rather than introduced — it exists identically in McpInputParser.ParseGuid. Worth a follow-up fix but does not block merge.

🤖 Generated with Claude Code

[thomasluizon]

Paired UI review PR: thomasluizon/orbit-ui-mobile#311

[sonarqubecloud]

Quality Gate failed

Failed conditions
71.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #253

Scope: PR #253 — chore(nuclear-review): behavior-preserving structural review — orbit-api
Recommendation: APPROVE

Summary

PR #253 is a large behavior-preserving structural refactor covering 46 files (+3295/−3077 lines). It splits six giant files using C# partial class, extracts pure-static helpers from within classes, and collapses five near-identical scheduler loops into a shared ScheduledServiceBase. The public API contract, DI registration order, route paths, response DTOs, and serialized shapes are all unchanged — confirmed by the contract-aligner and security-reviewer subagents. The build passes (0 errors) and the full test suite passes (3724/0 pass/fail) per the PR body. One Low finding (member-visibility inconsistency) and one Info observation (test gap for new pure-static helpers) are noted below; neither blocks merge.

---

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

[Low] internal class + public methods on HabitInvariants and GoalInvariants

• dimension: SOLID / clean architecture (convention consistency)
• location: src/Orbit.Domain/Entities/HabitInvariants.cs:13-15, GoalInvariants.cs:10-12
• issue: Both helper classes are declared internal static class but every member is public static. public on members of an internal type is effectively internal — the compiler allows it, but the mismatch is misleading to readers who expect the access modifier to convey intent.
• risk: No runtime risk. A future extract of these types to a test helper or separate assembly would silently have narrower scope than public implies.
• fix: Change all method modifiers to internal to match the containing type, consistent with AgentOperationResponseFactory (also internal class).
• reference: CLAUDE.md clean-architecture conventions

[Info] No dedicated unit tests for extracted pure-static helpers

• dimension: Backend hard rules — tests
• location: AchievementChecks.cs, HabitScheduleFilters.cs, McpToolHelpers.cs, HabitInvariants.cs, GoalInvariants.cs, AgentOperationResponseFactory.cs, ScheduledServiceBase.cs
• issue: Seven pure-static helpers were extracted but no direct unit tests were added. Existing tests cover the behavior via the owning classes (GamificationServiceTests, command handler tests, scheduler service tests) — so there is no current coverage gap.
• risk: None immediately. These helpers are now independently testable. Future changes to a helper may regress behavior not caught at the owner-level granularity.
• fix: Consider adding targeted unit tests for AchievementChecks.CheckPerfectDay, HabitInvariants.ValidateDateOptions, HabitScheduleFilters.FilterScheduledHabits, etc. as follow-on work (not required for this merge).
• reference: CLAUDE.md "Every new feature needs unit tests"

---

Subagents

Agent	Verdict
security-reviewer	PASS
contract-aligner	MATCH (orbit-ui-mobile Zod side NOT VERIFIABLE — mobile repo not checked out)

---

Validation

Check	Result
Build (dotnet)	N/A — CI handles (PR body reports 0 errors)
Tests (dotnet)	N/A — CI handles (PR body reports 3724 passed, 0 failed)

---

What's good

• Technique is sound. C# partial class splits produce the identical compiled type — zero behavior risk and zero change to the DI graph, EF model, or serialized output. All claims verified.
• ScheduledServiceBase is the standout. Five near-identical scheduler loops correctly consolidated into a sealed override with proper OperationCanceledException pass-through and finally cleanup.
• SyncControllerMutations.ApplyEntityMutationAsync correctly keeps the userId ownership filter in the Expression<Func<TEntity, bool>> predicate (EF translates to SQL server-side). Security reviewer confirmed all four call sites preserve the filter verbatim.
• GoalInvariants.ValidateCoreFields correctly deduplicates the identical validation triplet in Goal.Create and Goal.Update — a genuine DRY win in the Domain layer.
• All XML-doc comments on new types follow the ORBIT0001-compliant contract-only form. The single surviving /// comment in HabitScheduleFilters.DetermineOverdueStatus is a valid WHY note citing the canonical service.
• Deferred-ledger discipline. The PR body lists every above-threshold file that was not touched and explains precisely why.

---

Recommendation

No Critical or High findings. The Low finding (visibility mismatch on internal/public member modifiers) can be a follow-up. APPROVE.

🤖 Generated with Claude Code