Code Review: PR #556 — fix: normalize rollout timestamps before deriving started_at/ended_at

Summary

This PR fixes a correctness bug in the agent rollout ingesters (Claude Code, Codex, ATIF) where started_at and ended_at were derived by calling min()/max() on raw ISO 8601 timestamp strings. Lexicographic comparison produces wrong results when timestamps have differing UTC offsets or sub-second precisions — e.g. "2025-01-01T00:30:00+01:00" sorts after "2025-01-01T00:00:00Z" as a string, but is chronologically 30 minutes earlier.

The fix introduces two helpers in utils.py — parse_iso8601() and min_max_timestamps() — that parse timestamps into datetime objects before comparison, then return the winners in their original string form so the output contract is unchanged. All three ingesters are updated to use the new helper. A parameterized test covers the key edge cases.

Files changed: 5 (3 ingesters, 1 utility module, 1 new test file)
Lines: +88 / -7

Findings

Correctness

• Fix is sound. Parsing to datetime before comparison correctly handles mixed offsets, mixed precisions, and naive-vs-aware timestamps. The Z → +00:00 replacement before fromisoformat() is the standard workaround for Python < 3.11 compatibility and is safe here since Z only appears as a trailing timezone designator in ISO 8601.
• Output contract preserved. min_max_timestamps() returns the original string values, not stringified datetime objects, so downstream consumers are unaffected.
• Codex short-circuit preserved. In codex.py, the session_meta.get("timestamp") fallback for started_at is correctly maintained (started_at=coerce_optional_str(session_meta.get("timestamp")) or earliest).

Design

• Centralised helper is the right call. The same parsing logic was duplicated across three files; moving it to utils.py eliminates the duplication cleanly.
• Silent skip of unparseable values is consistent. The PR description notes this mirrors the existing tolerance of coerce_optional_str. If all timestamps are unparseable, (None, None) is returned, which matches the previous behavior of an empty list producing None. No log warning is emitted for skipped values — this is a minor observation, not a blocker, since the ingestion layer already tolerates missing timestamps.

Style & Conventions

• from __future__ import annotations present in both new/modified files — compliant.
• SPDX license header present in the new test file.
• Absolute imports only — no relative imports introduced.
• Type annotations on all new functions and variables.
• Naming: earliest/latest in codex.py avoids shadowing and reads naturally.

Tests

• Coverage is good. The parameterized test (test_utils.py) covers 6 cases:
  1. Empty list → (None, None)
  2. Mixed offsets where lexicographic ordering disagrees with chronological
  3. Mixed sub-second precision
  4. Naive timestamp treated as UTC compared against aware
  5. Unparseable values skipped, valid ones retained
  6. Only unparseable values → (None, None)
• No negative-offset test case. A case like "2025-01-01T00:00:00-05:00" vs "2025-01-01T04:00:00Z" (same instant, different representation) would exercise negative offsets, but this is not a gap since fromisoformat handles negative offsets identically to positive ones — nice-to-have at most.

Potential Concerns (non-blocking)

1. value.replace("Z", "+00:00") — This replaces all occurrences of the character Z in the string, not just a trailing one. In practice, ISO 8601 timestamps won't contain a capital Z anywhere except as the UTC designator at the end, so this is safe. A more defensive approach would be value[:-1] + "+00:00" if value.endswith("Z") else value, but the current approach matches widespread convention and is fine.

2. No logging on unparseable skip — If a rollout file contains garbled timestamps, the helper silently discards them. For debugging production ingestion issues, a logger.debug on skip could be useful, but this is a future enhancement, not a requirement for this fix.

Verdict

Approve. This is a clean, well-scoped bug fix with good test coverage. The implementation is correct, follows project conventions, and doesn't introduce unnecessary complexity. No blocking issues found.

Greptile Summary

Fixes incorrect started_at/ended_at derivation caused by lexicographic string comparison of ISO 8601 timestamps with differing UTC offsets or sub-second precisions. The fix introduces parse_iso8601() / min_max_timestamps() helpers in utils.py and swaps the raw min/max calls in all three ingesters.

• parse_iso8601() normalises the "Z" suffix for Python < 3.11 compatibility, coerces naive datetimes to UTC, and returns None for unparseable values so callers can skip them silently.
• min_max_timestamps() builds a (datetime, original_str) pair list, runs min/max by the parsed datetime, and returns the original strings unchanged — preserving the output contract.
• codex.py retains its existing session_meta[\"timestamp\"] priority for started_at, now falling back to earliest from the helper instead of a lexicographic min.

Important Files Changed

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[timestamps: list of str] --> B{any items?}
    B -- No --> C[return None, None]
    B -- Yes --> D[for each timestamp string]
    D --> E[parse_iso8601\nreplace Z → +00:00\nfromisoformat]
    E -- ValueError --> F[skip entry]
    E -- Success --> G{tzinfo is None?}
    G -- Yes --> H[replace tzinfo=UTC]
    G -- No --> I[keep as-is]
    H --> J[append datetime, original_str]
    I --> J
    F --> D
    J --> K{all skipped?}
    K -- Yes --> C
    K -- No --> L[min by datetime → earliest string\nmax by datetime → latest string]
    L --> M[return earliest, latest]

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into worktree-issue-..." | Re-trigger Greptile}

packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/pi_coding_agent.py:203-204

started_at=min(timestamps) if timestamps else None,
ended_at=max(timestamps) if timestamps else None,

looks like this file has the same min(timestamps) / max(timestamps) pattern that was fixed in the other three ingesters - worth including in this PR since the helper already exists?