Repository navigation
fix(usage): count dictionary usage in token callbacks - #8171
Conversation
Strix Security ReviewWarning This pull request has 55 commits after the last Strix review ( No security issues found. Review summaryReviewed the PR's two changed files. The production change in Updated for Reviewed by Strix |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
VANDRANKI
left a comment
There was a problem hiding this comment.
I ran this on Windows with Python 3.13.3 (crewai 1.15.28 from the repo, openai SDK installed, litellm not installed), against main (40bb2d3) and the PR head.
I called TokenCalcHandler.log_success_event({}, {"usage": usage}, 0, 0) with a fresh TokenProcess and printed the counters.
| usage passed | main | PR |
|---|---|---|
| OpenAI style dict (12 prompt, 3 completion, 4 cached) | requests 1, tokens 0 | prompt 12, completion 3, total 15, cached 4 |
the same data as an openai CompletionUsage object |
12, 3, 15, 4 | 12, 3, 15, 4 |
Anthropic style dict (input_tokens 10, output_tokens 5, cache_read_input_tokens 20, cache_creation_input_tokens 2) |
tokens 0 | prompt 32, completion 5, total 37, cached 20 |
Gemini style dict (prompt_token_count 7, candidates_token_count 2) |
tokens 0 | prompt 7, completion 2, total 9 |
prompt_tokens_details set to None |
tokens 0 | prompt 5, completion 1, cached 0, no error |
dict with None token values |
tokens 0 | tokens 0, no error |
So the bug is real. On main a dict counts the request but no tokens. The object path is unchanged on the PR.
The new tests/utilities/test_token_counter_callback.py has 7 tests. All 7 pass on the PR. With the file copied onto main, the two dict cases (cached_tokens 0 and 4) fail with assert 0 == 24, and 5 pass.
One thing to know when reading the Anthropic row. from_provider_dict counts cache read and cache creation tokens inside the prompt total (10 + 20 + 2 = 32). So the callback now gives billed prompt tokens for those dicts. The PR says it reuses that helper on purpose, and the number agrees with it. I did not compare it to the BaseLLM tracker at runtime.
I did not run a real LLM call. I also did not check whether the streaming path in llm.py can call the callback and also update the same TokenProcess somewhere else, which would double count. That is read-only and unchecked.
The fix is small and verified for the cases above.
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Reviewed the dictionary-usage normalization path and its regression coverage. handles nested and provider aliases; accumulation correctly relies on to derive totals. SDK-object behavior remains intact. Approved.
f20d4a0 to
e5d2a34
Compare
Related issue
Fixes #8170.
Summary
TokenCalcHandler increments successful_requests for dictionary usage but only reads token counters as attributes, leaving prompt, completion, total and cached-prompt counts at zero. This affects the callback accumulator; BaseLLM's separate internal tracker already supports dictionaries.
Reuse UsageMetrics.from_provider_dict for dictionary payloads and accumulate into the existing TokenProcess counters. Keep SDK-object handling, missing usage and absent-process behavior unchanged.
Tests compare dictionary usage with real OpenAI CompletionUsage objects, with and without cached tokens, and verify that repeated callbacks accumulate exactly once per request. They run without provider calls.
Verification
Python 3.13.12 / macOS arm64, repository-lockfile dependencies:
Tests used CREWAI_TELEMETRY_DISABLED=true and OTEL_SDK_DISABLED=true. Full monorepo tests and live LiteLLM integrations were not run. No paid model or provider/registry calls.
Additional context
AI assistance: Codex investigated the code, prepared the implementation and regression tests, and ran the checks above.
Note
Low Risk
Narrow fix to token accounting for dict usage in callbacks; SDK-object and edge-case paths are preserved and covered by new tests.
Overview
Fixes a bug where
TokenCalcHandlerincremented successful request counts for dict-shaped LLMusagepayloads but never updated prompt, completion, or cached token totals because it only read attribute-style SDK objects.When
usageis a dict, the handler now parses it viaUsageMetrics.from_provider_dict(same path asBaseLLM) and feeds the resulting metrics intoTokenProcess, then returns so object-style handling is unchanged. Missing/empty usage and a nil token process behave as before.Adds offline tests that dict and
CompletionUsageusage accumulate identically (including cached tokens), skip empty usage, and no-op without aTokenProcess.Reviewed by Cursor Bugbot for commit e5d2a34. Bugbot is set up for automated code reviews on this repo. Configure here.