Fix canonical Codex long-context pricing - #2862
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed August 11, 2026, 1:41 PM ET / 17:41 UTC. ClawSweeper reviewWhat this changesThe PR replaces stale in-window Codex pricing rows on rescans, avoids guessing long-context tiers from aggregate tokens, and propagates incomplete Codex costs through reports and CLI output. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 4 items remain Current main still uses aggregate fallback pricing, while this PR introduces the request-tier-safe path and targeted regression coverage. No discrete correctness defect was found in the rebased patch; real after-fix CodexBar proof is still required before merge. Likely related people: Peter Steinberger (high) and Xu Xiang (medium). Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans local Codex session/cache data, derives API-equivalent prices, then publishes daily, project, snapshot, and CLI summaries. The change is at the reconciliation boundary between cached request rows and those downstream aggregates. flowchart LR
A[Codex session records] --> B[Rescan and fork reconciliation]
B --> C[Request-level pricing rows]
C --> D[Daily and project reports]
D --> E[Usage snapshots]
E --> F[CLI text and JSON]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain request-scoped pricing where canonical ownership can be rebuilt, otherwise expose an explicitly unavailable aggregate while preserving known rows and tokens; verify it with a redacted real local rescan/CLI transcript. Do we have a high-confidence way to reproduce the issue? Yes—source inspection and the added deterministic fixture establish the current-main aggregate-fallback path: multiple short requests can be collapsed above a model threshold and priced as long-context usage. Is this the best way to solve the issue? Yes—the branch preserves request-level pricing where available and fails safely when it is not, which is narrower and safer than applying a tier to an aggregate. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e5528d452d4f. LabelsLabel 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 (1 earlier review cycle)
|
![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: 301f4c2e20
ℹ️ 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 thresholdTokens = self.codex[key]?.thresholdTokens ?? modelsDevLookup?.pricing.thresholdTokens | ||
| if let thresholdTokens, max(0, inputTokens) > thresholdTokens { | ||
| return nil |
There was a problem hiding this comment.
When any day/model lacks trusted rows and its aggregate input exceeds this threshold, the helper now returns nil, but buildCodexReportFromCache still sums every other priced model/day and exposes that partial sum as summary.totalCostUSD. Consequently, a 30-day report containing one affected fork-deduplicated day plus any normally priced day displays a numeric “last 30 days” cost that silently excludes the ambiguous usage instead of indicating that the total is unavailable; track the missing cost through the entry and summary aggregation rather than treating the remaining subtotal as complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The ambiguous model breakdown was omitted, but day, report, merge, snapshot, comparison-window, and CLI layers could still reconstruct a numeric subtotal from the remaining priced rows. The follow-up now propagates incompleteness through every enclosing Codex aggregate while retaining known breakdowns and token totals. Mixed-model, mixed-day, merge, snapshot, comparison-window, and CLI regressions cover the full path.
There was a problem hiding this comment.
Pull request overview
This PR fixes Codex cost inflation during canonical fallback by ensuring request-level pricing metadata (especially long-context tiering) is preserved through fork deduplication and rescan cache rebuilds, and by avoiding unsafe aggregate pricing when request tiers can’t be reconstructed.
Changes:
- Replace cached Codex pricing rows inside the scan window with newly parsed, fork-deduplicated rows (preventing stale in-window rows from leaking into rebuilt caches).
- Introduce a Codex-specific aggregate pricing helper that returns
nilwhen an aggregate would cross a long-context threshold (instead of mispricing the entire day/model as one long-context request). - Add regression tests covering window-expansion repair, long-context tier isolation, and ambiguous aggregate fallback omission; regenerate the parser fingerprint to force SQLite cache rebuilds.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodexBarTests/CostUsageScannerForkSplitTests.swift | Adds regression tests for rescan window row replacement, long-context tiering correctness, and aggregate fallback omission. |
| Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CacheHelpers.swift | Ensures cached rows within the scan window are not retained during rescan; switches canonical fallback to aggregate-safe pricing. |
| Sources/CodexBarCore/Vendored/CostUsage/CostUsagePricing.swift | Adds codexAggregateCostUSD to avoid applying request-scoped long-context tiers to day/model aggregates. |
| Sources/CodexBarCore/Generated/CodexParserHash.generated.swift | Bumps parser fingerprint to invalidate/rebuild persisted cost caches. |
| CHANGELOG.md | Documents the user-facing Codex pricing fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
301f4c2 to
c7972ec
Compare
|
Rebased and addressed the incomplete-total edge case. A missing cost for nonzero Codex usage now propagates through day, history, comparison-window, project/source, and CLI totals instead of exposing a subtotal. The requested real-account cache/CLI proof is intentionally not included: this contribution's privacy constraints prohibit reading or publishing real account data, provider sessions, browser state, or Keychain material. The revised PR body instead documents deterministic synthetic proof across the production layers from rescan and report construction through snapshot and CLI presentation, including explicit incomplete-pricing text and omitted ambiguous JSON cost fields. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Thanks for the detailed investigation and the strong synthetic coverage, @thomaschow19. This is now superseded by #2863, which landed as The landed repair preserves request-level base/long-context and Standard/Fast tiers, reconciles copied fork prefixes from existing persisted rows, keeps compatible SQLite history without a destructive rescan, and handles concurrent cache writers without rebuilding. It passed all 840 local test selections, exact-head CI, P2 autoreview, and ClawSweeper review. Your report and fixtures materially shaped the final fix, and the 0.49.3 changelog credits you. Closing this duplicate implementation in favor of the landed version. |
Summary
Root cause
Full rescans replaced canonical day totals but merged stale in-window pricing rows back into file caches. When row totals then exceeded canonical fork-owned totals, the fallback priced the day/model aggregate as one request, incorrectly applying the >272K tier to multiple short requests.
Review follow-up
An unavailable nonzero model cost now invalidates every enclosing Codex aggregate. Known model/day breakdown costs and token totals remain visible, while CLI text renders
Unavailable (incomplete pricing data)and JSON omits the ambiguous optional cost field.Privacy-safe behavioral proof
Validation intentionally uses isolated synthetic rollout and cache data across the production layers from rescan and report construction through snapshot and CLI presentation. It does not read a real Codex home, account, provider session, browser profile, or Keychain.
Validation
make test: all 840 discovered selections were exercised. The normal runner stopped in group 65/70 on unchangedUsageStoreCodexCostCatchUpTestsbackground-QoS starvation; groups 66–70 then passed separately (439 tests), and the affected suite passed 4/4 with deterministic cooperative scheduling. Its test overrides bypass the changed pricing paths.make check: passedgit diff --check: passedFixes #2858