Fix token cost estimation for non-Anthropic models in Claude logs (#2393) - #2413
Fix token cost estimation for non-Anthropic models in Claude logs (#2393)#2413Yuxin-Qiao wants to merge 41 commits into
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: 40c26b9f2d
ℹ️ 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".
| inputTokens: totalPromptTokens, | ||
| cachedInputTokens: cacheReadInputTokens, | ||
| outputTokens: outputTokens, | ||
| cacheWriteInputTokens: 0, |
There was a problem hiding this comment.
When a Claude Code log row for a GPT/OpenAI model includes nonzero cache_creation_input_tokens (for example gpt-5.6, which has cache-write rates in the bundled Codex table), this fallback folds those tokens into totalPromptTokens but then passes cacheWriteInputTokens: 0. codexCostUSD therefore bills those cache writes as ordinary uncached input instead of the cache-write rate, so cost totals are wrong for those proxy logs. Pass cacheCreationInputTokens through as cacheWriteInputTokens rather than dropping it.
Useful? React with 👍 / 👎.
|
|
||
| publish-release-assets: | ||
| runs-on: ubuntu-24.04 | ||
| needs: build-cli |
There was a problem hiding this comment.
In release-cli.yml release runs, publish-release-assets and update-homebrew-tap both only depend on build-cli, so after the matrix finishes they can run in parallel. Since the tap workflow is dispatched with an artifact_template that points at GitHub release assets, it can start resolving or publishing the formula before this new publisher job has completed gh release upload, causing the Homebrew update to fail or reference assets that are not available yet. Make the tap job depend on publish-release-assets (or keep uploading before dispatch).
Useful? React with 👍 / 👎.
40c26b9 to
6d22c2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d22c2ea7f
ℹ️ 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".
| if let lookup = self.modelsDevLookupAnyProvider( | ||
| model: key, | ||
| catalog: modelsDevCatalog, | ||
| cacheRoot: modelsDevCacheRoot) |
There was a problem hiding this comment.
When this new any-provider fallback prices a Claude Pi-compatible row via models.dev for a Google or DeepSeek model, PiSessionCostScanner.pricingContext still fingerprints only anthropic and openai, while buildReport reuses complete cached costNanos without recomputing. After a models.dev price change for one of the providers added here, the Pi cache remains valid and continues showing stale costs for those rows; include every provider this fallback can read in the pricing key before caching those computed costs.
Useful? React with 👍 / 👎.
6d22c2e to
3fce373
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fce373039
ℹ️ 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".
| L("≈%d full 5h window of weekly left"), | ||
| 1) | ||
| } else { | ||
| String.localizedStringWithFormat( | ||
| L("≈%d full 5h windows of weekly left"), |
There was a problem hiding this comment.
By splitting the existing .stringsdict lookup into new standalone L(...) keys, these strings no longer match the localized plural entries, which are still keyed by the original combined sentence (and the verdict key has the same issue below). In any non-English locale this falls back to the English key text for the session-equivalent details instead of using the translated resources; keep formatting through the existing stringsdict key or add localized entries for every new key.
Useful? React with 👍 / 👎.
| if let lookup = self.modelsDevLookupAnyProvider( | ||
| model: model, | ||
| catalog: modelsDevCatalog, | ||
| cacheRoot: modelsDevCacheRoot) |
There was a problem hiding this comment.
This expands Codex-session pricing to models.dev providers beyond OpenAI, but the regular Codex daily cache is still keyed through CostUsageScanner.codexPricingKey, which fingerprints only OpenAI pricing. For a Codex history row priced via this fallback, such as a deepseek-r1 model, a later DeepSeek/Google/Anthropic models.dev price change will leave the cached costNanos valid and reports will continue showing the stale cost until users force-rescan or clear the cache; include all providers reachable from this fallback in the Codex pricing key or bump the cache formula.
Useful? React with 👍 / 👎.
|
I built the latest version of the PR and the calculations might be off. GPT 5.6 sol in Codex GPT 5.6 sol in claude cli The estimated token cost is way higher in Claude cli. It should also really be counted towards Codex usage, no Claude |
|
I tested with CLIProxyAPI correctly separates uncached input and cache-read tokens. However, Claude CLI writes multiple transcript rows for a single response—typically CodexBar currently deduplicates only when both IDs exist. Without In my sample:
The final row often reclassifies interim uncached input as cache reads, so summing the rows inflates cost significantly. Suggested fix: when Relevant code:
A regression test with interim and final rows sharing a |
clawsweeper
Bot
added
proof: 📸 screenshot
|
Codex review: needs real behavior proof before merge. Reviewed August 1, 2026, 6:21 PM ET / 22:21 UTC. ClawSweeper reviewWhat this changesThe branch replaces Claude transcript-row deduplication with session- and alias-aware reconciliation, invalidates Claude and Vertex caches, and also changes subprocess, CI-install, and performance-test behavior. Merge readiness⛔ Blocked until stronger real behavior proof is added - 8 items remain Keep this PR open for a narrower re-review: it addresses a real Claude transcript deduplication defect, but it still claims to close an unresolved provider-pricing issue, includes unrelated CI behavior, and lacks after-fix evidence from an affected real transcript. Likely related people: Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans Claude JSONL transcript files, turns usage rows into cached per-model token and cost data, then renders daily usage totals. This PR changes the row-identity layer that decides which streamed or copied transcript rows contribute to those totals. flowchart LR
A[Claude JSONL transcripts] --> B[Claude usage scanner]
B --> C[Row identity and session reconciliation]
C --> D[Claude and Vertex cache]
D --> E[Daily token and cost report]
E --> F[CodexBar usage display]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep a focused single-ID Claude-stream deduplication and cache-invalidation repair with regression coverage, remove unrelated CI/process/performance changes, and track model normalization and cross-provider pricing separately. Do we have a high-confidence way to reproduce the issue? No live high-confidence reproduction was provided for this PR revision. Current main source clearly makes message-only rows unkeyed during incremental merging, but the branch needs a redacted affected Claude-proxy transcript proving the after-fix initial scan and append refresh. Is this the best way to solve the issue? No. Sharing single-ID identity semantics across initial parsing and incremental merging is the right repair boundary, but the current alias-reconciliation expansion, unrelated automation changes, and provider-pricing closing claim make this broader than the safest solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 78523f4ad890. 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 (44 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
9d93fe0 to
90bf63f
Compare
…eipete#2393) and add swiftly retry
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90bf63f306
ℹ️ 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".
| if let messageId, let requestId { | ||
| return "\(messageId):\(requestId)" | ||
| } | ||
| return messageId ?? requestId |
There was a problem hiding this comment.
When a stream with only messageId or requestId appends after its file has already been cached, this parser deduplicates the newly read chunk by that lone ID, but mergeClaudeRows still calls claudeInFileKey, which returns a key only when both IDs exist. The cached cumulative snapshot and the appended cumulative snapshot therefore both survive and are summed, inflating tokens and cost during normal incremental refreshes; use the same single-ID key semantics in the merge path.
Useful? React with 👍 / 👎.
| if let messageId, let requestId { | ||
| return "\(messageId):\(requestId)" | ||
| } | ||
| return messageId ?? requestId |
There was a problem hiding this comment.
When a proxy omits one identifier and reuses the other—especially when it writes an empty-string placeholder—this turns every such row into the same dictionary entry and retains only the last one. The existing claude counts different request ids separately test already establishes that a message ID alone is not sufficient to distinguish requests, so logs with that request field omitted will now undercount unrelated assistant calls; validate that a lone ID is nonempty and uniquely identifies a stream before using it as a deduplication key.
Useful? React with 👍 / 👎.
| let key: String? = { | ||
| if let messageId, let requestId { | ||
| return "\(messageId):\(requestId)" | ||
| } | ||
| return messageId ?? requestId |
There was a problem hiding this comment.
For users upgrading with affected rows already stored in the Claude/Vertex v6 cache, unchanged JSONL files are skipped by processClaudeFile, and these providers have neither a producer key nor an artifact-version change to invalidate their cached rows. Consequently the new deduplication behavior is not applied to existing logs until a force/full rescan happens, leaving previously overcounted usage visible after the upgrade; rotate the Claude/Vertex cache version or add an equivalent migration trigger.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
…teipete#2393) - Share one nonempty single-ID fallback key between in-file parsing and incremental cache merging so appended cumulative rows replace instead of double-counting - Treat empty lone IDs as missing to avoid collapsing unrelated rows - Bump Claude/Vertex artifact version to 7 so existing caches rebuild - Add append-refresh and cache-migration regression coverage Co-authored-by: Cursor <cursoragent@cursor.com>
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Addressed the pair-transition thread: when the persisted alias bridge shows pair(m1,r) -> req(r) -> pair(m2,r) is one stream, the superseding row now retires the old pair canonical entry instead of emitting both cumulative totals. Regression test added; 64/64 related tests pass, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6129d33176
ℹ️ 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".
| if let messageId = Self.claudeNonEmptyID(row.messageId), | ||
| let aliasRequestId = row.aliasRequestIds?.first | ||
| { | ||
| return "pair:\(Self.claudeEscapeKeyComponent(messageId)):\(Self.claudeEscapeKeyComponent(aliasRequestId))" |
There was a problem hiding this comment.
When one transcript ends with pair(m,r)=100 -> msg(m)=200 and a lexicographically earlier fork contains only the original pair(m,r)=100, this maps both cached rows to the same canonical key. reconciledClaudeRows then applies the existing path-based claudeRowWins before equivalence-class consolidation, so the fork's stale 100-token row wins and the latest 200-token row is discarded, underreporting tokens and cost. Resolve collisions involving alias-derived partial rows using cumulative recency/size rather than arbitrary path ordering.
Useful? React with 👍 / 👎.
|
Addressed the fork-reconciliation thread: when a canonical-key collision involves an alias-derived row, the larger cumulative row now wins instead of path ordering, so a lexicographically earlier fork with stale totals can no longer discard the latest snapshot. Regression test added; 65/65 related tests pass, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7389fc1eb
ℹ️ 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".
|
|
||
| if let messageId, !messageId.isEmpty { | ||
| if let requestId, !requestId.isEmpty { | ||
| return "pair:\(messageId):\(requestId)" |
There was a problem hiding this comment.
When a proxy JSONL file contains distinct calls in sessions A and B that reuse the same (messageId, requestId) pair, this key is identical for both rows. claudeInsertRow correctly filters A out of B's sessionClassKeys, but then its no-representative branch overwrites keyedRows[key], so A disappears and only B's tokens and cost are reported; give the in-file storage key a session component while keeping any cross-file canonical key separate if copied-history reconciliation must ignore sessions.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if Self.claudeRowTotalTokens(row) >= Self.claudeRowTotalTokens(representative) { | ||
| var mergedRow = row |
There was a problem hiding this comment.
Fresh evidence beyond the reviewed A/B-before-sessionless case is the A → sessionless → B order: when the larger sessionless chunk replaces A, initializing mergedRow from row discards A's sole known sessionId, and all aliases are then stored with a nil session. A subsequent B chunk matches those nil-session aliases as wildcards and replaces the whole class, reporting only B instead of A and B; when a wildcard merge has exactly one known session, preserve that session or equivalent class metadata.
Useful? React with 👍 / 👎.
clawsweeper
Bot
removed
the
merge-risk: 🚨 other
|
Addressed both threads:
Regression tests added for both; 67/67 related tests pass, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e2cda5182
ℹ️ 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".
| if aliasInvolved { | ||
| if Self.claudeRowTotalTokens(row) >= Self.claudeRowTotalTokens(existing.row) { | ||
| winners[canonicalKey] = candidate |
There was a problem hiding this comment.
When a parent row and a copied sidechain/subagent row share a canonical pair and either row carries persisted aliases, this branch bypasses claudeRowWins and selects solely by token count. A larger—or equally sized but later-visited—sidechain row can therefore replace the parent even though cross-file reconciliation otherwise deliberately prefers non-sidechain parent rows, causing duplicated transcript data to determine the reported usage. Apply the provenance precedence before comparing cumulative alias snapshots.
Useful? React with 👍 / 👎.
|
Addressed the precedence thread: alias-involved canonical collisions now apply @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f54ad51d2f
ℹ️ 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".
| if let messageId = claudeNonEmptyID(row.messageId), | ||
| let requestId = claudeNonEmptyID(row.requestId) | ||
| { | ||
| return "pair:\(self.claudeEscapeKeyComponent(messageId)):\(self.claudeEscapeKeyComponent(requestId))" |
There was a problem hiding this comment.
When one JSONL file contains distinct sessions that reuse the same message/request pair, both session-qualified rows returned by parseClaudeFile map to this session-free canonical key, so reconciledClaudeRows keeps only one winner and the daily report undercounts usage. Fresh evidence beyond the earlier in-file-key issue is that the final reconciliation re-collapses the rows even though same pair in different sessions stays distinct in file establishes they are separate calls; retain session identity when resolving duplicates from the same file while still reconciling copied histories across files.
Useful? React with 👍 / 👎.
| let sessionSuffix = Self.claudeNonEmptyID(row.sessionId) | ||
| .map { "@\(Self.claudeEscapeKeyComponent($0))" } ?? "" |
There was a problem hiding this comment.
When a stream transitions pair(m1,r) without a session, then req(r) with session A, then pair(m2,r) with session A, this suffix is derived from the newest row, so retirement searches for pair:m1:r@A although the old canonical key is pair:m1:r. The old 150-token snapshot therefore remains alongside the new 300-token snapshot. Fresh evidence beyond the earlier all-one-session transition is the sessionless-to-known-session change; retirement must locate aliases using the stored pair's session-qualified and sessionless identities.
Useful? React with 👍 / 👎.
|
Addressed both threads:
70/70 related tests pass, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e40fc43fc0
ℹ️ 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".
| if existing.path == path, | ||
| Self.claudeNonEmptyID(existing.row.sessionId) | ||
| != Self.claudeNonEmptyID(row.sessionId) | ||
| { |
There was a problem hiding this comment.
When a lexicographically earlier copied file contains (m,r) for session B and a later file contains the same pair for sessions A and B, existing.path points to the copy while both rows in the later file are processed. Neither row reaches this same-file preservation branch, and path precedence can discard both in favor of the copied B row, losing session A's usage entirely. Group the distinct sessions within each file before applying the session-free cross-file winner selection.
Useful? React with 👍 / 👎.
|
Addressed the file-local session thread: reconciliation now pre-groups distinct sessions per file, and a session without a cross-file counterpart survives the copy file's path precedence when its own file holds the pair in multiple sessions. Regression test added; 71/71 related tests pass, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 345ab564c4
ℹ️ 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".
| // The same file can hold distinct sessions that reuse the pair; | ||
| // keep both instead of collapsing them through the session-free | ||
| // canonical key. | ||
| rows.append(row) |
There was a problem hiding this comment.
When the lexicographically first file contains the same pair in sessions A and B and a later copied file contains session B, this branch appends the first B row without adding B to keptSessions; processing the copy then enters the cross-file preservation branch and appends B again. Because these appended rows bypass the later identity consolidation, the report counts session B twice. Fresh evidence beyond the previously reviewed copy-first ordering is that reversing the path order exposes this duplicate-counting path.
Useful? React with 👍 / 👎.
| let classKeys = (state.keyedRows[key] != nil && key.hasPrefix("pair:") ? [key] : indexedClassKeys).filter { | ||
| // Paired rows are unique identities: another pair with the same messageId is a | ||
| // different request, while partial rows may bridge through the pair. | ||
| !key.hasPrefix("pair:") || !$0.hasPrefix("pair:") || $0 == key |
There was a problem hiding this comment.
When a transcript evolves through pair(m1,r) -> req(r) -> pair(m2,r) while a copied fork still contains pair(m1,r), the latest cached row retains m1 as an alias, but rebuilding winners from the two files reaches this filter with two paired keys and deliberately excludes the old pair. Both snapshots therefore remain canonical and their cumulative usage is summed. Fresh evidence beyond the previously reviewed same-file transition is that crossing the cache-file boundary removes the partial bridge on which claudeRetireSupersededPairs depended.
Useful? React with 👍 / 👎.
|
Addressed both threads:
Regression tests added for both; 73/73 related tests pass, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Maintainer note: this PR is code-complete and CI-green on head What changed since the original scope: the Claude row identity layer now deduplicates streaming chunks whose The automated review loop has been running for many rounds (each round found new edge cases; all were fixed). The latest re-review has been queued since Aug 1 22:17 UTC without landing - likely the Codex review quota noted at 18:08. A human review would unblock this. Please review when convenient; happy to adjust. |
2 participants
Summary
Claude logs with only a
messageIdorrequestIdwere causing overcounted token costs due to inconsistent deduplication keying between initial scanning and incremental append refreshes. This PR unifies row identity key generation with namespaced single-ID fallback keys (msg:/req:), bumps the cache artifact version, and handles Linux POSIX process execution cleanly.Changes Made
claudeInFileKey. Namespaced single-ID fallback keys (msg:\(messageId)andreq:\(requestId)).artifactVersionfor Claude and Vertex AI from 6 to 7 to invalidate legacy un-namespaced cache records.launchProcessWithRetryto inspect POSIXNSPOSIXErrorDomain+ETXTBSYcode directly.Runtime Behavior Proof: Non-Anthropic Proxy Cost Calculation
Scenario: Incremental Append Refresh with Single-ID Proxy Logs
Before Fix (Overcounting):
After Fix (Namespaced Single-ID Key):
Automated Test & Check Verification:
swift test --filter CostUsageScannerClaudeRegressionTests✅ (13/13 tests pass)make check✅ (0 format/lint violations in 1657 files)Closes #2393