fix: robust token usage normalization for OpenAI-compatible providers#45535
fix: robust token usage normalization for OpenAI-compatible providers#45535yiShanXin wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes token usage being reported as 0 for Alibaba Bailian and other OpenAI-compatible providers by replacing the Key changes:
Correctness assessment: The two-pass logic is sound across all edge cases: providers returning a single non-zero value behave identically to before; the new behavior only differs when at least one candidate field is 0 while another is positive. The existing negative-input clamp ( Confidence Score: 4/5
Last reviewed commit: 6596552 |
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Good fix — the two-pass pick approach (prefer non-zero, fall back to zero) is a real improvement over the previous ?? chain which could silently skip a valid zero value.
🔴 Semantic change in toUsage (helpers.ts): zero-value behaviour
The new pick in helpers.ts now returns 0 for a key that exists with value 0, whereas the old code explicitly required raw[key] > 0. This is intentional per the PR description, but it changes how toUsage handles the "no data" guard:
if (!input && !output && !cacheRead && !cacheWrite && !total) {
return undefined;
}This guard uses falsy check, so a response where every field legitimately returns 0 will still correctly return undefined. That's fine — but note that a response with only cacheRead: 0 will now return a CliUsage object with everything undefined except cacheRead: 0, where before it returned undefined. Make sure downstream consumers handle partial usage objects with all-zero fields gracefully.
🟡 Inconsistency between the two pick implementations
toUsage in helpers.ts takes rest args of string[] (keys to look up in raw), while normalizeUsage in usage.ts takes rest args of unknown (pre-resolved values). They're not interchangeable. This is fine as-is, but it's a footgun if someone tries to reuse one in the other's context. Worth extracting to a shared utility with clear naming, e.g. pickFirstPositive(...values: unknown[]).
🟡 pick in usage.ts does two full passes over args
The double-pass pattern (first pass: positive values only, second pass: zero) is correct but iterates args twice. For small arg lists this is negligible, but the pattern is repeated in both files. A single-pass with a "best candidate" approach would be cleaner:
const pick = (...args: unknown[]) => {
let fallback: number | undefined;
for (const arg of args) {
const val = asFiniteNumber(arg);
if (val !== undefined) {
if (val > 0) return val;
if (fallback === undefined) fallback = val;
}
}
return fallback;
};💡 No tests added
The PR touches a normalization function that's exercised across many provider integrations. A few unit tests covering the new aliases (prompt_tokens, completion_tokens, cached_tokens) and the zero-value edge cases would give confidence this doesn't regress for existing providers.
💡 cached_tokens in cacheRead path
cached_tokens has been moved from helpers.ts (where it was absent) and added to both files' cacheRead pick list. This is consistent with OpenAI's prompt_tokens_details.cached_tokens usage but double-check that providers using top-level cached_tokens as total cache aren't being inadvertently double-counted against cacheRead when they also populate prompt_tokens_details.cached_tokens.
Overall the intent is correct and the normalization is more robust. The two-pass pick logic and the inconsistent implementations between files are the main things worth tidying before merge.
![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: 6596552803
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| input === undefined && | ||
| output === undefined && | ||
| cacheRead === undefined && | ||
| cacheWrite === undefined && | ||
| total === undefined |
There was a problem hiding this comment.
Returning a usage object when all parsed token fields are 0 causes parseCliJsonl to overwrite previously captured nonzero usage, because each line does usage = toUsage(parsed.usage) ?? usage. In JSONL streams where a later bookkeeping event includes zeroed usage (common for end-of-stream/status events), this change drops real token totals and downstream session/cost reporting treats the run as having no usage.
Useful? React with 👍 / 👎.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep this PR open, but it is not merge-ready: the requested usage-normalization fix is still useful, current main has not fully implemented the mixed-zero alias behavior, and the branch needs current-main rework plus real behavior proof before maintainers can decide whether to land or supersede it. Canonical path: Close this PR as superseded by #78085. So I’m closing this here and keeping the remaining discussion on #78085. Review detailsBest possible solution: Close this PR as superseded by #78085. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main still lets Is this the best way to solve the issue? No. The direction is useful, but this stale branch must be reworked against current Security review: Security review cleared: The diff only changes TypeScript usage-normalization helpers and does not touch dependencies, workflows, secrets, package metadata, downloads, permissions, or other code-execution surfaces. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 37a9f58d1b10. |
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
impact:session-state
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
ClawSweeper applied the proposed close for this PR.
|
2 participants
Fixes token usage being reported as 0 for Alibaba Bailian and other OpenAI-compatible providers that may return both
input_tokens: 0andprompt_tokens: N.Also adds missing
prompt_tokensandcompletion_tokenssupport to the CLI runner usage helper.Fixes #45038
Fixes #45061