refactor(api): resolve S1144 EF setter smells via backing fields + cap ReminderTimes in domain

[thomasluizon]

What & why

Two changes, both scoped to Report / PendingClarification / Habit and their EF config + tests.

1. Resolve csharpsquid:S1144 "unused private setter" x2 (the correct, model-preserving way)

Report.ReviewedAtUtc and PendingClarification.ResolvedAtUtc carried a private set that is invoked only by EF via reflection (materialization; and, for ResolvedAtUtc, the SQL-layer ExecuteUpdate in PendingClarificationStore.MarkResolvedAsync). Sonar can't see the reflective writer, so it flags the setter as unused — a true false positive.

Fixed at the root without touching the DB mapping:

• Each property is now a read-only property over a private backing field (public DateTime? ReviewedAtUtc => _reviewedAtUtc;), so there is no private setter left for S1144 to flag.
• Each column is mapped explicitly in its config block — Property(x => x.Prop).HasField("_field") in ConfigureReportEntity / ConfigurePendingClarificationEntity. This is what prevents a repeat of chore(sonar): burn down csharpsquid smell cluster across API #389: EF convention drops read-only auto-properties, but an explicitly-configured property with a backing field stays mapped.
• The one-shot resolve still flips ResolvedAtUtc atomically at the SQL layer via ExecuteUpdate (SetProperty(x => x.ResolvedAtUtc, ...) translates the mapped property to a column, independent of the CLR accessor) — unchanged behavior.
• The backing fields are written only by EF, so CS0649 is suppressed locally with a URL-linked WHY note.

The relational model is byte-identical. HasField / access mode are model annotations, not schema, so dotnet ef migrations has-pending-model-changes reports no pending changes (verified with a full build). Column name, type (timestamp with time zone), and nullability are all unchanged. No migration added.

2. Cap ReminderTimes count in the domain (hardening, same PR)

The AI-tool path (CreateHabitTool / CreateSubHabitTool / UpdateHabitTool) calls Habit.Create / Habit.Update directly, bypassing the FluentValidation MaxReminderTimes (=10) cap that guards the HTTP/onboarding/bulk paths. There was no domain-layer guard on ReminderTimes count.

• Added HabitInvariants.ValidateReminderTimes (count <= DomainConstants.MaxReminderTimes), called from both Habit.Create and Habit.Update, mirroring ValidateScheduledReminders.
• DomainConstants.MaxReminderTimes already existed and is already the single source of truth — AppConstants.MaxReminderTimes = DomainConstants.MaxReminderTimes, so the Application validator stays in lockstep automatically.
• Verified no existing caller legitimately exceeds the cap (HTTP/onboarding/bulk are validator-capped; no test passes >10 to Habit.Create/Update expecting success).

Tests

• New domain tests: ==max passes and >max fails, for both Create and Update.
• Full suite green: Domain 512, Application 2786, Infrastructure 1992, Analyzers 7.
• dotnet ef migrations has-pending-model-changes = no pending changes.
• dotnet build Orbit.slnx = 0 errors; openapi.json unchanged.

Refs thomasluizon/orbit-ui-mobile#243, thomasluizon/orbit-ui-mobile#447

🤖 Generated with Claude Code

[claude]

Code Review: PR #396 (thomasluizon/orbit-api)

Scope: PR #396 — refactor(api): resolve S1144 EF setter smells via backing fields + cap ReminderTimes in domain
Recommendation: APPROVE

Summary

Two independent, small, well-scoped changes: (1) resolves a Sonar S1144 false-positive by converting Report.ReviewedAtUtc and PendingClarification.ResolvedAtUtc from EF-only private set auto-properties to read-only properties over explicitly-mapped (HasField) backing fields, and (2) closes a real validation gap by adding a domain-layer cap on Habit.ReminderTimes (mirroring the existing ValidateScheduledReminders pattern) so the AI-tool paths (CreateHabitTool/CreateSubHabitTool/UpdateHabitTool), which call Habit.Create/Habit.Update directly, can no longer persist more than 10 reminder times — previously only the HTTP path's FluentValidation MaxReminderTimes enforced this.

Verified: the new validation is wired into both Habit.Create and Habit.Update with correct short-circuiting; ReminderTimes: null ("keep existing"/"use default") is handled correctly and matches the existing ScheduledReminders convention; the backing-field EF pattern is standard and the CS0649 suppressions carry a valid WHY-with-URL note pointing at merged PR #390; the ExecuteUpdate-based one-shot resolve in PendingClarificationStore is unaffected since it translates the mapped property to its column independent of the CLR accessor.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

None — nothing concretely actionable to post.

Subagents

Agent	Verdict
security-reviewer	PASS — no Controllers/DTOs/endpoints touched; the ReminderTimes cap closes the AI-tool bypass with no residual bypass found (traced every Habit.Create/Habit.Update caller, including ReminderStoreNormalizer's merge path); the backing-field refactor has no concurrency/spoofing issue — PendingClarificationStore.MarkResolvedAsync's ExecuteUpdate with ResolvedAtUtc == null in the WHERE clause remains atomic.
contract-aligner	N/A — diff touches no DTO, Controller route, or packages/shared surface.

Validation

Check	Result
Build/Tests (dotnet)	N/A — skipped per instructions; CI runs Build/Unit Tests/SonarCloud as separate required checks. PR description states full suite green and dotnet ef migrations has-pending-model-changes reports no pending changes.

Deferred — N/A dimensions

• DESIGN.md/AI-slop, web↔mobile parity, i18n, FEATURES.md parity: N/A — backend-only bugfix/hardening diff with no new user-facing feature or UI surface.
• Contract drift + backward-compat: N/A — no DTO/Controller/shared-type changes; the ReminderTimes cap hardens an already-validated field, not a shape/field rename.
• Not verifiable in CI: sibling orbit-ui-mobile repo not checked out, but this diff touches no cross-repo surface.

What's good

• ValidateReminderTimes mirrors ValidateScheduledReminders exactly (same null-means-unchanged semantics, same AppError.Format error pattern), keeping the domain layer internally consistent.
• The EF backing-field fix is the correct root-cause resolution for the recurring S1144/EF-unmapping tension rather than another suppression band-aid.
• Test coverage is precise: both Create and Update, both boundary sides (==max passes, >max fails).
• Both #pragma warning disable CS0649 suppressions carry a proper WHY-with-URL note per the comment policy, scoped tightly to just the field declaration.

Recommendation

Merge. No Critical/High findings; the diff is small, root-causes both problems it claims to fix, and is fully covered by new unit tests.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Test body, will replace

[claude]

Code Review: PR #396 -- chore/api-ef-setter-smells-and-reminder-cap (correction)

Note: my previous review on this PR contained placeholder test text ("Test body, will replace") posted in error. This review supersedes it with the actual assessment.

Scope: PR #396 in thomasluizon/orbit-api
Recommendation: APPROVE

Summary:
Two independent, well-scoped changes: (1) resolves a true Sonar S1144 false-positive by converting Report.ReviewedAtUtc / PendingClarification.ResolvedAtUtc from get;private set; auto-properties to read-only properties over explicitly-mapped backing fields (HasField), keeping the relational model byte-identical; (2) adds a domain-layer HabitInvariants.ValidateReminderTimes cap (10) called from both Habit.Create and Habit.Update, closing a real validation-bypass gap where AI-tool paths called the domain factory/mutator directly and skipped the FluentValidation-only cap. All 7 changed files were reviewed in full diff context; an independent security-review pass confirmed the ExecuteUpdate WHERE-guards and the EF field-mapping are unchanged in behavior. No Critical or High findings.

Findings:
Critical: None.
High: None.
Medium: None.
Low/Info: Report.ReviewedAtUtc has no writer anywhere in application code (pre-existing, out of this diff scope; noted for awareness only).

Security review:
PASS. PendingClarificationStore.MarkResolvedAsync retains its atomic WHERE guards (Id, UserId, ResolvedAtUtc == null, ExpiresAtUtc > now) via ExecuteUpdateAsync -- the one-shot/not-expired semantics are unchanged. EF HasField mapping is the only writer for both ResolvedAtUtc and ReviewedAtUtc, matching prior private-setter behavior exactly. The ReminderTimes cap is now the single choke point all callers go through, including AI tool paths (CreateHabitTool, CreateSubHabitTool, UpdateHabitTool, BulkUpdateHabitEmojisTool, ApplyOnboardingCommand, BulkCreateHabitsCommand) that previously bypassed the FluentValidation-only cap -- this closes a real gap rather than introducing one.

Validation:
Build/Tests: N/A here -- CI runs Build/Unit Tests/SonarCloud as separate required checks. PR description reports full suite green and no pending EF model changes.

What is good:

• Root-cause fix (unwritable-in-C# property + explicit field mapping), not a suppression.
• ReminderTimes cap mirrors the existing ValidateScheduledReminders pattern exactly.
• Update() existing validation still runs fully before mutation -- no partial-apply risk introduced.
• New tests cover both boundary conditions (==max success, >max failure) for both Create and Update.
• CS0649 suppressions carry URL-linked WHY notes per the comment policy.
• AppConstants.MaxReminderTimes = DomainConstants.MaxReminderTimes keeps Application and Domain caps in lockstep.

Recommendation:
Merge as-is. No blocking issues; the ReviewedAtUtc Info note is pre-existing and out of scope.