test(habits): bulk-command partial-failure isolation + batch idempotency replay

[thomasluizon]

Closes tests-audit gaps for the bulk-command transaction/isolation guarantees with intelligent, behavior-first tests — each fails if the guarantee breaks (no rubber stamps).

What changed

BulkLogHabitsCommandHandlerTests — the handler processes items independently with a per-item try/catch and a single trailing save (per-item isolation, not whole-batch rollback):

• Handle_InvalidItemsInterleavedWithValid_AppliesValidReportsInvalidPerItem — invalid item before valid ones (dependency ordering): valid items log and persist, invalid items are reported per-item with the correct ErrorCode + Index, and the batch is not rolled back (AddAsync received twice, both saves fire).
• Handle_ItemPersistenceThrowsMidBatch_IsolatesFailureAndAppliesOtherItems — a genuine mid-batch throw (repo AddAsync throws for one habit) is caught per-item and reported as MUTATION_FAILED, while the other item still succeeds and reaches gamification. Covers the previously-untested catch block.
• Handle_CompletedOneTimeTask_ReportsDomainFailurePerItemAndContinuesBatch — a domain Log() failure (HABIT_ALREADY_COMPLETED) is surfaced per item without aborting the batch. Covers the logResult.IsFailure branch.

BulkDeleteHabitsCommandHandlerTests — the handler is all-or-nothing (no per-item catch, single save + streak recalc inside one transaction). Replaced the rubber-stamp Handle_UsesTransactionWrapper (only asserted the wrapper was called) with:

• Handle_SaveFailsMidBatch_RollsBackWholeBatchAndSkipsRecalcAndCacheInvalidation — a mid-batch save failure propagates, streak recalc never runs, and the summary cache is left intact (post-commit side effects gated behind a successful transaction).
• Handle_PersistenceAndRecalcRunInsideTransaction_NotOutsideIt — proves persistence and recalc execute strictly inside the transaction callback, not before it (would fail if a refactor moved either outside the atomic boundary).

IdempotencyBehaviorRaceTests — extended the concurrent-duplicate race (#243 iter 2) to the batch-command path:

• Handle_ConcurrentDuplicateBatchCommand_ReplaysWinnersFullBatchResponse — the loser of the unique-index race replays the winner's full Result<batch> response through the ResultJsonConverterFactory (all items preserved, in order) and never re-runs its own handler.

Verification

• dotnet build clean.
• Orbit.Application.Tests: 2757 passed. Orbit.Infrastructure.Tests: 1917 passed.
• No source changes were required — the handlers already implement the guarantees; these tests lock them in.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

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

Scope: PR #374, test/bulk-command-transaction-isolation → main. Test-only diff: BulkDeleteHabitsCommandHandlerTests.cs, BulkLogHabitsCommandHandlerTests.cs, IdempotencyBehaviorRaceTests.cs.
Recommendation: APPROVE

Summary

This PR replaces a rubber-stamp test (Handle_UsesTransactionWrapper, which only asserted the wrapper was invoked) and adds five new behavior-first tests covering per-item isolation in the bulk-log handler, all-or-nothing rollback semantics in the bulk-delete handler, and batch-response replay through the idempotency ledger. Every new assertion was traced against the actual handler source (BulkLogHabitsCommand.cs, BulkDeleteHabitsCommand.cs, IdempotencyBehavior.cs, ResultJsonConverterFactory.cs) and confirmed to exercise a real code path with a correct expected outcome — none are tautological.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

• Info: Handle_SaveFailsMidBatch_RollsBackWholeBatchAndSkipsRecalcAndCacheInvalidation (tests/Orbit.Application.Tests/Commands/Habits/BulkDeleteHabitsCommandHandlerTests.cs:129) is named "RollsBack" but, being a unit test with a mocked ExecuteInTransactionAsync, it actually verifies exception propagation + gating of post-commit side effects (recalc, cache invalidation), not a literal DB rollback. This is correct — it's the only thing a unit test can verify here. Not a blocking finding.

Subagents

Agent	Verdict
security-reviewer	N/A — diff touches only tests/, zero src/ files
contract-aligner	N/A — no DTO, Controller route, or packages/shared surface changed

Validation

Check	Result
Build (dotnet)	N/A — skipped per instructions; CI's Build check covers this
Tests (dotnet)	N/A — skipped per instructions; CI's Unit Tests check covers this; PR body states 2757 + 1917 passed locally

Deferred — N/A dimensions

• Parity / i18n / FEATURES.md: N/A — frontend/mobile-only dimensions; sibling orbit-ui-mobile repo not checked out, not verifiable in this job.
• Contract drift + backward-compat: N/A — no DTO/Zod/endpoint surface touched.
• Security: reviewed directly — test-only code, no injection/auth/crypto/logging surface at risk.
• Backend hard rules: reviewed — diff satisfies rather than violates the "every handler has a unit test" rule.

What's good

• Genuinely deletes a rubber-stamp test rather than leaving it alongside the new ones.
• Handle_PersistenceAndRecalcRunInsideTransaction_NotOutsideIt is a strong regression guard: captures an insideTransaction flag via the mocked ExecuteInTransactionAsync closure and asserts both SaveChangesAsync and RecalculateAsync observe it true — would actually fail if a future refactor moved either call outside the atomic boundary.
• The new idempotency test reuses the exact same ResultJsonConverterFactory-configured JsonSerializerOptions the production IdempotencyBehavior uses internally, and asserts WithStrictOrdering() on the replayed batch — a faithful round-trip check.
• Error-path tests correctly account for check-ordering inside ProcessLogItem (e.g., the future-date check fires before the not-found check for an item with both a bogus ID and a future date).

Recommendation

No changes required. Clean to merge.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

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

Scope: PR #374 — test(habits): bulk-command partial-failure isolation + batch idempotency replay
Recommendation: APPROVE

Summary

Test-only diff (3 files, all under tests/) adding intelligent, behavior-grounded coverage for BulkDeleteHabitsCommandHandler, BulkLogHabitsCommandHandler, and IdempotencyBehavior's batch-command replay path. Every new assertion was traced against the real handler/domain code (BulkDeleteHabitsCommand.cs, BulkLogHabitsCommand.cs, Habit.Log(), IdempotencyBehavior.cs, ResultJsonConverterFactory.cs) rather than the PR description, and every assertion matches actual control flow, including subtle ordering details (e.g. the future-date check running before the not-found check in ProcessLogItem, so a nonexistent habit with a future date correctly fails with CannotLogFutureDate not HabitNotFound). No source changes were required or made, consistent with the stated intent of locking in existing guarantees.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

None posted (signal gate — no Low/Info noise on a clean diff).

Subagents

Agent	Verdict
security-reviewer	N/A — no src/ files changed (test-only diff)
contract-aligner	N/A — no DTO, Controller route, or packages/shared type changed

Validation

Check	Result
Build (dotnet)	Skipped — CI runs Build as a separate required check
Tests (dotnet)	Skipped — CI runs Unit Tests as a separate required check

What's good

• Replaced a genuine rubber-stamp test (Handle_UsesTransactionWrapper, which only asserted the transaction wrapper was invoked on an empty list) with tests that fail if the actual guarantee regresses — Handle_SaveFailsMidBatch_... and Handle_PersistenceAndRecalcRunInsideTransaction_NotOutsideIt genuinely instrument the mock to prove ordering, not just call counts.
• Handle_InvalidItemsInterleavedWithValid_... deliberately orders a not-found item after a future-dated item to exercise the real branch-precedence in ProcessLogItem — this is the kind of test that would actually catch a refactor bug, not just restate the code.
• The idempotency batch-replay test correctly forces the handler's next() to never execute (handlerCalls.Should().Be(0)) by having SaveChangesAsync throw on the first call (inside Reserve's flush), before next is ever reached — this matches the real unique-index-race code path exactly, not an approximation of it.
• ResultJsonConverterFactory round-trip is exercised with a 3-item batch and WithStrictOrdering(), which would catch a converter regression that reorders or drops items.
• Comment policy: no narration comments introduced anywhere in the diff.

Recommendation

No changes requested. This PR is a clean, well-verified strengthening of existing test coverage with no source changes and no risk surface. Safe to merge.