ocr review commit 08bb28c · 3 comments
ocr review --commit 08bb28c --format text --audience human --concurrency 2
surfaced three findings across 5 reviewed Go files in
1m21s. One CRITICAL token-accounting inconsistency, one defensive
nil-check on a new prompt-caching code path, and one minor comment-style note.
Every anchor below is grep-able in diff.patch.
CRITICAL: Token accounting logic is inconsistent with the
usage resolver. In usage_resolver.go (line 78), TotalTokens
is computed as prompt + completion + cacheRead + cacheWrite,
indicating cache tokens are additional to the base prompt/completion
tokens. However, the comment here claims "PromptTokens already includes
cache read/write tokens" for Anthropic. This is contradictory. The
implementation should either (1) subtract cache tokens if they're included in
PromptTokens, or (2) update the comment to match the actual
behavior of adding cache tokens separately. As written, the returned total
will not match resp.Usage.TotalTokens from the API response.
Potential nil pointer dereference: OfTool field
can be nil according to the test checks (see
client_test.go lines 134-135), but this code dereferences it
without validation. While the current construction guarantees it won't be
nil, add a defensive nil check to prevent potential panics if
the code is modified in the future or if the SDK allows nil values.
The struct field comment for totalInputTokens should be more
precise about atomic access. Since totalCacheReadTokens and
totalCacheWriteTokens are also accessed atomically but lack
the clarifying comment, add the same comment to all atomic fields for
consistency.
Net: cosmetic — no behavior change suggested.
How to consume human · CI
Paste the rendered Markdown above into the PR description, or as inline review
comments using each anchor. For CI gating, the structured form lives in
review.json — see the Raw JSON tab on the right
for the exact schema OCR ships. A minimal merge-blocker, matching this run: