fix anthropic tool call - #3767
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds per-tool-block tracking and fixes streaming conversion: suppresses empty tool input fragments, stops repeating ChangesTool-call streaming spec compliance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
Confidence Score: 5/5Safe to merge — changes are confined to a pure streaming converter with no HTTP calls or side effects, all three bugs have direct test coverage, and the state cleanup on content_block_stop correctly bounds map growth. The converter logic is straightforward: suppress empty deltas, omit the type field on continuations, and flush No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix anthropic tool call" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/anthropic/chat.go (1)
1150-1152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
input_json_deltawhen the tool block was never registered.Lines 1150-1152 intentionally skip
content_block_startfor the structured-output tool, but Line 1229 still readscontentBlockToToolCallIdxwithout checking whether that block was registered. When Anthropic then streamsinput_json_deltafor that skipped block, the zero value sends a continuation chunk to tool-call index0, which reintroduces a corrupted OpenAI stream.💡 Minimal fix
- toolCallIdx := state.contentBlockToToolCallIdx[*chunk.Index] - state.sawArgsDelta[*chunk.Index] = true + toolCallIdx, ok := state.contentBlockToToolCallIdx[*chunk.Index] + if !ok { + return nil, nil, false + } + state.sawArgsDelta[*chunk.Index] = trueAlso applies to: 1228-1230
🤖 Prompt for 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. In `@core/providers/anthropic/chat.go` around lines 1150 - 1152, The code skips emitting a tool call for the structured-output block (when structuredOutputToolName matches) but later unconditionally reads contentBlockToToolCallIdx (and uses it for input_json_delta), causing a zero-index continuation when the block was never registered; update the handling in chat.go to check whether chunk.ContentBlock.Name exists in the contentBlockToToolCallIdx map (use a map lookup/ok pattern) before using the index and, if not present, ignore/skip any input_json_delta or continuation handling for that block (same guard should be added to the other occurrence around contentBlockStart/input_json_delta handling at the referenced nearby lines).
🤖 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.
Outside diff comments:
In `@core/providers/anthropic/chat.go`:
- Around line 1150-1152: The code skips emitting a tool call for the
structured-output block (when structuredOutputToolName matches) but later
unconditionally reads contentBlockToToolCallIdx (and uses it for
input_json_delta), causing a zero-index continuation when the block was never
registered; update the handling in chat.go to check whether
chunk.ContentBlock.Name exists in the contentBlockToToolCallIdx map (use a map
lookup/ok pattern) before using the index and, if not present, ignore/skip any
input_json_delta or continuation handling for that block (same guard should be
added to the other occurrence around contentBlockStart/input_json_delta handling
at the referenced nearby lines).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45470367-1b5b-404a-ac9f-c8bb8abda2b9
📒 Files selected for processing (2)
core/providers/anthropic/chat.gocore/providers/anthropic/chat_test.go
The merge-base changed after approval.
f83b880 to
2995197
Compare
|
Rebased on |
The merge-base changed after approval.
2995197 to
d075f48
Compare
|
@TejasGhatte can you review this please |
6711ce3 to
a1beab5
Compare
e389df7 to
a65fce4
Compare
|
Do I need to update something? Just let me know. This is blocking our bifrost testing/adoption. |
fa15f50 to
ca190fc
Compare
|
Hi @TejasGhatte can you please have an estimate on this review? I'm stuck waiting for this |
Hey @fspaniol checking this one |
|
@akshaydeo it seems it needs another approval too, given that the org has 2 people, I guess it needs to be you 😄 |
4 participants
Summary
Fix Anthropic → OpenAI streaming conversion for tool calls. Two bugs were corrupting tool-call deltas seen by strict OpenAI-compatible clients (e.g. genkit-go): continuation chunks re-declared
function.type, and tools with no input fields (struct{}schema → JSON schema{}) ended up with empty accumulatedarguments, which failsjson.Unmarshalwith "unexpected end of JSON input".Changes
core/providers/anthropic/chat.goType: schemas.Ptr(string(schemas.ChatToolTypeFunction))from continuationinput_json_deltachunks. Only the initialcontent_block_startsetup chunk now declaresfunction.type; strict OpenAI Chat Completions stream parsers treat a repeatedtypeon a continuation as a fresh tool-call declaration.partial_jsonmarker Anthropic emits immediately aftercontent_block_start. The setup chunk already carriesarguments: "", so re-emitting it as a continuation tripped strict parsers.arguments: "{}"flush oncontent_block_stopfor tool blocks where noinput_json_deltawas ever forwarded. Without it, no-arg tools (e.g.response_startwithstruct{}input) accumulate"", which is not valid JSON.sawArgsDelta map[int]booltoAnthropicStreamStateto track, per content-block index, whether any non-empty delta was forwarded. Needed because no existing per-block accumulator on the state struct could be repurposed.content_block_stop, bothcontentBlockToToolCallIdxandsawArgsDeltaentries are deleted to bound state size on long streams with many tool_use blocks. Deletion also acts as the duplicate-stop guard (second stop hits "not a tool block" and returns nil).core/providers/anthropic/chat_test.goTestToBifrostChatCompletionStream_NoArgToolFlushesEmptyObject: start → stop yields a flushed"{}"; accumulated arguments parse as an empty JSON object; duplicate stop does not re-flush.TestToBifrostChatCompletionStream_EmptyPartialJSONSuppressedBeforeArgs: emptypartial_jsonreturns nil; subsequent real fragments stream through; stop does NOT add a synthetic"{}"(would yield{"x":1}{}).TestToBifrostChatCompletionStream_ContinuationOmitsTypeField: regression guard — start chunk carriestype=function, continuation chunk omits it.TestToBifrostChatCompletionStream_MixedToolBlocks: interleaved no-arg and real-args tool blocks across two content-block indices; flush fires only for the no-arg block.Design notes / trade-offs
partial_jsonmarker is a documented quirk of Anthropic's SSE stream and the OpenAI-side strict-parser expectation is what we own.sawArgsDeltais a separate map rather than reusing an accumulator because no per-content-block argument accumulator exists onAnthropicStreamStatetoday; adding one purely for this check would be heavier than amap[int]bool.typefield), matching the rule above.Type of change
Affected areas
How to test
Expected: all four new
TestToBifrostChatCompletionStream_*subtests pass; fullcore/providers/anthropicpackage suite passes.End-to-end validation can be done with this main.go against a strict client (genkit-go) calling a no-arg tool through the Anthropic → OpenAI path: previously failed with
json: unexpected end of JSON inputwhen unmarshalling the accumulatedarguments; now succeeds with{}.No new configs or environment variables.
Screenshots/Recordings
N/A — backend-only change.
Breaking changes
Related issues
Closes #3443
Security considerations
None. No changes to auth, secrets handling, PII, or sandboxing; the converter operates on already-authenticated streamed responses and the new code paths only adjust delta framing.
Checklist
docs/contributing/README.mdand followed the guidelinesgo build ./core/providers/anthropic/succeeds; UI unaffectedgo test ./providers/anthropic/passesSummary by CodeRabbit
Bug Fixes
Tests