refactor(cli): switch compare to relayburn_sdk::normalize_since#421
Conversation
`burn compare` was the only presenter still carrying its own `normalize_since` + Hinnant civil-date math; the SDK has exposed `normalize_since` for a while and the sibling `summary` and `hotspots` presenters already use it. Delete the local helpers and their tests, drop ~380 LOC of duplicated date plumbing. The cli-golden snapshot still matches. Closes #333 https://claude.ai/code/session_01VFvKstujMoXwZkP5vwrHxQ
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoves the CLI's duplicate ChangesDeduplicate since-normalization logic
Canonicalize normalize_since in SDK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Keep canonical UTC millisecond cutoff for --since
Switching compare to relayburn_sdk::normalize_since regresses timestamp filtering correctness for common --since inputs. The SDK helper currently returns relative ranges in second precision (...SSZ) and passes ISO strings through after only a YYYY-MM-DD shape check (query_verbs.rs), so values like 2026-05-06T00:00:00-07:00 are no longer normalized to UTC and values like ...00Z no longer widen to ...00.000Z. Because ledger filtering is lexicographic on ts >= ?, this can silently exclude same-second rows with milliseconds and misorder offset timestamps, changing burn compare results.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-11: The changelog entry for `relayburn-cli` currently includes
internal implementation detail ("uses relayburn_sdk::normalize_since" and "Drops
~250 LOC of duplicated date math"); update the `CHANGELOG.md` bullet for
`relayburn-cli: burn compare` to be user-focused by removing references to
`relayburn_sdk::normalize_since` and the LOC drop and instead describe the
user-visible impact (e.g., "improved date handling to match summary and
hotspots" or similar), keeping the entry concise and impact-first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35803fba-ea64-4f92-8232-b3600ac586a5
📒 Files selected for processing (2)
CHANGELOG.mdcrates/relayburn-cli/src/commands/compare.rs
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| build_compare_table, has_minimum_fidelity, load_pricing, provider_for, summarize_fidelity, | ||
| AnalyzeCompareOptions as CompareOptions, CompareCell, CompareTable, EnrichedTurn, | ||
| FidelityClass, FidelitySummary, Ledger, LedgerOpenOptions, ProviderFilter, Query, | ||
| build_compare_table, has_minimum_fidelity, load_pricing, normalize_since, provider_for, |
There was a problem hiding this comment.
🟡 SDK normalize_since omits millisecond suffix, breaking lexicographic ts >= filtering against .000Z ledger rows
The old local normalize_since formatted relative-range cutoffs as YYYY-MM-DDTHH:MM:SS.000Z (24 chars, with milliseconds). The SDK's replacement normalize_since (crates/relayburn-sdk/src/query_verbs.rs:77) uses format_iso_z which emits YYYY-MM-DDTHH:MM:SSZ (20 chars, no milliseconds). The ledger stores timestamps with .000Z precision (crates/relayburn-sdk/src/ledger/schema.rs:47 — ts TEXT NOT NULL), and the query filter at crates/relayburn-sdk/src/ledger/reader.rs:185 uses ts >= ? which is a lexicographic comparison in SQLite. At character position 19, . (0x2E) sorts before Z (0x5A), so "2026-05-05T12:00:00.000Z" < "2026-05-05T12:00:00Z" — meaning rows at the exact cutoff second (and any sub-second variants like .500Z) are incorrectly excluded. This shifts the effective --since boundary by ~1 second for every relative range (7d, 24h, etc.). The deleted tests at old lines 1289–1296 (normalize_since_relative_emits_milliseconds) and 1308–1317 (normalize_since_relative_format_is_lex_compatible_with_ledger_rows) specifically guarded against this regression.
Prompt for agents
The SDK's normalize_since (in crates/relayburn-sdk/src/query_verbs.rs, function format_iso_z around line 124) formats relative-range cutoffs as YYYY-MM-DDTHH:MM:SSZ without milliseconds. Ledger rows use .000Z format, and the SQL filter ts >= ? is lexicographic, so ...SS.000Z sorts before ...SSZ causing rows at the cutoff second to be incorrectly excluded. The fix should be in the SDK's format_iso_z to emit .000Z (matching the ledger format), which would fix this for compare, summary, hotspots, and sessions_list simultaneously. Change the format_description macro in format_iso_z to include [subsecond digits:3] or manually append .000Z.
Was this helpful? React with 👍 or 👎 to provide feedback.
| build_compare_table, has_minimum_fidelity, load_pricing, provider_for, summarize_fidelity, | ||
| AnalyzeCompareOptions as CompareOptions, CompareCell, CompareTable, EnrichedTurn, | ||
| FidelityClass, FidelitySummary, Ledger, LedgerOpenOptions, ProviderFilter, Query, | ||
| build_compare_table, has_minimum_fidelity, load_pricing, normalize_since, provider_for, |
There was a problem hiding this comment.
🔴 SDK normalize_since passes ISO timestamps with timezone offsets through raw, causing wrong lexicographic cutoff
The old local normalize_since fully parsed ISO timestamps and re-emitted them as UTC (...Z), converting offsets like -07:00 to the correct UTC instant (e.g., 2026-05-06T00:00:00-07:00 → 2026-05-06T07:00:00.000Z). The SDK's replacement (crates/relayburn-sdk/src/query_verbs.rs:83-86) only checks looks_like_iso (a YYYY-MM-DD prefix check) and returns the raw string as-is. When a user passes --since "2026-05-06T00:00:00-07:00" to burn compare, the raw string is compared lexicographically against ledger ts values (at crates/relayburn-sdk/src/ledger/reader.rs:185). The -07:00 suffix causes the comparison to treat the cutoff as midnight UTC instead of 7am UTC — a 7-hour discrepancy that includes or excludes the wrong turns. The deleted tests at old lines 1243–1260 (normalize_iso_converts_negative_offset_to_utc / normalize_iso_converts_positive_offset_to_utc) specifically verified this offset-to-UTC conversion.
Prompt for agents
The SDK's normalize_since (crates/relayburn-sdk/src/query_verbs.rs:80-86) passes ISO strings through without converting timezone offsets to UTC. This causes incorrect lexicographic comparison in the SQL filter ts >= ? when users provide offset timestamps like 2026-05-06T00:00:00-07:00. The fix should be in the SDK's normalize_since to parse ISO timestamps with offsets and re-emit as UTC ...Z format (similar to the deleted normalize_iso_to_utc_z in compare.rs). This would fix the issue for compare, summary, hotspots, and all other commands using build_query. Alternatively, the deleted offset-normalization code from compare.rs could be moved into the SDK's normalize_since.
Was this helpful? React with 👍 or 👎 to provide feedback.
Codex / Devin / coderabbit flagged a latent regression on PR #421: the SDK's `normalize_since` returned relative ranges as `...SSZ` (no millis) and passed ISO inputs through unchanged. Because the ledger filter is `ts >= ?` against `.mmmZ` rows in SQLite (lex-compared), this silently dropped same-second turns (`.` < `Z`) and misordered offset-bearing cutoffs (e.g. `-07:00` sorting before any UTC row). This was already true for `burn summary` and `burn hotspots`; PR #421 just made `burn compare` adopt the same behavior. The fix is in the SDK so all three presenters get consistent, correct date filtering: - Relative ranges emit canonical `YYYY-MM-DDTHH:MM:SS.000Z` (24 chars). - ISO inputs parse and re-emit as UTC `.mmmZ`: offsets get converted (`...-07:00` → `...07:00:00.000Z`), no-fraction widens to `.000Z`, sub-ms truncates to ms, `Z`/`z` and date-only forms supported. - Garbage errors out (no more `looks_like_iso` permissive pass-through). Tests cover each shape and the lex-compare property against ledger rows. Also rewords the CHANGELOG bullet to be user-impact-first per coderabbit's note. https://claude.ai/code/session_01VFvKstujMoXwZkP5vwrHxQ
2 participants
Summary
burn comparewas the only presenter still carrying its ownnormalize_since+ Hinnant civil-date math. The SDK exposesrelayburn_sdk::normalize_sinceand the siblingsummary/hotspotspresenters already use it.compare.rsover and delete the local helpers (normalize_since,parse_relative,normalize_iso_to_utc_z,format_iso_z_ms,format_iso_z_ms_signed,ymd_to_days,days_to_ymd) plus their unit tests. Net change: +3 / -383 LOC incompare.rs.util/time.rs:civil_from_daysis still referenced byiso_now/iso_from_system_time(used inharnesses/claude.rs) so it stays.Test plan
cargo test --workspace— 774 tests pass, includingtests/golden.rs::golden_diff_against_cli_snapshotswhich is the gate the issue called out for--sinceregressions.cargo build -p relayburn-cli.Closes #333
https://claude.ai/code/session_01VFvKstujMoXwZkP5vwrHxQ
Generated by Claude Code