Feat: Record per-tool-call cost inputs on the Anthropic path - #814
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe inference pipeline now records raw content sizes and separate cache token counts. It reconstructs streamed tool calls, folds incremental usage updates, finalizes interrupted streams, and preserves cache metrics in the TUI. ChangesInference parser metrics and streaming
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change adds per-tool-call cost accounting data for Anthropic requests without introducing a new provider call or capture surface. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant AnthropicSSE
participant AnthropicParser
participant InferenceExtension
AnthropicSSE->>AnthropicParser: send message, usage, and tool-call events
AnthropicParser->>AnthropicParser: assemble indexed argument fragments and update totals
AnthropicParser->>InferenceExtension: write completion, cache usage, and tool calls
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
4c1435f to
e0e7e74
Compare
huang195
added
the
ready-for-ai-review
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Adds CacheWriteTokens/CacheReadTokens, ContentBytes, and streamed tool-call capture to the Anthropic inference-parser path — all derived from data already on the wire, backed by real captured frames, and covered by focused tests for each failure mode.
All checks pass. Ready for human review.
Reviewed by clawgenti using the github-pr-review skill
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Adds three missing cost-accounting fields to the Anthropic inference parser (streamed tool calls, ContentBytes for non-text messages, and CacheWriteTokens/CacheReadTokens), all well-motivated by measured production data and backed by targeted regression tests. All checks pass. Ready for human review.
Reviewed by clawgenti using the github-pr-review skill
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Three well-scoped accounting additions, each traced to a measured gap, with tests built from real captured frames. I verified promptTotal() sums input + cache_creation + cache_read, so CacheWriteTokens + CacheReadTokens <= PromptTokens holds and the split is internally consistent with the total it decomposes. The pointer-held strings.Builder and the index-keyed fragment routing are both correct — the interleaved test is exactly the right thing to pin, since arrival-order accumulation is the failure mode that would silently concatenate one call's arguments into another.
No must-fix findings. Three inline notes, all polish; I fetched the branch and probed each one rather than eyeballing it.
One process note, not a code issue: #811 is now merged, so the PR body's "opened as a draft; once #811 merges I'll rebase onto main" is stale (it is no longer a draft either). The branch is 3 ahead / 1 behind main, and GitHub still renders the merge-base diff — 651+/44- displayed vs 507+/34- actual. Rebasing drops endpointPath and the message_delta hunk from the view, which is ~145 lines of already-merged code reviewers currently have to mentally subtract. Worth doing before merge so the diff matches the change.
Areas reviewed: Go (parser, pipeline types, TUI), tests
Agent/IDE config (.claude/.vscode): none
Commits: 5 listed / 3 net vs main, all signed off, no Co-authored-by
CI: all 20 checks passing (Spellcheck skipped)
Assisted-By: Claude Code
| state.finalize(ext) | ||
| // Empty stream with no body and no chunks — record Skip to | ||
| // pair the response row with the request row. | ||
| if ext.Completion == "" && ext.FinishReason == "" && ext.TotalTokens == 0 { |
There was a problem hiding this comment.
suggestion — this guard predates streamed tool calls, and finalize can now populate ext.ToolCalls. So a stream whose only captured content is a tool call is recorded as a skip and the call is discarded.
Probed on this branch:
ToolCalls=1 Completion="" FinishReason="" TotalTokens=0
invocation: action=skip reason=no_response_body
Frames were just content_block_start (tool_use) + one input_json_delta, then EOF — a turn cancelled while the model was still emitting tool arguments.
Not live on a real Anthropic stream: message_start always precedes and its input_tokens pushes TotalTokens non-zero. But TestInferenceParser_AnthropicMessages_StreamToolUseInterleaved has no message_start and escapes this guard only via the trailing "output_tokens":60. Remove that one frame and the two tool calls the test just proved are captured vanish into a skip — which is the same class of loss point 1 of this PR set out to close.
if ext.Completion == "" && ext.FinishReason == "" && ext.TotalTokens == 0 && len(ext.ToolCalls) == 0 {| CompletionTokens int `json:"completion_tokens"` | ||
| TotalTokens int `json:"total_tokens"` | ||
|
|
||
| CacheWriteTokens int `json:"-"` |
There was a problem hiding this comment.
nit — the json:"-" rationale is right, but foldOpenAIFrame assigns the whole struct:
if chunk.Usage.TotalTokens > 0 {
state.usage = chunk.Usage
}chunk.Usage is JSON-decoded, so these two are always zero in it — the assignment clears whatever the accumulator held. Probed: seeded CacheWrite=111 CacheRead=222, folded one OpenAI usage frame, got 0/0.
No bug today, since nothing on the OpenAI path sets them. It becomes one the moment someone wires up prompt_tokens_details.cached_tokens — the exact follow-up this comment anticipates — because the failure is silent. Assigning the three JSON-backed fields individually there, or a one-line warning here, closes it.
| // than the 4 bytes the literal `null` occupies — the field is a size signal | ||
| // for content that exists, and an assistant turn that carries only tool_calls | ||
| // has none. | ||
| func contentBytes(raw json.RawMessage) int { |
There was a problem hiding this comment.
nit — raw is a json.RawMessage, so len(raw) preserves the client's formatting verbatim. Identical content therefore measures differently depending on how the client serialises:
compact ContentBytes=75
pretty ContentBytes=89
(same single tool_result block, ~19% apart)
The doc comment covers "syntax and escapes included" and "a size signal rather than an exact one," which is the right framing — whitespace just isn't named, and it is the one source of variance that has nothing to do with payload size. SDK clients send compact JSON so real-world impact is small, but it does mean the counts aren't comparable across clients.
Either name it in the comment, or json.Compact into a scratch buffer before measuring if cross-client comparability matters.
The inference-parser records enough to see that a turn happened, but not enough to say what it cost. Three gaps, all on data already on the wire: Streamed tool calls were dropped. A tool call arrives across three event types — id and name on content_block_start, arguments as input_json_delta fragments that are only valid JSON once concatenated — and the stream folder modeled none of them. A streaming turn recorded finishReason "tool_use" with an empty toolCalls list while the equivalent non-streaming response recorded the call in full. Fragments are routed by content block index, so interleaved calls don't merge into each other. Non-text request messages read as free. Content keeps only text blocks, so a tool_result — a whole file, in an agent loop — flattens to "" while the model was billed for every byte. InferenceMessage.ContentBytes is the wire size of the content value before that reduction. It is a byte count, not a token count: a size signal, not an exact one. Prompt-cache writes and reads were collapsed into PromptTokens. A provider that prices caching charges a premium to write an entry and a steep discount to read one — 12.5x apart for Anthropic — so two turns with identical PromptTokens can differ by an order of magnitude in cost. CacheWriteTokens and CacheReadTokens split it, from the same usage block the totals already come from. Scope: OpenAI *streaming* tool calls remain uncaptured — a separate pre-existing gap. Recording streamed tool-call Arguments is parity with the non-streaming path, which already records them. Depends on the message_delta usage fix (rossoctl#811) — same hunk. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
…tput
finalize gates the whole usage copy on TotalTokens > 0, but the Anthropic
fold only computed that total inside the output_tokens > 0 arm. Any stream
that reported a prompt and no completion therefore refreshed PromptTokens
and then threw it away.
Two streams do that, and the provider billed the prompt in both:
- a terminal message_delta carrying the prompt with output_tokens == 0
(a refusal, or a generation stopped immediately);
- a turn the caller interrupted after message_start, which never reaches
a message_delta at all — the shape an agent produces every time a user
cancels a running turn.
Derive the total from the parts after any usage update instead. The
interrupted case now records a real response row with prompt tokens and
zero completion rather than skip/no_response_body; recovery there is
partial by construction, since the ?beta=true path defers the cache
counts to message_delta.
The OpenAI fold is untouched: it takes total_tokens off the wire, where it
can legitimately differ from prompt + completion.
Raised in review of rossoctl#811 by CodeRabbit and @mrsabath.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Adding tool-call capture to the Anthropic streaming path made ToolCalls populated on three of the four response paths instead of two. Before that it was uniformly empty on both streaming dialects, so a consumer could read "empty" the same way everywhere; now an empty ToolCalls means different things depending on which dialect streamed the response. The asymmetry is structural, not an oversight: the OpenAI streaming chunk shape decodes only `choices[].delta.content`, so assembling `choices[].delta.tool_calls[]` would need a second index-keyed accumulator — its own change. Until then the only record of the gap is a PR description, which stops being reachable the moment it merges. Document it on the field, in the shape the neighbouring CacheWriteTokens and ContentBytes comments use: say what populates it, and say what its zero value does and does not license a consumer to conclude. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Three findings from review of rossoctl#814, all latent rather than live today. 1. A stream carrying only tool calls was recorded as a skip. The "no response body" guard tested completion text, finish reason and usage but not ToolCalls, so a turn cancelled while the model was still emitting tool arguments — none of those three present, but a captured call — was labelled as having carried nothing and dropped out of any timeline filtered on observe. Latent because a real Anthropic stream opens with message_start, which sets usage; reachable on a truncated one. 2. An OpenAI usage frame cleared the cache accumulator. foldOpenAIFrame assigned the decoded chunk's usage over state.usage wholesale. inferenceUsage doubles as the OpenAI wire shape and the dialect-neutral accumulator, and its two cache fields are json:"-", so they are always zero in a freshly decoded chunk — the assignment cleared whatever had accumulated. Now copied field by field. Latent because nothing on the OpenAI path fills those fields today, which is precisely what would make the clobber silent when something does. 3. ContentBytes counts whatever whitespace the client's serializer emitted. Documented rather than changed: the measure is of what was sent, so compacting first would buy comparability across clients at the cost of an allocation per message on every request-body parse. Comparable across messages from one client, not across clients. Both source fixes carry a regression test, each verified to fail with its fix reverted. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
d4bb581 to
78bc732
Compare
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
4 participants
What
The inference-parser records enough to see that an inference turn happened, but not enough to say what it cost. This adds three fields' worth of accounting, all derived from data already on the wire — no new capture surface, no new request to any provider.
1. Streamed tool calls were dropped entirely. An Anthropic tool call arrives across three event types: id and name on
content_block_start, arguments asinput_json_deltafragments that are only valid JSON once concatenated. The stream folder modeled none of them. A streaming turn recordedfinishReason: "tool_use"with an emptytoolCallslist, while the equivalent non-streaming response recorded the call in full. Fragments are routed by content-block index, so interleaved calls (a text block plus two tool calls) don't concatenate into each other.2. Non-text request messages read as free.
InferenceMessage.Contentkeeps only text blocks, so atool_resultmessage flattens to""— and in an agent loop that message is a whole file. Measured: a post-tool-call turn recorded system 27,772 chars, user 13,714, assistant 0, tool_result user 0, while the model billed 34,018 prompt tokens.ContentBytesis the wire size of the content value before that reduction. It is a byte count of raw JSON (syntax and escapes included), not a token count — a size signal, not an exact one.3. Prompt-cache writes and reads were collapsed into
PromptTokens. A provider that prices prompt caching charges a premium to write an entry (1.25× base for Anthropic) and a steep discount to read one (0.1×) — a 12.5× spread. Measured: a 5,213-token cache write and a 5,213-token cache read both recordedpromptTokens = 24,038, while costing $0.4495 and $0.0405 respectively — 11.1× apart, identical recorded number.CacheWriteTokens/CacheReadTokenssplit it, read from the same usage block the totals already come from.Why
PromptTokensanswers "how big was the context." It cannot answer "what did this cost," and cost is what an operator watching an agent loop actually needs. These three fields are the minimum that makes per-tool-call cost accounting possible from session events alone.Scope boundaries
InferenceExtension.ToolCallsnow carries a doc comment saying so, because a cost consumer that spans dialects must not read an empty list on a streamed OpenAI turn as "the model requested no tools."Argumentsis parity, not new exposure.parseAnthropicJSONalready recordsArguments: string(blk.Input)on the non-streaming path; this makes the streaming path match.PromptTokensstays authoritative.content: nullreports 0 bytes rather than the 4 bytes of the literal.Rebase status
#811 has merged; this branch is rebased onto
mainand carries only its own three commits plus the review follow-up below. Diff againstmainis 6 files, 507+/34-.Review follow-ups
Three findings from review, all latent rather than live today. Each source fix has a regression test that was verified to fail with the fix reverted.
A stream carrying only tool calls was recorded as a skip. The "no response body" guard tested completion text, finish reason and usage but not
ToolCalls, so a turn cancelled while the model was still emitting tool arguments — none of those three present, but a captured call — was labelled as having carried nothing and dropped out of any timeline filtered onobserve. Latent because a real Anthropic stream opens withmessage_start, which sets usage; reachable on a truncated one.An OpenAI usage frame cleared the cache accumulator.
foldOpenAIFrameassigned the decoded chunk's usage overstate.usagewholesale.inferenceUsagedoubles as the OpenAI wire shape and the dialect-neutral accumulator, and its two cache fields arejson:"-", so they are always zero in a freshly decoded chunk — the assignment cleared whatever had accumulated. Now copied field by field. Latent because nothing on the OpenAI path fills those fields today, which is precisely what would make the clobber silent when something does.ContentBytescounts whatever whitespace the client's serializer emitted. Documented rather than changed: the measure is of what was sent, so compacting first would buy comparability across clients at the cost of an allocation per message on every request-body parse. Comparable across messages from one client, not across clients.Testing
go test ./authlib/... ./cmd/abctl/...passes. Cases ininferenceparser:input_json_deltafragments reassemble into complete argumentsContentByteson both dialects (separate unmarshalers), including thetool_resultmessage that flattens to""but is the largest thing in the request?beta=truestreaming path (real captured frames: write 3,755 / read 30,008)observe, notskip, and keeps the truncated arguments the model left behindThe existing plain-path streaming test still asserts 25/15/40, which guards the max-not-assign behavior #811 introduced.
golangci-lint runon the changed packages is clean (three pre-existingQF1008findings inpipeline/finisher_test.go, untouched by this PR).Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit