refactor(infra): canonicalize session usage timestamps#100687
Conversation
Co-authored-by: sheyanmin <44184140+sheyanmin@users.noreply.github.com>
…ge-timestamp-canonical # Conflicts: # CHANGELOG.md
|
Codex review: needs changes before merge. Reviewed July 6, 2026, 3:25 AM ET / 07:25 UTC. Summary PR surface: Source -6, Tests +39, Docs +1. Total +34 across 3 files. Reproducibility: yes. at source level. Current main still returns Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Land the canonical Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main still returns Is this the best way to solve the issue? Yes for the runtime and test path: reusing Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against de30d2f5c333. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source -6, Tests +39, Docs +1. Total +34 across 3 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
…ge-timestamp-canonical
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d22f63bc08
ℹ️ 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".
| - **Small-context compaction:** cap the effective reserve against the known model context window so small local models do not enter compaction from the first token. (#100621) Thanks @vincentkoc. | ||
| - **Plugin install diagnostics:** suppress the misleading hook-pack fallback after plugin install failures only when the hook manifest is absent, while preserving actionable malformed hook-pack errors. (#100554) Thanks @vincentkoc. | ||
| - **Config validation diagnostics:** emit each unchanged sanitized validation-warning payload once per config path, reset deduplication after a clean validation, and preserve the warning fingerprint across transient invalid reads and failed refreshes. (#100569, #25574) Thanks @vincentkoc. | ||
| - **Session usage logs:** normalize malformed transcript timestamps before sorting and Gateway serialization so invalid dates cannot surface as null usage-log times. (#99418) Thanks @sheyanmin. |
There was a problem hiding this comment.
For this normal infra fix, CHANGELOG.md should stay release-owned: AGENTS.md says normal PRs/direct fixes should carry release-note context in the PR body or commit message instead, and scripts/pr-lib/common.sh:34 makes changelog updates never required for changed files because release automation generates the file. Leaving this manual Unreleased entry can make release generation duplicate or reorder notes for #99418, so please drop it from the code change.
Useful? React with 👍 / 👎.
|
Merged via squash.
|
Co-authored-by: sheyanmin <44184140+sheyanmin@users.noreply.github.com>
Co-authored-by: sheyanmin <44184140+sheyanmin@users.noreply.github.com>
What Problem This Solves
Malformed transcript timestamps can become
NaNin session usage logs. Gateway JSON serialization then exposes that value asnull, and direct parsing also skips the existing nested message-timestamp fallback.This supersedes #99418 with a stable maintainer-owned branch after repeated contributor-branch rebases invalidated exact-head proof. It preserves @sheyanmin's authorship through a
Co-authored-bytrailer and changelog credit, and tightens the minimal guard merged in #99420.Why This Change Was Made
loadSessionLogsnow uses the same canonicalparseTimestamppath as session usage summaries. That preserves valid ISO timestamps, falls back to numeric nested message timestamps, and normalizes truly malformed entries to epoch zero before sorting or serialization.The regression test covers all three states in one transcript: malformed without fallback, malformed with nested fallback, and valid ISO input.
User Impact
Session usage detail logs always return finite numeric timestamps. Existing valid timestamps and nested fallback behavior remain unchanged.
Evidence
tbx_01kwv0vw0yy55xenj6evvanps7:CI=true pnpm test src/infra/session-cost-usage.test.ts— 65/65 passed.run_97d2116b617eoncbx_49e34d8a40d4(swift-shrimp) atd22f63bc08cff61d1f2cd25a32ae73b8ca32c093— finite/order/content assertions, byte-identical repeat and restart, valid-top and nested-removal mutations, and missing-session isolation all passed. Public network, no Tailscale/hydration/ambient credentials; lease stopped.28774705174passed after rerunning one unrelated flaky PTY assertion; 4,713 tests passed on the first attempt.git diff --check: passed.