fix(api): derive habit DueDate from the user's timezone, not UTC (#243)

[thomasluizon]

Problem

Habit.Create() (Orbit.Domain/Entities/Habit.cs) defaulted a null DueDate to DateOnly.FromDateTime(DateTime.UtcNow). For any user whose timezone differs from UTC (e.g. UTC+13 already on the next calendar day, or UTC-5 still on the previous one) this produced the wrong "today", violating the hard timezone rule (user-facing dates MUST come from IUserDateService.GetUserTodayAsync). Because the fallback lived in Orbit.Domain, the csharp-tz hook that guards Orbit.Application couldn't reach it.

Fix

The domain factory can't inject services, so the date is pushed in from the Application layer:

• HabitCreateParams.DueDate is now a required, non-null DateOnly (5th positional param) and the UTC fallback is deleted. A future caller can no longer silently reintroduce the bug — omitting DueDate is now a compile error.
• All nine Habit.Create call sites already resolved the user's today via IUserDateService.GetUserTodayAsync(userId) (and passed it as DueDate: … ?? userToday); they now pass it as the required positional argument.

Behavior is identical for UTC users and corrected for everyone else.

Tests

• HabitTests.Create_UsesProvidedDueDateVerbatim_WithoutUtcSubstitution — factory persists the caller's date with no UTC substitution.
• HabitTests.Create_MonthlyHabit_AnchorsOriginalDayToProvidedDueDate — OriginalDayOfMonth anchors off the provided date, not UTC today.
• CreateHabitCommandHandlerTests.Handle_UserTimezoneAheadOfUtc_UsesUserTodayNotUtcToday — a user a day ahead of UTC gets their local today as DueDate, explicitly not UTC today (mocked IUserDateService).
• "No caller passes null" is now enforced by the type system.

All suites green locally: Domain 505, Application 2628, Infrastructure 1512.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

PR #330 Review — fix(api): derive habit DueDate from the user's timezone, not UTC

Recommendation: Request changes

Severity	Count
Critical	0
High	1
Medium	1
Low / Info	0

Summary

This PR correctly root-causes the original bug: HabitCreateParams.DueDate (src/Orbit.Domain/Entities/Habit.cs) is now a required, non-null DateOnly, and all nine Habit.Create call sites resolve the date via IUserDateService.GetUserTodayAsync (or propagate an existing entity's already-correct DueDate). New tests cover the timezone boundary, including a mocked-timezone-ahead-of-UTC case. Security review passed clean; there is no wire-contract change (backward-compat guard N/A — HabitCreateParams is never serialized).

Findings

[HIGH] Leftover UTC-fallback in HabitInvariants.ValidateDateOptions reintroduces the exact bug this PR claims to have closed

src/Orbit.Domain/Entities/HabitInvariants.cs:52

var effectiveDueDate = dueDate ?? DateOnly.FromDateTime(DateTime.UtcNow);

Untouched by this PR, in the sibling method called directly by both Habit.Create (Habit.cs:123-124) and Habit.Update → ValidateUpdateParams (Habit.cs:334-337). Verified: both current call sites always supply a non-null dueDate now (HabitCreateParams.DueDate is a required DateOnly; the Update path passes p.DueDate ?? DueDate where DueDate is the entity's own non-null property), so this is currently dead code, not a live bug today.

But it's a landmine: it's the exact "domain layer silently substitutes UTC-today" pattern this PR exists to kill, sitting one call away from reappearing — in Orbit.Domain, the exact layer the csharp-tz hook (scoped to Orbit.Application) can't reach, which is why this bug was hard to catch in the first place. It directly violates the explicit rule in src/Orbit.Domain/CLAUDE.md: "NEVER add a static DateTime.UtcNow call to a domain method that decides 'is this user's today'". It also makes the PR's implicit claim — that a future caller can no longer silently reintroduce the bug — not fully true.

Fix: Make ValidateDateOptions's dueDate parameter non-nullable DateOnly (both real call sites already guarantee non-null) and delete the fallback line — small, compile-checked, mirrors what was already done for HabitCreateParams.DueDate.

[MEDIUM] Create_NullDueDate_DefaultsToToday now tests the test helper, not Habit.Create

tests/Orbit.Domain.Tests/Entities/HabitTests.cs:161-169 (test), :16-35 (CreateValidHabit helper)

Habit.Create can no longer accept a null DueDate (compile error) — the exact "default to today" behavior this PR removed. The test now only proves the local test helper's own ?? DateOnly.FromDateTime(DateTime.UtcNow) fallback works, not the SUT — a stale, misleading assertion, superseded by the new Create_UsesProvidedDueDateVerbatim_WithoutUtcSubstitution (line 1436).

Fix: Delete or rewrite it; it can no longer test production null-handling since that path is now a compile-time impossibility.

Subagents

Agent	Verdict
security-reviewer	PASS — no findings; date resolution stays server-side/user-scoped, no authz/validation/data-exposure surface touched
contract-aligner	N/A — no DTO/Controller route/packages/shared type changed; only the internal, never-serialized HabitCreateParams became required

Validation

Build/Tests: N/A — skipped per CI adaptation (Build/Unit Tests/SonarCloud run as separate required checks on this PR).

Deferred

Dimensions for DESIGN.md, cross-repo parity, i18n, FEATURES.md — N/A, backend-only bugfix with no UI/feature-surface change. Cross-model second opinion not run (no Critical-severity finding to stress-test).

What's good

Correct root-cause approach (illegal state made unrepresentable at the type level), all 9 Habit.Create call sites verified correct, consistent positional-parameter reordering, targeted new tests for the exact timezone boundary, zero backward-compat risk.

Once the HabitInvariants.ValidateDateOptions gap is closed (small, low-risk) and the stale test is cleaned up, this is ready to merge.

[thomasluizon]

Addressed both review findings in 50a299d:

• [HIGH] HabitInvariants.ValidateDateOptions — made dueDate a required non-null DateOnly and deleted the ?? DateOnly.FromDateTime(DateTime.UtcNow) fallback. Both call sites (Habit.Create and Habit.Update→ValidateUpdateParams) already pass non-null, so this is compile-checked and completes the 'no silent UTC-today in the domain' guarantee.
• [MEDIUM] Deleted Create_NullDueDate_DefaultsToToday — with a required DueDate the null path is a compile-time impossibility; the test only exercised the helper's own fallback and is superseded by Create_UsesProvidedDueDateVerbatim_WithoutUtcSubstitution.

Local suites green: Domain 504, Application 2628, Infrastructure 1512.

[claude]

Review Complete

Scope: PR #330 in thomasluizon/orbit-api — diff since the last automated review (commit bb3606a → 50a299d)
Recommendation: APPROVE

Summary

The prior automated review (submitted CHANGES_REQUESTED) flagged two issues on this PR: a [HIGH] leftover UTC-fallback in HabitInvariants.ValidateDateOptions (src/Orbit.Domain/Entities/HabitInvariants.cs:52) that reintroduced the exact "domain silently substitutes UTC-today" pattern the PR was fixing, and a [MEDIUM] stale test (HabitTests.Create_NullDueDate_DefaultsToToday) that had become impossible to exercise once HabitCreateParams.DueDate went non-nullable. Commit 50a299d (fix(api): drop the sibling UTC fallback in ValidateDateOptions (#243)) addresses both directly and nothing else changed in this increment.

Verified independently:

• ValidateDateOptions's dueDate parameter is now non-nullable DateOnly; the ?? DateOnly.FromDateTime(DateTime.UtcNow) fallback line is deleted; both remaining endDate < dueDate comparisons use dueDate directly.
• Both call sites (Habit.Create at Habit.cs:123-124 passing the now-required HabitCreateParams.DueDate, and ValidateUpdateParams at Habit.cs:334-337 passing p.DueDate ?? DueDate where the entity's own DueDate is a non-null DateOnly) already guaranteed non-null — the signature change is compile-checked, not a behavior change.
• grep confirms no dangling reference to the deleted Create_NullDueDate_DefaultsToToday test anywhere in the repo, and no leftover DateTime.UtcNow in HabitInvariants.cs.
• The change is a clean net-negative diff (2 files, +2/-13) — pure fallback removal, no new logic.

Findings

Critical

None.

High

None. Prior High finding (leftover UTC fallback) is resolved.

Medium

None. Prior Medium finding (stale test) is resolved.

Low / Info

None.

Subagents

Agent	Verdict
security-reviewer	PASS — no findings; pure internal domain validator, no auth/HTTP/DTO surface
contract-aligner	N/A — no DTO/Controller route/packages/shared type changed in this increment

Validation

Check	Result
Build (dotnet)	N/A — skipped per caller instruction; CI runs Build/Unit Tests/SonarCloud separately
Tests (dotnet)	N/A — skipped per caller instruction

Deferred

• DESIGN.md/AI-slop, Parity, i18n — N/A, backend-only 2-file diff, no UI/feature-surface change.
• FEATURES.md parity — N/A, no user-facing feature surface changed.
• Backward-compat guard — N/A, HabitCreateParams/HabitInvariants are internal domain types, never serialized to packages/shared.
• Cross-model second opinion — not invoked; no Critical finding survived to trigger it.

What's good

Root-caused correctly: the fix is proportionate (5-line net diff) and closes the exact gap the prior review named, in the layer (Orbit.Domain) the csharp-tz hook can't reach. The author's PR comment (50a299d) explicitly maps each change back to the reviewer's finding, and the stale test was deleted rather than patched to fake-pass. Local suite counts reported green (Domain 504, Application 2628, Infrastructure 1512).

Recommendation

Approve. Both outstanding findings from the last automated review are fully and correctly resolved with no new issues introduced.

---

Severity	Count
Critical (incl. ⚠️ old-client breaks)	0
High	0
Medium	0
Low / Info	0

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

Review Complete

Scope: PR #330 in thomasluizon/orbit-api — re-verified after merge commit f6944ee (main merged into the branch). No new commits landed on the feature branch itself since the prior APPROVE; the merge only pulled in unrelated main history (refresh-token concurrency fix, #329) and does not touch any file in this PR's diff.

Summary: Confirms the prior automated review's APPROVE still holds. HabitCreateParams.DueDate is now a required non-nullable DateOnly; all call sites resolve it via IUserDateService.GetUserTodayAsync(userId) before construction, HabitInvariants.ValidateDateOptions no longer has the UTC-fallback that was flagged and fixed in 50a299d, and targeted domain/handler tests cover the user-timezone-ahead-of-UTC boundary.

Findings
Critical: None.
High: None.
Medium: None.
Low / Info: None.

Subagents

• security-reviewer: PASS — UserId in all touched commands/tools is always derived from the JWT-authenticated caller (HttpContext.GetUserId()), never request-body-supplied; HabitCreateParams/HabitInvariants are internal domain types never bound directly from HTTP; no auth, injection, or data-exposure issues.
• contract-aligner: N/A — no Controller or packages/shared-relevant DTO surface touches HabitCreateParams.

Validation

• Build (dotnet): N/A — skipped per caller instruction; CI runs Build/Unit Tests/SonarCloud separately.
• Tests (dotnet): N/A — skipped per caller instruction.

Deferred

• Backward-compat / cross-repo parity — N/A, HabitCreateParams and HabitInvariants are internal Orbit.Domain types, never serialized to packages/shared or exposed via a Controller/DTO.
• FEATURES.md / UI / i18n parity — N/A, backend-only change, no user-facing feature surface changed.
• Cross-model second opinion — not invoked; no Critical finding present to trigger it.

What's good: Both issues raised by the first automated review (leftover UTC fallback in HabitInvariants.ValidateDateOptions, and a stale now-unreachable test) were resolved cleanly in 50a299d and previously re-verified and approved. This pass confirms nothing has regressed since — the merge commit brought in unrelated upstream changes only.

Recommendation: Approve. No Critical/High/Medium findings; the PR's actual diff is unchanged since the last APPROVE.

Severity counts — Critical (incl. old-client breaks): 0 | High: 0 | Medium: 0 | Low/Info: 0