fix: preserve Codex request-tier pricing across forks - #2863
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
@clawsweeper review Please review exact head |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs maintainer review before merge. Reviewed August 11, 2026, 2:28 PM ET / 18:28 UTC. ClawSweeper reviewWhat this changesThis PR reconciles forked Codex usage rows at request granularity, avoids unsafe threshold pricing of aggregates, and safely adopts compatible SQLite caches under contention. Regression provenancePossible regression — probable (reviewed change; known regression link). No predecessor PR is attributed. Merge readinessKeep open for normal maintainer merge handling. The exact head addresses the prior SQLite-lock blocker and still supplies the requested fix for the linked Codex pricing defect. Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans Codex sessions into a local usage cache, reconciles forked rows into canonical daily/model usage, and produces CLI and menu-bar cost reports. The changed cache-open path also controls whether existing local usage history is reused or rebuilt. flowchart LR
A[Codex session files] --> B[Local usage cache]
B --> C[Fork ownership reconciliation]
C --> D[Request-level cost calculation]
D --> E[Daily and project reports]
B --> F[Cache compatibility validation]
F --> B
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Keep request-level prices where ownership is provable, leave thresholded aggregates unavailable when boundaries are lost, and preserve locked compatible caches for retry rather than rebuilding them. Do we have a high-confidence way to reproduce the issue? Yes—source fixtures deterministically combine two 200K requests under a 272K threshold and exercise copied fork prefixes; this read-only review did not execute them. Is this the best way to solve the issue? Yes. The patch preserves request boundaries when evidence is exact and fails closed for thresholded aggregates when those boundaries cannot be established. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e5528d452d4f. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
@clawsweeper re-review Please review exact head |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Resolved the remaining SQLite lock finding at exact head
Final proof: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Landed as The red repro was two 200K short requests: request-level expected cost was $2.006, while the old day-aggregate fallback produced $4.009. The final path reconciles copied fork prefixes from canonical request rows, preserves valid request-tier pricing, and refuses ambiguous long-context pricing on aggregates. Proof: Compatible shipped-predecessor SQLite caches retain complete rows/history without reparse while sidecar generation rotates. Under external writer locks, current stores remain readable and predecessor stores are preserved for safe retry/adoption. There is no schema change and no persisted estimated USD. |
Problem
Fork children can retain copied parent-prefix usage rows even when canonical per-file/day/model token ownership has already removed that prefix. When the physical rows exceeded canonical totals, the report rejected row pricing and priced the whole canonical day/model aggregate as one request. For GPT-5.6 Sol, that let many sub-272K requests cross the 272K long-context threshold only after aggregation and nearly doubled the estimate.
Synthetic red proof on
e5528d4: two independent 200K-input requests should cost$2.006; the fallback priced their 400K aggregate at$4.009, a$2.003overcount.Root cause
buildCodexReportFromCacheconcatenated physical pricing rows across files, then used only a global token-total guard. Once that guard rejected fork-inflated rows, request-scoped threshold pricing received day/model aggregates. Project and session rollups happened to price smaller groups, so grouping changed the result. Existing SQLite rows already retained the full request boundaries needed to repair the report on read.Fix
usage.daysownership, using persisted array/SQLiterow_indexorder. Exact token-bearing sets retain order; inflated sets may retain only the newest contiguous exact suffix. Zero-owned groups use the empty suffix.b975eb705f905b9apredecessor store in place only when its derived version,quick_check, and incremental auto-vacuum all validate. Validation and metadata/version rotation run under oneBEGIN IMMEDIATEtransaction, so concurrent openers cannot observe a torn adoption pair. Unknown, mismatched, or corrupt stores still rebuild.Proof
swift test --filter CostUsageScannerForkSplitTestsfailed on current main with the synthetic$2.006request sum versus$4.009aggregate result.swift test --filter CostUsageScannerForkSplitTests— 14/14 tests.swift test --filter CostUsagePricingTests— 48/48 tests.swift test --filter CostUsageScannerPriorityTests— 17/17 tests.swift test --filter CostUsagePerformanceGateTests— 26/26 tests, including zero session-head reparses during compatible warm adoption.swift test --filter CostUsageStoreTests— 64/64 tests, including 16 concurrent predecessor openers with zero rebuilds and preserved tables/metadata.make checkand current generated parser-hash check.make test— 840 selections, 70/70 groups, no retries, failures, or timeouts.git diff --checkand configured Codex branch autoreview through P2.usage_rowsrequest payloads without a schema change.Fixes #2858