chore(api): collapse empty entity ctors + lock HabitLog read isolation with tests (#243)

[thomasluizon]

Summary

Behavior-preserving code-quality cleanup (Batch 5) against the frozen /prod-readiness audit, plus regression tests that lock the HabitLog cross-user isolation invariant into the suite.

1. Empty private-constructor whitespace (entities)

Collapsed the empty EF materialization constructors to one line, matching Habit.cs / User.cs style:

• DistributedRateLimitBucket, PendingClarification, PendingAgentOperationState.

2. HabitLog read isolation — verified + tested (defense-in-depth)

The audit flagged several HabitLog reads as "filter only by HabitId, trusting the relational chain." On inspection each read is already scoped to the requesting user via the audit's permitted equivalent join-through-Habit ownership check, so no redundant filter was added (that would duplicate existing logic and trip ORBIT rules 1/3). Instead the invariant is now locked in with tests that capture and evaluate the actual read predicate, so removing the ownership scope fails them:

Query	Existing isolation mechanism
GetStreakHistoryQuery	read filters by streakEligibleHabitIds (⊆ h.UserId == request.UserId)
GetDailySummaryQuery (bad-habit slips)	read filters by badHabitIds (⊆ owned habits)
DuplicateHabitCommand (completion logs)	read filters by completedOneTimeIds (⊆ owned subtree)
GetHabitByIdQuery / GetHabitFullDetailQuery	ownership pre-check returns HabitNotFound for a foreign HabitId before any log query runs
GetHabitLogsQuery	ownership pre-check (already covered by WrongUser / DoesNotQueryLogs tests)

New tests assert a foreign-user HabitId is excluded from each read (or never reaches it).

HabitLog has no UserId column and no Habit navigation (HasMany(h => h.Logs).WithOne()), so an in-predicate l.UserId/l.Habit.UserId term is impossible without a model change; the owned-habit-id join is the correct approach and is already present.

3. Challenge queries — deliberately excluded from a per-user filter

GetChallengeDetailQuery and GetUserChallengesQuery aggregate progress across every participant's linked habits (ChallengeProgressCalculator.GetContributingHabitSets iterates all active participants). A request.UserId filter there would regress shared CoopGoal / StreakTogether progress. Their isolation boundary is challenge-membership (already enforced) and the habit ids are never user-supplied. Tests lock in that scope: every participant's linked habit counts, a habit linked to no participant is excluded.

4. Result<T>.Value (review-only)

Left as-is: it already throws InvalidOperationException on failure access (line 47-49); the _value! is null-forgiving on a legitimately-nullable T? backing field, guarded by IsSuccess. That is the intended guard — no change needed.

Tests

dotnet build clean; Orbit.Application.Tests 2695 passed, Orbit.Domain.Tests 512 passed (7 new isolation tests). Changed files grepped clean for bare // (ORBIT0001).

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Review Summary — PR #366

Recommendation: APPROVE

This is a genuinely low-risk, behavior-preserving PR:

1. Constructor collapse — three Domain entities (DistributedRateLimitBucket, PendingAgentOperationState, PendingClarification) get their empty private EF-materialization constructors collapsed from 3 lines to 1, matching existing Habit/User style. Verified each file in full: accessibility unchanged (still private, still parameterless), all Create() factory validation and property setters intact — no other lines touched.
2. Test-only additions — seven test files gain new regression tests that capture the actual LINQ predicate expression tree each handler passes to IGenericRepository<HabitLog>.FindAsync, asserting it both includes the caller's own data and excludes a foreign/unlinked habit's logs. No production handler code changed.

Independent verification

Traced every isolation claim in the PR body against live handler source rather than trusting the description:

• GetStreakHistoryQueryHandler — habit IDs derived from habitRepository.FindAsync(h => h.UserId == request.UserId, ...), so the log predicate is already user-scoped.
• GetDailySummaryQueryHandler — badHabitIds comes from habits already filtered by h.UserId == request.UserId.
• DuplicateHabitCommandHandler — completedOneTimeIds built from a subtree rooted in a habit fetched via FindTrackedAsync(h => h.UserId == request.UserId, ...).
• GetHabitByIdQueryHandler / GetHabitFullDetailQueryHandler — ownership pre-check (h.Id == request.HabitId && h.UserId == request.UserId) returns HabitNotFound before any HabitLog query runs; new tests assert DidNotReceive().FindAsync(...) for this case.
• GetChallengeDetailQueryHandler / GetUserChallengesQueryHandler + ChallengeProgressCalculator.GetContributingHabitSets — cross-participant aggregation confirmed intentional; GetChallengeDetailQueryHandler already gates on participant membership before returning anything.

All new tests wire into each file's existing NSubstitute mocking conventions — capturing the Expression<Func<HabitLog,bool>>, compiling it, and asserting against synthetic in-scope/out-of-scope HabitLogs. None are tautological or disconnected from the real predicate.

Findings

Critical: None
High: None
Medium: None
Low/Info: None — comment-policy grep across all 10 changed files found zero bare // narration comments.

Subagents

Agent	Verdict
security-reviewer	PASS — only change to src/ is a private-constructor whitespace collapse in three Domain entities; no field/accessibility/factory/validation-guard change, no auth/secrets/injection surface touched.
contract-aligner	N/A — no DTO, Controller route, or packages/shared type/endpoints.ts touched.

Deferred / N/A dimensions

• Build/tests: skipped per CI adaptation (separate Build/Unit Tests/SonarCloud required checks handle this). PR description states dotnet build clean, 2695+512 tests passing incl. 7 new.
• Backward-compat guard / cross-repo (orbit-ui-mobile): N/A — no contract-surface change in this diff; not verifiable in this job regardless (sibling repo not checked out), but moot here.
• FEATURES.md/DESIGN.md/i18n/parity: N/A — backend-only test/refactor diff, no user-facing feature surface change.

What's good

Exemplary "verify, don't just refactor" discipline: rather than bolting on a redundant filter, the author traced each read's actual isolation mechanism, confirmed it was already correct, and wrote tests that capture the real predicate expression tree so any future regression that weakens ownership scoping fails the suite immediately. The Challenge-query exclusion is correctly tested as intentional cross-participant design, not an oversight.

Recommendation

Merge as-is. No changes requested.

[claude]

Review: APPROVE

Scope: PR #366 (chore/habitlog-read-isolation-and-ctor-cleanups) - entity constructor collapse (DistributedRateLimitBucket.cs, PendingAgentOperationState.cs, PendingClarification.cs) plus 7 test files adding predicate-capture regression tests for HabitLog read isolation.

Since the last review

A prior claude review already APPROVED this PR at commit 03ca1f126 (2026-07-13T01:29:31Z), covering the full 10-file, 121-insertion/9-deletion diff.

The only commit added since then is a merge from main (dd980dcb), which pulls in 34 unrelated files - FluentValidation query-validator additions (ApiKeys, Calendar, Checklist, Gamification, Goals, Habits, Notifications, Profile, Referral, Subscription, Tags, UserFacts + QueryValidatorGuardTests.cs) that landed via PR #365 and were already reviewed/merged separately. Zero overlap with the files PR #366 itself changes; no merge-conflict markers in the touched entity files. gh pr diff 366 at current HEAD still shows the exact same PR #366 diff as before.

Findings

None - no Critical, High, Medium, or Low findings. The prior review's analysis stands unchanged since the reviewed code is byte-identical: constructor collapse is whitespace-only with accessibility/factory validation intact, and the new tests capture real Expression<Func<HabitLog,bool>> predicates (not tautological).

Severity	Count
Critical	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