fix(claude): stop counting base64 attachments as raw characters in token estimates - #983
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughClaude request token estimation now treats base64 image and document attachments as bounded data. It uses image dimensions or decoded byte size. Ordinary text remains character-estimated. Routed usage logging and ChangesClaude token estimation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CountTokens as /count_tokens
participant UsageLogging as Routed usage logging
participant Estimator as estimateClaudeRequestTokens
participant ImageSniffer as sniffImageDimensions
participant TextEstimator as estimateTokens
CountTokens->>Estimator: request content and modelId
UsageLogging->>Estimator: system, messages, tools, and modelId
Estimator->>ImageSniffer: inspect base64 image data
ImageSniffer-->>Estimator: image dimensions
Estimator->>TextEstimator: estimate text and adjusted content
TextEstimator-->>Estimator: token estimate
Estimator-->>CountTokens: estimated token count
Estimator-->>UsageLogging: estimated token count
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/claude-messages.ts`:
- Line 869: Update the decoded-size calculation in the attachment token
estimator to remove trailing base64 padding characters before converting encoded
length to bytes, so a padded 131,072-byte payload estimates 256 tokens. Add a
focused regression test alongside the existing attachment-related tests covering
that exact padded payload and expected token count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c7b18fa-fd1d-4eac-a461-619c523d1e12
📒 Files selected for processing (2)
src/server/claude-messages.tstests/claude-messages-endpoint.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6dc37aee0
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/claude-messages.ts`:
- Around line 892-902: Restrict the attachment-processing logic around the
visible image/document branch to Claude protocol content-block containers,
including nested tool_result.content, and do not recurse into arbitrary
tool_use.input values. Preserve attachment token estimation and data redaction
only for genuine content blocks. Add a focused regression test alongside the
existing claude-messages tests using a complete attachment-shaped object in
tool_use.input and assert it uses the normal serialized-text estimate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d5dd6c5c-bb5c-4215-976e-73d0bf1bcd15
📒 Files selected for processing (2)
src/server/claude-messages.tstests/claude-messages-endpoint.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b78fddf6b
ℹ️ 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".
[GD] Verdict: changes-requestedTLDR
Full verdictSemantic propagation
Linked: none UsefulnessFixes a real correctness bug in token estimation: a 2MB pasted screenshot (~2.7M base64 chars) was previously counted as ~680k input tokens by the plain char/4 estimator, whereas the actual Anthropic cost is ~pixels/750 ≈ 1.6k tokens. The PR prices base64 attachments per-attachment at both the Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification (for the PR owner)Bounded, behavior-preserving candidates only; nothing was edited or pushed (foreign PR):
These are optional; candidates 2 and 3 are stylistic and can be skipped without loss. Gatenone Bottom lineThe PR is correct, useful, and well-tested. It fixes a real estimation bug, keeps text-only parity, and the scoping fix ( |
|
✅ Deterministic PR hygiene checks passed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Summary
Verification
bun run typecheck && bun run lint:gui && bun run test && bun run privacy:scanChecklist
Summary by CodeRabbit
Improvements
Bug Fixes