fix: standardize timestamps to DateTimeOffset UTC (fixes #386)#751
fix: standardize timestamps to DateTimeOffset UTC (fixes #386)#751
Conversation
Replace DateTime.Now/DateTime.UtcNow with DateTimeOffset.UtcNow across all timestamp fields in ChatMessage, ToolActivity, PendingOrchestration, ReflectionCycle, AgentSessionInfo, ActiveSessionEntry, and SessionSummary. Key changes: - ChatMessage.Timestamp: DateTime → DateTimeOffset - All 10 ChatMessage factory methods use DateTimeOffset.UtcNow - ToolActivity.StartedAt/CompletedAt: DateTime → DateTimeOffset - PendingOrchestration.StartedAt: DateTime → DateTimeOffset - ReflectionCycle timestamps: DateTime? → DateTimeOffset? - AgentSessionInfo.CreatedAt: DateTime → DateTimeOffset - Remove ToLocalTime() workarounds in dispatch/resume paths - UI display uses .LocalDateTime.ToShortTimeString() - ChatDatabase boundary: DateTimeOffset ↔ DateTime for SQLite - 6 new behavioral tests for UTC standardization Not changed (by design): - AgentSessionInfo.LastUpdatedAt/ProcessingStartedAt (Interlocked ticks) - ScheduledTask timestamps (separate concern, local time intentional) Fixes #386 Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-Platform Verification — PR #751Build Results
✅ All platforms verified Triggered by: verify-build run |
🧪 Integration Test Report — PR #751
✅ All platforms passed |
Design-Level Findings (outside diff hunks)🟡 MODERATE — Incomplete migration: Two 🟢 MINOR — Mixed Expression
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Good direction — standardizing on DateTimeOffset with UTC eliminates the mixed-timezone comparison bugs and the ToLocalTime() workarounds. The scope is well-considered (correctly excluding Interlocked ticks patterns and display-only paths).
Findings
| Severity | File | Issue |
|---|---|---|
| 🔴 | ChatDatabase.cs:52 |
new DateTimeOffset(Timestamp, TimeSpan.Zero) misinterprets pre-existing local-time ticks as UTC — old message timestamps shift by the user's UTC offset on read. Also affects the LoadBestHistoryAsync DB-vs-events comparison. |
| 🟡 | ExternalSessionScanner.cs:391, Utilities.cs:661 |
DateTimeOffset.TryParse out parameter clobbers the UtcNow fallback on parse failure (sets to 0001-01-01). Pre-existing bug carried forward — worth fixing since this code is touched. |
| 🟡 | Organization.cs:2041,4119 |
Two SavePendingOrchestration calls still use DateTime.UtcNow for the StartedAt field. Works via implicit conversion but inconsistent with the PR's goal. |
| 🟢 | Organization.cs:3232,3263 |
DateTime.UtcNow - pending.StartedAt mixes types. Works but should be DateTimeOffset.UtcNow. |
What looks good
- Factory methods in
ChatMessageconsistently useDateTimeOffset.UtcNow✅ ChatMessageItem.razorcorrectly uses.LocalDateTimefor display ✅- Thread-safe
Interlockedpatterns correctly left asDateTime✅ - JSON backward compat for
active-sessions.jsonis fine (System.Text.Json handles ISO 8601 without offset by assuming local) ✅ default(DateTimeOffset)comparison inClearPendingOrchestrationAndResetStateis semantically equivalent to the olddefault(DateTime)✅
The ChatDatabase backward compatibility issue is the main concern — recommend addressing it before merge.
Generated by Expert Code Review (auto) for issue #751 · ● 13.8M
- ChatDatabase: use local offset for old data instead of assuming UTC (prevents timestamp shift for pre-migration local-time data) - ExternalSessionScanner/Utilities: fix TryParse clobbering fallback (parse failure no longer overwrites UtcNow default with epoch) - Organization: use DateTimeOffset.UtcNow for PendingOrchestration StartedAt (consistent with type, not implicit DateTime conversion) - Organization: use DateTimeOffset.UtcNow for elapsed time math (avoid mixed-type subtraction) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 Integration Test Report — PR #751
✅ All platforms passed |
Cross-Platform Verification — PR #751Build Results
✅ All platforms verified Previous Review HistoryFound 3 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
This comment has been minimized.
This comment has been minimized.
….UtcNow sites - CopilotService.Utilities.cs: DateTime.MinValue → DateTimeOffset.MinValue (prevents ArgumentOutOfRangeException in UTC+ timezones) - CopilotService.cs: CreatedAt = DateTime.UtcNow → DateTimeOffset.UtcNow (2 sites in session creation paths) Addresses second expert review findings for #386. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-Platform Verification — PR #751Build Results
✅ All platforms verified Previous Review HistoryFound 3 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
🧪 Integration Test Report — PR #751
✅ All platforms passed |
This comment has been minimized.
This comment has been minimized.
) feat: add quota usage display and rate limit warnings - Color-coded quota indicator pill in dashboard header (green/yellow/red) - Warning banner when quota drops below 20% - Enhanced /usage command with reset countdown - 22 new unit tests Fixes #691 Generated by agent-fix workflow. Passed integration tests + expert code review.
DateTime.MaxValue (Kind=Unspecified) → DateTimeOffset conversion overflows in UTC- timezones. Use DateTimeOffset.MaxValue for all 3 sort branches. Addresses third expert review finding for #386. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-Platform Verification — PR #751Build Results
✅ All platforms verified Previous Review HistoryFound 3 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
🧪 Integration Test Report — PR #751
✅ All platforms passed |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Finding E: Quota UI negative values when entitlement data is missing — DISAGREE (not a real issue)Verdict: Theoretical concern with negligible real-world risk. AnalysisThe reviewer correctly identified that the parser defaults
However, the premise — a limited (non-unlimited) plan missing its A defensive guard (e.g., hiding the quota display when
|
Last two DateTime.Now sites in migrated ToolActivity properties. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Summary
Standardizes all timestamp fields to
DateTimeOffsetwithDateTimeOffset.UtcNow, eliminating the mixedDateTime.Now/DateTime.UtcNowconvention that caused cross-timezone comparison bugs in orchestration dispatch paths.Fixes #386
Problem
ChatMessage.TimestampusedDateTime.Now(local time) whilePendingOrchestration.StartedAtusedDateTime.UtcNow. When comparing these values (e.g., filtering messages after dispatch time), results were wrong by the UTC offset. PR #375 addedToLocalTime()workarounds, but the root cause remained.Solution
Replace
DateTimewithDateTimeOffsetfor all timestamp fields involved in cross-component comparisons.DateTimeOffsetcarries timezone offset information, making comparisons safe regardless of which timezone the values were created in.Production Changes (16 files)
Core Models:
ChatMessage.Timestamp:DateTime→DateTimeOffset, all 10 factory methods useDateTimeOffset.UtcNowToolActivity.StartedAt/CompletedAt:DateTime→DateTimeOffsetPendingOrchestration.StartedAt:DateTime→DateTimeOffsetReflectionCycle.StartedAt/CompletedAt:DateTime?→DateTimeOffset?AgentSessionInfo.CreatedAt:DateTime→DateTimeOffsetActiveSessionEntry.CreatedAt:DateTime?→DateTimeOffset?SessionSummary.CreatedAt:DateTime→DateTimeOffsetService Layer:
ToLocalTime()workarounds inCopilotService.Organization.csdispatch/resume pathsChatMessageconstructor calls across Events.cs, Bridge.cs, Providers.csChatDatabase.cs: Boundary conversion (DateTimeOffset↔DateTimefor SQLite)ExternalSessionScanner.csandUtilities.cs:DateTimeOffset.TryParsefor events.jsonlUI:
ChatMessageItem.razor: Display uses.LocalDateTime.ToShortTimeString()Test Changes (13 files)
DateTime.Now→DateTimeOffset.UtcNowNot Changed (by design)
AgentSessionInfo.LastUpdatedAt/ProcessingStartedAt— useInterlockedticks pattern for thread safety, not involved in cross-timezone bugsScheduledTasktimestamps — separate concern, local time is intentionalMauiProgramcrash logging — display-onlyTest Results
Backward Compatibility
Old persisted sessions with
DateTimestrings deserialize correctly intoDateTimeOffsetvia System.Text.Json (ISO 8601 without offset assumes local time, with 'Z' assumes UTC).Warning
The following domain was blocked by the firewall during workflow execution:
192.0.2.1To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.