fix(api): add boundary date rule to SkipHabitCommandValidator - #342
Conversation
The required-id rules (UserId/HabitId NotEmpty) already existed since the original endpoint, but the optional Date carried no boundary validation. Reject the default(DateOnly) sentinel at the trust boundary so a malformed zeroed date is caught before the handler. The timezone-dependent guards (future date, overdue window #326, scheduling) stay in the handler's domain guard because they need the user's resolved "today". Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Review Complete
Scope: PR #342 — fix(api): add boundary date rule to SkipHabitCommandValidator
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. old-client breaks) | 0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 2 |
Findings
Medium — Sentinel-date boundary rule added to SkipHabitCommandValidator (src/Orbit.Application/Habits/Validators/SkipHabitCommandValidator.cs:16-19) but not mirrored to the sibling BulkSkipHabitsCommandValidator (src/Orbit.Application/Habits/Validators/BulkSkipHabitsCommandValidator.cs), whose BulkSkipItem.Date is the identical optional DateOnly? field. Not a merge blocker: BulkSkipHabitsCommand.cs:86-88 (ProcessSkipItem) already re-checks the same overdue-window bound per item and fails gracefully, so no crash/data-loss risk — just a validation-boundary/DRY inconsistency between the two skip paths. Suggested follow-up: add the matching item.RuleFor(i => i.Date).NotEqual(default(DateOnly)).When(i => i.Date.HasValue) rule + a mirrored test.
Info: the plain-string WithMessage (no i18n key, no ErrorMessages catalog entry) matches this folder's existing convention.
Info: confirmed default(DateOnly) == DateOnly.MinValue (0001-01-01), so the new sentinel test exercises the intended condition.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — pure value-object equality check, no auth/injection/data-exposure impact; new rule is defense-in-depth (domain-level ValidateSkipTarget already rejected DateOnly.MinValue via the overdue-window bound) |
| contract-aligner | N/A — diff only touches a validator + its test; no DTO/route/packages/shared change |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — separate required CI check (per workflow instructions) |
| Tests (dotnet) | N/A — separate required CI check (per workflow instructions) |
Deferred
Dimensions 8/9/10/14 (DESIGN.md, parity, i18n, FEATURES.md) — N/A, backend-only bugfix, no UI/feature-surface/locale surface touched. Dimension 11 (contract drift) — N/A, no DTO/shared-schema field changed. /second-opinion not triggered — no Critical finding survived to require it. Both changed files (SkipHabitCommandValidator.cs, SkipHabitCommandValidatorTests.cs) read in full and verdicted.
What's good
Correctly keeps timezone-dependent checks in the handler (validator has no IUserDateService access); PR body re-verified a stale task premise before scoping the change narrowly; new tests cover exactly the three meaningful cases with clear naming.
|



What
Adds a boundary-appropriate validation rule for the optional
DateonSkipHabitCommand, so skip validation isn't only handler-deferred.Context — verified current state
The task premise was that
SkipHabitCommandValidatoris empty with all validation in the handler. On inspection that's stale: theUserId/HabitIdNotEmptyrules have existed since the original skip endpoint (#21). What was genuinely missing is a boundary rule for the optionalDate.Change
default(DateOnly)/DateOnly.MinValue(0001-01-01) sentinel — a malformed/zeroed date payload — at the trust boundary, before it reaches business logic.nullstays valid (the field is optional; the handler defaults to the user's today).What deliberately stays in the handler's domain guard
The timezone-dependent checks cannot live at the boundary because the validator has no access to the user's resolved "today":
CannotSkipFutureDate)BeyondOverdueWindow, fix(api): enforce overdue-window guard on skip commands (#243) #326)These remain in
ValidateSkipTargetunchanged — boundary handles structural validity, handler handles domain rules.Tests
Added to
SkipHabitCommandValidatorTests:Validate_NullDate_NoDateError— omitted date is validValidate_DateOnlyMinValueSentinel_HasError— zeroed sentinel is rejectedValidate_RealPastDate_NoError— a real date passes the boundary (domain window check is the handler's job)dotnet buildclean; fullOrbit.Application.Testssuite green (2631 passed).Refs thomasluizon/orbit-ui-mobile#243