fix(api): enforce overdue-window guard on skip commands (#243)

[thomasluizon]

Problem

Skip operations skipped the overdue-window guard that the complete/log paths enforce. LogHabitCommand.ValidateTargetDate and BulkLogHabitsCommand.ProcessLogItem both reject a target older than AppConstants.DefaultOverdueWindowDays with ErrorMessages.BeyondOverdueWindow, but the two skip paths had no such bound — so a habit occurrence older than the window could still be skipped (overdue = DueDate < today; skips are Value=0 HabitLog rows).

Fix

Add the identical guard to both skip commands, positioned right after the future-date check to mirror the log/bulk-log ordering exactly:

• SkipHabitCommand.ValidateSkipTarget — return Result.Failure(ErrorMessages.BeyondOverdueWindow).
• BulkSkipHabitsCommand.ProcessSkipItem — returns the per-item BulkItemStatus.Failed result carrying the BeyondOverdueWindow message/code.

Placing the bound before HabitNotYetDue / NotScheduledOnDate matches the log paths and yields the accurate reason on edge overlaps (a 10-day-old target reads "beyond overdue window", not "not yet due").

Same shared constant + error the complete/log paths use — no new contract surface (BEYOND_OVERDUE_WINDOW already exists and is consumer-handled), so this is backend-only.

Tests

• Single skip: target beyond the window fails with BeyondOverdueWindow (and does not save); target at the window boundary (today - DefaultOverdueWindowDays) succeeds.
• Bulk skip: a request mixing an in-window success with a beyond-window item reports both correctly (success + BeyondOverdueWindow failure).

Full Orbit.Application.Tests suite green (2620 passed).

Refs thomasluizon/orbit-ui-mobile#243

[claude]

PR Review — #326: enforce overdue-window guard on skip commands

Scope: SkipHabitCommand.ValidateSkipTarget and BulkSkipHabitsCommand.ProcessSkipItem gain the same targetDate < today.AddDays(-AppConstants.DefaultOverdueWindowDays) → ErrorMessages.BeyondOverdueWindow guard already enforced by LogHabitCommand.ValidateTargetDate / BulkLogHabitsCommand.ProcessLogItem. Backend-only (Orbit.Application + unit tests), no DTO/route changes.

Findings

None at Critical/High/actionable-Medium. Verified against the rubric:

• Correctness — guard is inserted in the exact same position (after the future-date check, before HabitNotYetDue/NotScheduledOnDate) as the pre-existing LogHabitCommand.ValidateTargetDate / BulkLogHabitsCommand.ProcessLogItem, byte-for-byte identical condition and error. Boundary math verified: today - DefaultOverdueWindowDays (7 days, AppConstants.cs:29) succeeds, one day older fails — matches the new tests.
• Authorization/ownership — confirmed by a dedicated security-reviewer pass: SkipHabitCommand keeps its habit.UserId != request.UserId check ahead of the new guard; BulkSkipHabitsCommand's habitMap is pre-filtered by h.UserId == request.UserId at the query, so ordering doesn't affect tenant isolation. No new endpoints/routes; both commands are reached via already-[Authorize]'d controllers.
• Contract drift — ErrorCodes.BeyondOverdueWindow / ErrorMessages.BeyondOverdueWindow already exist and are used by the log paths, so this is not a new contract surface. (Cross-repo consumer-side verification not possible — orbit-ui-mobile isn't checked out in this job — but the PR correctly doesn't require any consumer change since the code is pre-existing and consumer-handled.)
• Backend hard rules — today is sourced from IUserDateService.GetUserTodayAsync upstream in both handlers (not raw UtcNow); no logging touched; no transaction rollback changes (bulk path's ExecuteInTransactionAsync untouched).
• Tests — new coverage added for both paths: single skip at/beyond the boundary, bulk skip mixing an in-window success with a beyond-window failure. Matches the existing test style in both files.
• Dead code / comments — none introduced.
• One-time habits (FrequencyUnit is null) — checked whether the new guard creates an asymmetry here. It doesn't: SkipHabitCommand.HandleOneTimeSkip short-circuits before any date validation (pre-existing, ignores request.Date entirely — it always postpones to today+1), while BulkSkipHabitsCommand validates targetDate before its one-time branch. That split already existed for the CannotSkipFutureDate check the new line sits directly beneath; this PR just extends the same pre-existing pattern rather than introducing a new inconsistency.

Recommendation: APPROVE

Minimal, well-scoped fix that mirrors an established pattern exactly, with matching test coverage on both the single and bulk paths. No blocking issues found.

[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]

PR Review — #326: enforce overdue-window guard on skip commands

Scope: SkipHabitCommand.ValidateSkipTarget and BulkSkipHabitsCommand.ProcessSkipItem gain the same targetDate < today.AddDays(-AppConstants.DefaultOverdueWindowDays) → ErrorMessages.BeyondOverdueWindow guard already enforced by LogHabitCommand.ValidateTargetDate / BulkLogHabitsCommand.ProcessLogItem. Backend-only, no DTO/route changes.

No new commits since the prior review at 0a0eaa4 — git diff 0a0eaa4..HEAD is empty (the current tip is only a merge-preview commit). Re-verified the findings below still hold; nothing new to flag.

Findings

Severity	Count
Critical (incl. ⚠️ old-client breaks)	0
High	0
Medium	0
Low / Info	2 (non-blocking, pre-existing)

Low / Info (unchanged from prior pass, informational only):

• SkipHabitCommand.HandleOneTimeSkip dispatches before ValidateSkipTarget runs for one-time habits, so it still bypasses the future-date and overdue-window checks — pre-existing behavior, unchanged by this diff, out of scope.
• The reused ErrorMessages.BeyondOverdueWindow.Message text says "log" even when returned from a skip failure — cosmetic, consumer keys off ErrorCode not the message string.

Verified

• Guard sits in the same relative position (after future-date check, before due/scheduled checks) in both single and bulk paths, mirroring the identical guard on the log commands. Boundary math confirmed: today - DefaultOverdueWindowDays succeeds, one day older fails.
• Ownership/authorization unaffected: SkipHabitCommand's explicit UserId check and BulkSkipHabitsCommand's pre-filtered query both remain intact ahead of/independent of the new guard.
• No new contract surface — BEYOND_OVERDUE_WINDOW is a pre-existing, consumer-handled error code; cross-repo verification not possible in this job (mobile repo not checked out) but not needed here since nothing new is exposed.
• Test coverage: single-skip boundary test and bulk-skip mixed success/failure test both present and match existing test conventions.

Recommendation: APPROVE

Minimal, well-scoped fix mirroring an established pattern exactly, with matching test coverage. No blocking issues found.