Fix invalid Codex Standard/Fast cost splits - #2774
Conversation
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: deacc86536
ℹ️ 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".
| let splitIsUntrusted = splitTokenOverflow || splitTokenTotal > totalTokens | ||
| let standardCost = splitIsUntrusted ? nil : cachedStandardCost | ||
| let priorityCost = splitIsUntrusted ? nil : cachedPriorityCost |
There was a problem hiding this comment.
When copied priority rows make the split exceed the canonical token total, this marks the split costs as nil, but that newly activates the fallback at lines 1719–1723, which adds codexPrioritySurchargeNanosByDayModel to the base cost. That surcharge is generated from the same per-file priority rows as codexModeSplitMaps, so those copied rows can inflate it too; affected fork/subagent caches therefore continue to overreport cost even though their Standard/Fast fields disappear. The untrusted path should also discard the cached surcharge or recompute it from canonical ownership.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed August 8, 2026, 7:41 PM ET / 23:41 UTC. ClawSweeper reviewWhat this changesThis PR hides Standard/Fast cost and token splits whose cached token sum exceeds the canonical daily model total, then falls back to aggregate pricing. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 7 items remain Keep open, but block merge: the prior P1 remains on the unchanged head, and the branch targets a cost-cache flow that current main has since replaced. Likely related people: steipete (high-confidence current-path owner) and Xu Xiang (medium-confidence adjacent fork-accounting contributor). Priority: P2 Review scores
Verification
How this fits togetherCodexBar converts locally cached Codex usage into daily cost reports consumed by the menu-bar app and CLI. Canonical day/model totals and optional Standard/Fast row pricing must agree so copied fork or subagent rows cannot inflate displayed estimates. flowchart LR
A[Cached Codex usage] --> B[Canonical day model totals]
A --> C[Standard Fast row data]
B --> D[Cost report builder]
C --> E[Ownership validation]
E --> D
D --> F[Daily cost estimate]
F --> G[Menu bar and CLI views]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Rebase the fix onto the current read-time pricing architecture and reject the entire row-derived price, including any Fast uplift, when row tokens exceed canonical ownership; preserve aggregate canonical pricing as the fallback. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: a cache with an inflated split and nonzero cached priority surcharge follows the PR’s fallback into another copied-row-derived cost. The added fixture omits that surcharge, so it does not establish after-fix recovery. Is this the best way to solve the issue? No: hiding the split fields alone is insufficient because the fallback retains a related untrusted surcharge. The whole row-derived price must be rejected or recomputed from canonical ownership after rebasing. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 617c292f650a. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles)
|
|
Closing in favor of #2782, which ports the fix to the current read-time pricing path and addresses the copied-row Fast surcharge as well. Thanks for carrying forward and crediting the initial fix/regression-test approach from this PR. |
1 participant
Summary
When the cached Standard/Fast split maps are inflated by copied fork/subagent rows, the report currently prefers
standardCost + priorityCostover the canonical day/model total. This narrow report-boundary fix treats that split as untrusted and falls back to the existing canonical base-cost or canonical-token pricing path.buildCodexReportFromCache, a split is no longer published when the knownstandardTokens + priorityTokensexceeds the canonicaltotalTokensfor the same day/model, or when the Int token sum overflows.nilStandard/Fast token and cost fields and reuse the existingcachedBaseCost/ canonical-token pricing fallback. No proportional scaling, no ownership guessing.Tests
swift test --filter CostUsageScannerPriorityTests: 20 passed (3 new: invalid split, valid split, Int overflow)make check: SwiftFormat clean, strict SwiftLint clean, parser hash regenerated (coversCostUsageScanner+CacheHelpers.swift)make test: 69/69 shards passed, 822 selections, 0 failuresgit diff --check: cleanCloses #2754