Add proposal lifecycle edge case tests (TST-41)#736
Conversation
Cover expiry timing boundaries, double-apply prevention, comprehensive state machine violations, dismissal edge cases, and operation mutation guards after state transitions.
Cover expiry enforcement during validation, operation limits, duplicate sequences, oversized parameters, risk classification defaults, and null proposal handling.
Cover approve-after-expiry race, batch expiry via service, double-apply and double-fail prevention, dismiss batch behavior with mixed dismissable/non-dismissable proposals, and reject after expiry.
Cover normal pending expiry, batch expiry of 50 proposals, mixed-state batch handling, database error propagation, worker-vs-manual-approval race condition, and ExecuteAsync cancellation behavior.
Adversarial Self-ReviewStrengths
Potential weaknesses
VerdictTests are structurally sound and test real failure modes. The main gap is the integration-level scenarios (6-16) which would require a separate follow-up issue with |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Adds broad edge-case test coverage for the automation proposal lifecycle across domain, application services, policy validation, and the API housekeeping worker to close #708 (TST-41).
Changes:
- Adds domain-level state machine tests for expiry boundaries, invalid transitions, dismissal rules, and post-decision mutation guards.
- Adds application service tests for approve/expire/dismiss flows and double-apply/fail prevention.
- Adds policy engine tests for expiry enforcement, operation limits, parameter sizing, and default risk classification.
- Adds API worker tests for stale-expiry batching, mixed-state handling, cancellation behavior, and error propagation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| backend/tests/Taskdeck.Domain.Tests/Entities/AutomationProposalLifecycleEdgeCaseTests.cs | New domain tests covering lifecycle transitions, expiry/dismissal edge cases, and mutation guards. |
| backend/tests/Taskdeck.Application.Tests/Services/AutomationProposalServiceEdgeCaseTests.cs | New service-layer tests for expiry/approval races, batch expiry, dismissal batching, and double-apply/fail prevention. |
| backend/tests/Taskdeck.Application.Tests/Services/AutomationPolicyEngineEdgeCaseTests.cs | New policy engine tests for expiry validation, operation constraints, parameter sizing, and risk classification defaults. |
| backend/tests/Taskdeck.Api.Tests/ProposalHousekeepingWorkerEdgeCaseTests.cs | New worker tests for stale expiry behavior (single + batch), mixed states, cancellation, and DB error propagation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Fact] | ||
| public void Approve_ShouldThrow_WhenExpiresAtIsExactlyNow() | ||
| { | ||
| // Arrange: proposal whose expiry has just passed | ||
| var proposal = CreateProposal(); | ||
| SetExpiresAt(proposal, DateTime.UtcNow.AddMilliseconds(-1)); | ||
|
|
||
| // Act | ||
| var act = () => proposal.Approve(Guid.NewGuid()); | ||
|
|
||
| // Assert | ||
| act.Should().Throw<DomainException>() | ||
| .WithMessage("Cannot approve expired proposal"); | ||
| } |
There was a problem hiding this comment.
Test name/comment don’t match the setup: the test says ExpiresAt is "exactly now" but sets it to UtcNow.AddMilliseconds(-1) (already in the past). Either rename to reflect "just expired" / "past expiry", or change the setup to explicitly exercise the equality boundary (and document the intended semantics for ExpiresAt == now).
| [Theory] | ||
| [InlineData("Applied")] | ||
| [InlineData("Rejected")] | ||
| [InlineData("Failed")] | ||
| [InlineData("Expired")] | ||
| public void CanBeDismissed_ShouldBeTrue_ForTerminalStatuses(string targetState) | ||
| { | ||
| var proposal = CreateProposal(); | ||
|
|
||
| switch (targetState) | ||
| { | ||
| case "Applied": | ||
| proposal.Approve(Guid.NewGuid()); | ||
| proposal.MarkAsApplied(); | ||
| break; | ||
| case "Rejected": | ||
| proposal.Reject(Guid.NewGuid()); | ||
| break; | ||
| case "Failed": | ||
| proposal.Approve(Guid.NewGuid()); | ||
| proposal.MarkAsFailed("Error"); | ||
| break; | ||
| case "Expired": | ||
| proposal.Expire(); | ||
| break; | ||
| } | ||
|
|
||
| proposal.CanBeDismissed.Should().BeTrue(); | ||
| } |
There was a problem hiding this comment.
CanBeDismissed_ShouldBeTrue_ForTerminalStatuses uses string literals + a switch to select the target state. This is not refactoring-safe (renames won’t be caught) and allows invalid values without compiler help. Prefer using ProposalStatus (or an enum-based [InlineData]) and switching on the enum, or separate [Fact]s per terminal status.
| private static void SetExpiresAt(AutomationProposal proposal, DateTime expiresAt) | ||
| { | ||
| var property = typeof(AutomationProposal).GetProperty( | ||
| nameof(AutomationProposal.ExpiresAt)); | ||
| property!.SetValue(proposal, expiresAt); | ||
| } |
There was a problem hiding this comment.
SetExpiresAt uses reflection without BindingFlags and relies on the null-forgiving operator. Other tests in this repo guard this more explicitly (e.g., ProposalHousekeepingWorkerTests uses BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic and asserts the property isn’t null). Aligning with that pattern would make failures clearer if the property accessibility/name changes.
| [Fact] | ||
| public async Task ExpireStaleProposals_ShouldLogWarning_WhenProposalWasApprovedBetweenFetchAndExpire() | ||
| { | ||
| // Simulate race: proposal fetched as PendingReview but was approved | ||
| // before the worker iterates to it. Approve first (while not expired), | ||
| // then set ExpiresAt to past (simulating time passing). | ||
| var proposal = CreatePendingProposal(); | ||
| proposal.Approve(Guid.NewGuid()); | ||
| SetExpiresAt(proposal, DateTime.UtcNow.AddMinutes(-5)); | ||
|
|
||
| var repository = new TrackingProposalRepository([proposal]); | ||
| var logger = new InMemoryLogger<ProposalHousekeepingWorker>(); | ||
| var unitOfWork = new FakeUnitOfWork(repository); | ||
| var (worker, sp) = CreateWorkerWithProvider(unitOfWork, logger); | ||
| using (sp) | ||
| { | ||
| await InvokeExpireStaleProposalsAsync(worker, CancellationToken.None); | ||
| } | ||
|
|
||
| proposal.Status.Should().Be(ProposalStatus.Approved, "proposal should remain approved"); | ||
| logger.Entries.Should().ContainSingle(e => e.Level == LogLevel.Warning); | ||
| logger.Entries.Single(e => e.Level == LogLevel.Warning) | ||
| .Message.Should().Contain("Failed to expire proposal"); | ||
| } |
There was a problem hiding this comment.
The test intends to simulate “approved between fetch and expire”, but it approves the proposal before calling the worker, and the fake repository returns proposals regardless of the requested status. With a realistic repo that honors GetByStatusAsync(PendingReview), this proposal would never be returned, so the race isn’t actually exercised. Consider making the fake repo filter by status and introducing the approval as a side-effect after fetch (e.g., via a callback) to match the scenario being described.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of edge-case tests for the automation proposal system, covering the domain entity lifecycle, application services, policy engine, and background housekeeping worker. The tests address scenarios such as state machine violations, race conditions between manual and automated actions, batch expiry, and database resilience. Feedback focuses on improving test robustness by increasing timeouts and offsets to prevent flakiness and correcting misleading exception handling in background worker tests.
| var (worker, sp) = CreateWorkerWithProvider(unitOfWork); | ||
| using (sp) | ||
| { | ||
| using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(100)); |
There was a problem hiding this comment.
Using a very short timeout (100ms) for a background worker loop test can lead to flakiness, especially in CI environments where resource contention might delay the worker's startup or execution. A more robust approach would be to use a longer timeout (e.g., 1 second) or to start the task and then manually cancel the token to ensure the worker has entered its execution loop.
| { | ||
| await task.WaitAsync(TimeSpan.FromSeconds(5)); | ||
| } | ||
| catch (TaskCanceledException) | ||
| { | ||
| // Expected: Task.Delay cancellation propagates | ||
| } | ||
|
|
There was a problem hiding this comment.
The try-catch block for TaskCanceledException is misleading here. In ProposalHousekeepingWorker.ExecuteAsync, the OperationCanceledException is explicitly caught and the loop is broken, which means the task will complete normally (RanToCompletion) rather than being in a Canceled state. Consequently, await task.WaitAsync(...) will not throw a TaskCanceledException. You should remove the try-catch and simply await the task to verify it completes promptly.
await task.WaitAsync(TimeSpan.FromSeconds(5));| { | ||
| // Arrange: proposal whose expiry has just passed | ||
| var proposal = CreateProposal(); | ||
| SetExpiresAt(proposal, DateTime.UtcNow.AddMilliseconds(-1)); |
There was a problem hiding this comment.
Setting the expiry time with a -1ms offset is risky and can lead to flaky tests, particularly on operating systems with lower clock resolution (like Windows, where DateTime.UtcNow resolution is typically ~15ms). If the subsequent Approve call occurs within the same clock tick, the IsExpired check (DateTime.UtcNow > ExpiresAt) might return false. It is safer to use a larger offset, such as AddSeconds(-1) or AddMinutes(-1), to ensure the proposal is consistently recognized as expired.
SetExpiresAt(proposal, DateTime.UtcNow.AddSeconds(-1));…e cases - Rename Approve_ShouldThrow_WhenExpiresAtIsExactlyNow to WhenJustPastExpiry and use -1 second instead of -1ms to avoid Windows clock-resolution flakiness - Replace string-based Theory with separate Facts for refactoring safety - Add BindingFlags to SetExpiresAt reflection helper for consistency - Add missing tests: Dismiss on PendingReview, Dismiss on non-expired Approved, Reject without reason for High/Critical risk, AddOperation on Approved - Fix worker race test name and comment to honestly describe what it exercises - Increase cancellation test timeout from 100ms to 1s for CI stability - Fix cancellation test comment to accurately describe TaskCanceledException propagation from Task.Delay outside the worker try-catch
Adversarial Review (Round 2)Issues found and fixed (commit a9dbec4)1. Clock-resolution flakiness (Domain tests, line 25) 2. String-based Theory not refactoring-safe (Domain tests, lines 397-425) 3. SetExpiresAt reflection inconsistency (Domain tests, line 548) 4. Race condition test name misleading (Worker tests, line 134) 5. Cancellation timeout too aggressive (Worker tests, line 170) 6. Misleading cancellation try-catch comment (Worker tests, line 184) Missing edge cases added
Issues reviewed but not actionable
Test resultsAll 74 edge case tests pass: 42 Domain + 25 Application + 7 Api. |
Final StatusAll CI checks pass on commit a9dbec4. Summary of changes made
Bot comment disposition
PR is ready for merge. |
Summary
Closes #708
74 new tests total across 4 files. All 2428 tests pass.
Issue scenarios covered
Not covered (out of scope for pure unit/service tests)
Test plan
dotnet build backend/Taskdeck.sln -c Releasepassesdotnet test backend/Taskdeck.sln -c Release -m:1passes (2428 tests, 0 failures)