fix: keep unrepresentable cost totals unavailable - #3501
Merged
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
steipete
added a commit
that referenced
this pull request
Sep 8, 2026
Replace unsafe floating-point boundary guards with failable integer conversion after each source's established rounding. Preserve valid neighboring fields and exact MiMo/OpenCodex integer payloads; saturate oversized finite subprocess timeouts. Reparse OpenCodex caches written by older numeric parsers once, then retain normal warm-cache reuse. Production LOC: -16. Changelog and docs included. Full make test: 1,035 selections and 87 groups, all first-pass; final integration 145 focused tests passed. make check, independent review, and final-head macOS/Linux CI passed. Report aggregation overflow is handled separately in #3501.
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 8, 2026, 4:02 AM ET / 08:02 UTC. ClawSweeper reviewWhat this changesUse checked token accumulation in OpenCodex and combined cost reports so overflowing totals remain unavailable while valid neighboring counts survive. Merge readiness✅ Ready for maintainer review This remains a useful, focused repair: main and v0.56.8 retain the unchecked arithmetic, and the related parsing PR addresses a separate failure. No blocking defect was found in the introduced changes. Priority: P2 Review scores
Verification
How this fits togetherCodexBar turns local usage records into daily, session, model, and rolling-window cost summaries for its app and CLI. This change protects the token arithmetic between parsed records and those reports. flowchart LR
A[OpenCodex usage records] --> C[Checked token accumulation]
B[Native cost reports] --> D[Combined report accumulation]
C --> E[Daily and session buckets]
C --> F[Rolling totals]
D --> E
E --> G[App and CLI cost reports]
F --> G
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain checked accumulation through each report pass, preserving explicit totals, valid token classes, and the existing serialized partial-data contract. Do we have a high-confidence way to reproduce the issue? Yes, from source: deriving a total from Int.max input tokens and one output token reaches unchecked addition on main. The author reports a failing regression; this review did not execute tests. Is this the best way to solve the issue? Yes. Reusing the existing checked accumulators and retaining overflow state until summary projection is a focused repair without introducing a new persistent format. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c3f3ea1faf70. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OpenCodex and combined cost reports could trap when individually representable token counts added up beyond the integer range. For example, deriving a total from input Int.max plus one output token crashes even though both fields are valid integers.
Reuse the existing checked optional-count accumulator and token-mix aggregation across derived totals, daily/model/session/hour buckets, and rolling-window totals. Keep overflow unavailable through the current aggregation pass, retain valid neighboring fields, and preserve explicit-total precedence and explicit zero. Summarize internal accumulators before projecting unavailable values into report rows, and avoid turning an unavailable present-day total into zero.
Remove duplicate sums and visibility flags: production LOC is net -125. Pricing, coverage, ordinary missing-data behavior, and serialized/cache formats remain unchanged. Materialized nil fields retain the existing partial-data contract; this patch does not add persistent overflow provenance. The regression reference now owns its original accumulator representation so it stays independent of the production refactor.
Validation: the new derived-total regression exits with signal 5 against main. The repaired focused run passed 113 tests in 8 suites, including same-day, cross-day, earlier-day overflow followed by valid rows, explicit zero, per-field preservation, and native-plus-OpenCodex report merging. Full make test passed all 1,036 selections across 87 groups on the first pass without retries or timeouts. make check and independent P0–P2 review passed. All checks passed for commit 14932f8: https://github.com/steipete/CodexBar/actions/runs/34202232775.
Includes changelog and documentation. This is the aggregation follow-up identified during #3486; that PR separately fixes numeric conversion and older parsed-cache reuse.