Skip to content

Fix DateTime/DateTimeOffset mismatch on AutomationProposal.ExpiresAt (#1191)#1201

Open
Chris0Jeky wants to merge 2 commits into
mainfrom
fix/1191-expiresat-datetime-mismatch
Open

Fix DateTime/DateTimeOffset mismatch on AutomationProposal.ExpiresAt (#1191)#1201
Chris0Jeky wants to merge 2 commits into
mainfrom
fix/1191-expiresat-datetime-mismatch

Conversation

@Chris0Jeky
Copy link
Copy Markdown
Owner

Summary

  • Add EF Core value converters to AutomationProposalConfiguration that normalize DateTime properties (ExpiresAt, DecidedAt, AppliedAt) to DateTimeKind.Utc on materialization from SQLite
  • SQLite stores DateTime as TEXT without timezone info; EF Core reads these as DateTimeKind.Unspecified, causing incorrect comparisons with DateTime.UtcNow and wrong expiry behavior
  • Chose the value converter approach over changing to DateTimeOffset because EF Core SQLite cannot translate DateTimeOffset comparisons in LINQ expressions -- the converter is zero-blast-radius (no migration, no entity type changes, no test changes)

Closes #1191

Test plan

  • Build passes (dotnet build backend/Taskdeck.sln -c Release -- 0 errors)
  • All 97 Domain AutomationProposal tests pass
  • All 44 Application AutomationProposal tests pass
  • All 36 Api AutomationProposal integration tests pass (including repo integration tests that exercise the SQLite round-trip)
  • All 136 related tests pass (ProposalHousekeeping, WorkerResilience, AutomationPolicyEngine, ConfidenceBreakdown, LiveBrowserRegression, MigrationBootstrap)
  • CI green

Add EF Core value converters to normalize DateTime properties (ExpiresAt,
DecidedAt, AppliedAt) to DateTimeKind.Utc on materialization from SQLite.

SQLite stores DateTime as TEXT without timezone info, and EF Core reads
these as DateTimeKind.Unspecified. This caused incorrect comparisons with
DateTime.UtcNow because Unspecified != Utc in .NET DateTime comparison
semantics, leading to wrong expiry behavior.

The value converter approach is the safest fix: no schema migration needed,
no entity type changes, no LINQ query translation issues, and all 177
related tests pass.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces UTC DateTime converters for specific properties in the AutomationProposal entity configuration to resolve timezone issues when materializing dates from SQLite. The reviewer suggests a more robust, global approach using EF Core's Pre-convention Model Configuration to automatically apply UTC normalization to all DateTime properties across the application, or at least extending the local converters to other DateTime properties like CreatedAt and UpdatedAt.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +53 to +58
var utcConverter = new ValueConverter<DateTime, DateTime>(
v => v,
v => DateTime.SpecifyKind(v, DateTimeKind.Utc));
var nullableUtcConverter = new ValueConverter<DateTime?, DateTime?>(
v => v,
v => v.HasValue ? DateTime.SpecifyKind(v.Value, DateTimeKind.Utc) : v);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Architectural Improvement: Global UTC DateTime Conversion

While applying these converters to ExpiresAt, DecidedAt, and AppliedAt fixes the immediate issue for these specific properties, other DateTime properties on AutomationProposal (such as CreatedAt and UpdatedAt) and other entities in the system will still suffer from the same DateTimeKind.Unspecified issue when materialized from SQLite.

To solve this robustly and globally, you can use EF Core's Pre-convention Model Configuration to automatically apply UTC normalization to all DateTime and DateTime? properties across the entire application.

Recommended Solution (Global)

  1. Define the converters in the infrastructure layer:
public class UtcDateTimeConverter : ValueConverter<DateTime, DateTime>
{
    public UtcDateTimeConverter() : base(
        v => v,
        v => DateTime.SpecifyKind(v, DateTimeKind.Utc))
    {
    }
}

public class NullableUtcDateTimeConverter : ValueConverter<DateTime?, DateTime?>
{
    public NullableUtcDateTimeConverter() : base(
        v => v,
        v => v.HasValue ? DateTime.SpecifyKind(v.Value, DateTimeKind.Utc) : v)
    {
    }
}
  1. Override ConfigureConventions in TaskdeckDbContext.cs:
protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<DateTime>()
        .HaveConversion<UtcDateTimeConverter>();
    configurationBuilder.Properties<DateTime?>()
        .HaveConversion<NullableUtcDateTimeConverter>();
}

This completely eliminates the need for manual property-by-property configuration and ensures that CreatedAt, UpdatedAt, and all other DateTime fields across all entities are always materialized as UTC.

Alternative Solution (Local to this configuration)

If you prefer to keep this change localized to this configuration for now, please ensure that CreatedAt and UpdatedAt also use the utcConverter:

builder.Property(ap => ap.CreatedAt)
    .IsRequired()
    .HasConversion(utcConverter);

builder.Property(ap => ap.UpdatedAt)
    .IsRequired()
    .IsConcurrencyToken()
    .HasConversion(utcConverter);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted the global ConfigureConventions approach in commit 8d16202. The per-entity converters have been removed and replaced with a convention-based solution in TaskdeckDbContext that covers all DateTime/DateTime? properties across every entity. Note that CreatedAt/UpdatedAt are DateTimeOffset (from the base Entity class), not DateTime, so they already materialize correctly.

@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Self-Review Findings

Change Summary

Single file changed: AutomationProposalConfiguration.cs -- adds EF Core value converters to normalize DateTime properties to DateTimeKind.Utc on materialization from SQLite.

Review

LOW: Other entities have the same DateTime/SQLite pattern (out of scope)
ArchiveItem.ArchivedAt, ArchiveItem.RestoredAt, CommandRun.StartedAt, CommandRun.CompletedAt, and CommandRunLog.Timestamp are also DateTime properties stored in SQLite TEXT columns. They could theoretically suffer the same Unspecified kind issue. However, these are not time-critical comparison targets like ExpiresAt -- they are recorded timestamps, not used in > DateTime.UtcNow guards. Tracked as out-of-scope; a follow-up issue could apply the same converter pattern globally via ConfigureConventions.

Verification

Check Result
Build (dotnet build -c Release) 0 errors
Domain tests 1626/1626 passed
Application tests 3295/3295 passed
Architecture tests 20/20 passed (1 pre-existing skip)
API integration tests 1740/1740 passed
Model snapshot No changes (no migration needed)
LINQ query translation Verified -- value converter does not affect SQL generation; all 14 repo integration tests pass

Conclusion

The fix is minimal, correct, and fully verified. The value converter approach is the standard pattern for SQLite + EF Core DateTime normalization. No findings require action in this PR.

Move the DateTime UTC normalization from per-entity converters in
AutomationProposalConfiguration to a global ConfigureConventions
override in TaskdeckDbContext. This ensures all DateTime and DateTime?
properties across every entity are materialized with DateTimeKind.Utc,
preventing the same SQLite/Unspecified kind bug in ArchiveItem,
CommandRun, and any future entities.

Addresses Gemini review feedback on PR #1201.
@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Fix Evidence (addressing Gemini review feedback)

Finding: Gemini suggested promoting the UTC DateTime converters from per-entity to a global ConfigureConventions override in TaskdeckDbContext.

Action: Implemented in commit 8d16202. The converters are now global via ConfigureConventions, covering all DateTime and DateTime? properties across every entity. The per-entity converters in AutomationProposalConfiguration have been removed (file reverted to its original form).

Note on CreatedAt/UpdatedAt: These are DateTimeOffset properties on the base Entity class, not DateTime, so they are not affected by this converter. The DateTimeOffset type already includes timezone information and EF Core materializes it correctly.

Verification after fix:

Check Result
Build 0 errors
Domain tests 1626/1626 passed
Application tests 3295/3295 passed
API integration tests 1740/1740 passed
Architecture tests 20/20 passed (1 pre-existing skip)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending

Development

Successfully merging this pull request may close these issues.

DateTime/DateTimeOffset mismatch on AutomationProposal.ExpiresAt causes wrong expiry in SQLite

1 participant