fix(codex): clamp cached tokens to input in CodexUsageRow - #3215
fix(codex): clamp cached tokens to input in CodexUsageRow#3215Yuxin-Qiao wants to merge 1 commit into
Conversation
TokScale clamps cached_input_tokens to input_tokens (clamped_cached = min(cached, input)) to prevent inflated totals when malformed JSONL reports more cached than input. CodexBar previously stored cached verbatim and only clamped reasoning, so a malformed row could inflate totalTokens and cost. This mirrors crates/tokscale-core/src/sessions/codex.rs:194-210 and keeps input/cached/output non-negative.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: 046448e649
ℹ️ 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".
| self.input = max(0, input) | ||
| // Clamp cached to not exceed input (toks scale: clamped_cached = min(cached, input)) | ||
| // prevents inflated totals when malformed JSONL reports more cached than input. | ||
| self.cached = min(max(0, cached), max(0, input)) |
There was a problem hiding this comment.
When a token_count has cached_input_tokens > input_tokens—the exact malformed case targeted here—the parser's add(...) calls still store the unclamped cached count in usage.days, while this initializer stores a different value in codexRows. codexCanonicalPricingRows then cannot reconcile the row with its aggregate and drops it, so reports still expose the inflated cacheReadTokens from usage.days; they also lose row-only reasoning and can return no cost for priority or authoritative-cost groups. Apply the clamp before both representations are recorded, and cover the scanner-to-report path rather than only constructing this row.
AGENTS.md reference: AGENTS.md:L19-L25
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 other
|
Codex review: needs real behavior proof before merge. Reviewed August 26, 2026, 3:07 PM ET / 19:07 UTC. ClawSweeper reviewWhat this changesThe PR clamps cached Codex session tokens to the input-token count when creating a parsed usage row. Merge readiness⛔ Blocked until stronger real behavior proof is added - 5 items remain Keep open: the clamp is applied only to per-event rows, while the scanner still records the malformed cached count in daily aggregates. Those representations then fail reconciliation, so the intended reporting fix is incomplete. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar scans local Codex JSONL sessions into daily aggregate counters and per-event rows, then reconciles them to calculate usage and cost reports. This change affects the per-event row branch of that pipeline. flowchart LR
A[Codex JSONL session] --> B[Token-count parser]
B --> C[Daily aggregate counters]
B --> D[Per-event usage rows]
C --> E[Row and aggregate reconciliation]
D --> E
E --> F[Usage and cost report]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Normalize cached tokens once at scanner ingestion before writing either aggregate counters or usage rows, and cover the complete scanner-to-report path with a malformed JSONL fixture. Do we have a high-confidence way to reproduce the issue? Yes—source inspection shows that a token_count with cached tokens above input is stored unclamped in the daily aggregate before the row initializer clamps only the row. Is this the best way to solve the issue? No—the clamp must be applied before both aggregate and row writes; changing only the row cannot preserve the reconciliation invariant. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cf79d1310493. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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
|
steipete
left a comment
steipete
left a comment
There was a problem hiding this comment.
Reviewed head 046448e. The aggregate/row mismatch already identified in review is a blocker: the scanner records the raw cached delta in the daily aggregate before constructing the newly clamped row, so reconciliation can discard the corrected rows.
There is also a valid-data case to preserve. The bare-usage parser currently subtracts cached tokens from prompt input before constructing its totals. A response with prompt=120 and cached=100 therefore reaches this initializer as input=20, cached=100. A universal initializer clamp would turn only the row into cached=20 and disagree with its aggregate even though the original response is valid. Please settle that representation at ingestion and cover both native token_count and bare-usage paths through scanner, persistence, reload, and final reporting. The initializer arithmetic alone does not establish the displayed token or cost result.
The generated parser-hash change also needs an upgrade strategy consistent with the no-rebuild guarantees in #3051. The current 0.55.1 hash is not a compatible predecessor, so a mechanical refresh would rebuild existing databases; simply allowlisting it would retain malformed persisted aggregates. Please prove selective repair or another bounded migration that preserves unaffected history and scan offsets, and include valid cached-heavy, malformed, incremental, and SQLite-upgrade regressions before rebasing this for merge.
|
Closing this PR based on review feedback:
Thanks for the thorough review @steipete! |
2 participants
Summary
cachedtomin(cached, input)inCodexUsageRow.initto prevent inflated totals when malformed~/.codex/sessions/*.jsonlreports morecached_input_tokensthaninput_tokensinput/outputnon-negative and preserve existingreasoningclampCodexParserHash(hashcfd84d13 -> 5dc8605a)Mirrors TokScale
crates/tokscale-core/src/sessions/codex.rs:194-210(clamped_cached = min(cached, input); clamped_reasoning = min(reasoning, output)).TokScale reference
cached_input_tokens/cache_read_input_tokensascachedand clamps toinputbeforeTokenBreakdownconversion.cachedverbatim and only clampedreasoning, so a single malformedtoken_countrow could inflatetotalTokensandcostUSDfor that day.Evidence (text, no screenshots)
make checkpass (2032 lint files, 0 violations, parser hash current5dc8605a458f782f,Provider manifests 69 providers):2 files changed, 6 insertions(+), 4 deletions(-)(onlyCostUsageScanner.swift+ generated hash).Testing
make checkfull pass (see above)