Conversation
There was a problem hiding this comment.
Code Review
This pull request implements token estimation and improved usage reporting across various executors, including Gemini, Claude, and OpenAI-compatible providers. It introduces mechanisms to estimate input tokens from request payloads and output tokens from response text when upstream usage metadata is missing, ensuring that usage statistics are consistently recorded. Additionally, the TUI has been updated to display a latency breakdown (average, minimum, and maximum) for requests, supported by new internationalization strings and rendering logic. Comprehensive tests have also been added to verify the accuracy of token estimation and usage logging. I have no feedback to provide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7db0483aee
ℹ️ 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".
| detail := r.estimatedDetail() | ||
| r.once.Do(func() { |
There was a problem hiding this comment.
Compute estimated usage only inside once.Do
ensurePublished always calls r.estimatedDetail() before once.Do, so even when a real usage record has already been published (the common path in these executors), we still re-tokenize the buffered output text. On large streamed responses this adds avoidable O(response_size) CPU work and can noticeably increase request latency under load. This estimation should be performed only in the fallback path that actually publishes via once.Do.
Useful? React with 👍 / 👎.
7db0483 to
8b9dbe1
Compare
luispater
left a comment
There was a problem hiding this comment.
Summary:
This looks correct. Removing the zero-usage early return matches the intended behavior, and the new regression test exercises a successful upstream response with zero tokens.
Key findings:
- No blocking findings.
Test plan:
- Reviewed the diff and the new regression coverage in
test/usage_logging_test.go.
This is an automated Codex review result and still requires manual verification by a human reviewer.
luispater
left a comment
There was a problem hiding this comment.
Summary:
This removes the early return that suppressed successful zero-token usage records and adds an end-to-end usage-statistics regression test.
Key findings:
- No blocking issues found in the diff I reviewed.
Test plan:
- Reviewed
internal/runtime/executor/helps/usage_helpers.goand the new usage logging test.
This is an automated Codex review result and still requires manual verification by a human reviewer.
This PR carries the record-zero-usage fix from the forked repository.