fix: stop reason with structured output tools - #3685
Merged
akshaydeo merged 1 commit intoMay 25, 2026
Conversation
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds stream-state flags and detection for structured-output tool consumption; folds structured-output into content (skipping toolCalls), conditions finish-reason remapping (tool_calls→stop) on actual consumption and absence of real tool calls, and avoids forcing structured-output tool choice when "thinking" is enabled. ChangesStructured Output Tool Consumption Detection
Sequence Diagram(s)sequenceDiagram
participant BifrostReq
participant Anthropic_BedrockReq
participant ModelStream
participant StreamState
participant BifrostResp
BifrostReq->>Anthropic_BedrockReq: ToAnthropic/ToBedrockRequest (set ToolChoice if thinkingDisabled)
Anthropic_BedrockReq->>ModelStream: model streams message_delta / tool_use events
ModelStream->>StreamState: mark SeenRealToolCall / UsedStructuredOutputTool / consumedStructuredOutput
ModelStream->>BifrostResp: ToBifrostChatResponse (fold structured-output into content, skip toolCalls)
StreamState->>BifrostResp: remap finish_reason (tool_calls -> stop if only structured-output consumed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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" Comment |
TejasGhatte
marked this pull request as ready for review
May 22, 2026 05:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/chat.go (1)
97-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
tool_callswhen real tool calls exist, and only mark SO as consumed when payload is folded.The current override remaps to
stopwhenever a structured-output tool appears, even if regular tool calls are also present. That can mask legitimatetool_calls. Also, the consumption flag is set even when noToolUse.Inputis folded intocontentStr.Suggested fix
if contentBlock.ToolUse != nil { // Check if this is the structured output tool if structuredOutputToolName, ok := ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName).(string); ok && contentBlock.ToolUse.Name == structuredOutputToolName { // This is structured output - set contentStr and skip adding to toolCalls if contentBlock.ToolUse.Input != nil { jsonStr := string(contentBlock.ToolUse.Input) contentStr = &jsonStr + usedStructuredOutputTool = true } - usedStructuredOutputTool = true continue // Skip adding to toolCalls } @@ FinishReason: func() *string { mapped := convertBedrockStopReason(response.StopReason) - if usedStructuredOutputTool && mapped == string(schemas.BifrostFinishReasonToolCalls) { + if usedStructuredOutputTool && + len(toolCalls) == 0 && + mapped == string(schemas.BifrostFinishReasonToolCalls) { mapped = string(schemas.BifrostFinishReasonStop) } return &mapped }(),Also applies to: 238-244
🤖 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/bedrock/chat.go` around lines 97 - 104, The current logic treats any appearance of a structured-output tool as consuming the structured output (setting usedStructuredOutputTool) and forces the stop override even if other real tool calls exist; change it so you only set usedStructuredOutputTool and fold contentBlock.ToolUse.Input into contentStr when the Input is actually present and used (i.e., only when contentBlock.ToolUse.Input != nil and you assign jsonStr to contentStr), and only apply the remap-to-"stop" override when usedStructuredOutputTool is true AND there are no other toolCalls present (preserve tool_calls when real tool calls exist). Apply the same adjustment to the mirrored block that uses the same pattern (the secondary occurrence that sets usedStructuredOutputTool and contentStr).core/providers/anthropic/responses.go (1)
1323-1342:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the overridden finish reason for
message_delta.This branch converts the raw Anthropic stop reason again, so SO-consumed responses can still emit
tool_callsto Anthropic compatibility clients even thoughstate.StopReasonwas already corrected above.Suggested fix
- var stopReason *string - if chunk.Delta != nil && chunk.Delta.StopReason != nil { - converted := ConvertAnthropicFinishReasonToBifrost(*chunk.Delta.StopReason) - stopReason = &converted - } + stopReason := state.StopReason🤖 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/responses.go` around lines 1323 - 1342, When constructing the schemas.BifrostResponsesResponse in the message_delta branch, reuse the already-normalized state.StopReason instead of reconverting chunk.Delta.StopReason; check if state.StopReason != nil and assign that pointer to response.StopReason, and only fall back to converting chunk.Delta.StopReason via ConvertAnthropicFinishReasonToBifrost if state.StopReason is nil, so SO-consumed responses preserve the overridden finish reason.
🤖 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 `@core/providers/anthropic/anthropic.go`:
- Around line 829-834: The finish_reason remap currently changes tool_calls→stop
whenever consumedStructuredOutput is true (using finishReason,
consumedStructuredOutput and schemas.BifrostFinishReasonToolCalls) even if real
(non-structured-output) tool calls were also emitted; update the logic to only
remap when there were no non-SO tool calls emitted by either (A) checking the
AnthropicStreamState for presence of non-SO tool calls (extend
AnthropicStreamState: add a bool like sawNonSOToolCall and set it when emitting
any tool_use block that is not the SO-only block, referencing
nextToolCallIndex/contentBlockToToolCallIdx semantics), or (B) if you prefer
minimal change, compute “no non-SO tool calls” by inspecting
nextToolCallIndex/contentBlockToToolCallIdx and any marker used for SO-only
blocks and only then perform the remap of finishReason from
schemas.BifrostFinishReasonToolCalls to schemas.BifrostFinishReasonStop.
In `@core/providers/anthropic/chat.go`:
- Around line 925-929: The remap that forces mapped from
schemas.BifrostFinishReasonToolCalls to schemas.BifrostFinishReasonStop should
only run when no real (non-structured) tools were invoked; update the
conditional around the block that checks usedStructuredOutputTool and mapped
(the code that currently reads usedStructuredOutputTool && mapped ==
string(schemas.BifrostFinishReasonToolCalls)) to also require that toolCalls is
empty (e.g., len(toolCalls) == 0), so the override only happens when there are
no other toolCalls to preserve.
In `@core/providers/anthropic/responses.go`:
- Around line 1312-1316: The current remapping of
ConvertAnthropicFinishReasonToBifrost(*chunk.Delta.StopReason) to stop only
checks state.UsedStructuredOutputTool and will incorrectly downgrade mixed
streams that also emitted real tool calls; update the logic to remap to
schemas.BifrostFinishReasonStop only when the structured-output tool was used
AND no other real tool call was seen (e.g., add or use a boolean like
state.SeenRealToolCall that is set when real tool events such as
tool_use/server_tool_use/mcp_tool_use are emitted), then change the condition to
if state.UsedStructuredOutputTool && !state.SeenRealToolCall { mapped =
string(schemas.BifrostFinishReasonStop) } and ensure state.SeenRealToolCall is
toggled in the code paths that handle real tool emissions.
- Around line 2763-2774: The current non-streaming remapping block checks
response.Content for AnthropicContentBlockTypeToolUse only and will incorrectly
remap to schemas.BifrostFinishReasonStop even when there are server or mcp tool
uses; update the check that sets hasRealToolUse (the loop over response.Content)
to treat AnthropicContentBlockTypeServerToolUse and
AnthropicContentBlockTypeMCPToolUse as legitimate tool calls as well (i.e., if
block.Type is ToolUse OR ServerToolUse OR MCPToolUse and block.Name !=
soToolName then set hasRealToolUse = true and break) before assigning mapped =
string(schemas.BifrostFinishReasonStop).
---
Outside diff comments:
In `@core/providers/anthropic/responses.go`:
- Around line 1323-1342: When constructing the schemas.BifrostResponsesResponse
in the message_delta branch, reuse the already-normalized state.StopReason
instead of reconverting chunk.Delta.StopReason; check if state.StopReason != nil
and assign that pointer to response.StopReason, and only fall back to converting
chunk.Delta.StopReason via ConvertAnthropicFinishReasonToBifrost if
state.StopReason is nil, so SO-consumed responses preserve the overridden finish
reason.
In `@core/providers/bedrock/chat.go`:
- Around line 97-104: The current logic treats any appearance of a
structured-output tool as consuming the structured output (setting
usedStructuredOutputTool) and forces the stop override even if other real tool
calls exist; change it so you only set usedStructuredOutputTool and fold
contentBlock.ToolUse.Input into contentStr when the Input is actually present
and used (i.e., only when contentBlock.ToolUse.Input != nil and you assign
jsonStr to contentStr), and only apply the remap-to-"stop" override when
usedStructuredOutputTool is true AND there are no other toolCalls present
(preserve tool_calls when real tool calls exist). Apply the same adjustment to
the mirrored block that uses the same pattern (the secondary occurrence that
sets usedStructuredOutputTool and contentStr).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e840f06-37dd-4373-9408-8b4339fd9f8d
📒 Files selected for processing (4)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/bedrock/chat.go
Contributor
Confidence Score: 5/5Safe to merge. All four previously flagged issues are correctly resolved, and no new defects were introduced. All previously flagged issues in the Anthropic and Bedrock paths are addressed: SeenRealToolCall is now set for computer_use before the early return; the mixed-tool finish-reason override is correctly gated by len(toolCalls) == 0 in non-streaming paths and nextToolCallIndex == 0 / !SeenRealToolCall in streaming paths. The thinkingEnabled detection logic, while duplicated four times, is applied consistently and correctly across all affected paths. No files require special attention. Important Files Changed
Reviews (9): Last reviewed commit: "fix: stop reason with structured output ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 22, 2026 05:34
f47fca4 to
e0280c7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/responses.go (1)
337-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark computer tool-use as a real tool call before finish-reason remap.
Line 337 enters the dedicated computer
tool_usepath, butstate.SeenRealToolCallis never set there. Then Lines 1319-1323 can still downgradetool_callstostopfor mixed streams that include both structured-output consumption and a computer tool call.Suggested fix
if chunk.ContentBlock.Type == AnthropicContentBlockTypeToolUse && chunk.ContentBlock.Name != nil && *chunk.ContentBlock.Name == string(AnthropicToolNameComputer) && chunk.ContentBlock.ID != nil { + state.SeenRealToolCall = true + // Start accumulating computer tool state.ComputerToolID = chunk.ContentBlock.ID state.ChunkIndex = chunk.Index state.AccumulatedJSON = ""🤖 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/responses.go` around lines 337 - 344, When detecting a computer tool use in the Anthropic response handler (the branch that checks chunk.ContentBlock.Type == AnthropicContentBlockTypeToolUse and *chunk.ContentBlock.Name == string(AnthropicToolNameComputer)), also mark this as a real tool call by setting state.SeenRealToolCall alongside assigning state.ComputerToolID and state.ChunkIndex; this ensures the later finish-reason remap logic (which inspects state.SeenRealToolCall) will not incorrectly downgrade tool_calls to stop for mixed streams.
🤖 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 `@core/providers/anthropic/anthropic.go`:
- Around line 729-730: The conversion in ToBifrostChatCompletionStream is
incorrectly treating any input_json_delta as delta.content whenever
structuredOutputToolName
(ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName)) is set, causing
real tool argument deltas to be lost for non-structured-output blocks; modify
the logic to only remap input_json_delta=>delta.content when the current block
is the structured-output block (use consumedStructuredOutput or compare the
current tool/block name against structuredOutputToolName) or remove the
delta-side branch entirely and rely on HandleAnthropicChatCompletionStreaming to
intercept structured-output blocks; ensure tool_calls and nextToolCallIndex
handling remains intact so real tool function.arguments are emitted as tool-call
deltas for non-structured-output tools.
In `@core/providers/bedrock/chat.go`:
- Around line 103-104: The bug: usedStructuredOutputTool is being set even when
ToolUse.Input is nil, causing nil-input structured-output tool blocks to be
treated as "consumed" and triggering incorrect tool_calls -> stop remapping.
Fix: in the handling logic for the structured-output tool (where
usedStructuredOutputTool is set and the code continues to skip adding to
toolCalls), add a guard to only set usedStructuredOutputTool and continue when
ToolUse.Input is non-nil (and/or non-empty if applicable) — update both the
occurrence around the usedStructuredOutputTool set near the first block and the
same pattern at the other occurrence (the block referenced at lines ~240-242) so
structured-output is only marked used when actually consumed into text.
---
Outside diff comments:
In `@core/providers/anthropic/responses.go`:
- Around line 337-344: When detecting a computer tool use in the Anthropic
response handler (the branch that checks chunk.ContentBlock.Type ==
AnthropicContentBlockTypeToolUse and *chunk.ContentBlock.Name ==
string(AnthropicToolNameComputer)), also mark this as a real tool call by
setting state.SeenRealToolCall alongside assigning state.ComputerToolID and
state.ChunkIndex; this ensures the later finish-reason remap logic (which
inspects state.SeenRealToolCall) will not incorrectly downgrade tool_calls to
stop for mixed streams.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d4c1680-3d79-4c0f-9d07-12a6332c59d8
📒 Files selected for processing (4)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/bedrock/chat.go
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 22, 2026 06:14
e0280c7 to
79657b9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1202-1223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard missing tool-call state before emitting argument deltas.
Line 1206 falls back to index
0when thecontent_block_startmapping was never recorded, so an out-of-order or malformed stream will silently attach arguments to the wrong tool call instead of failing fast.Suggested fix
case AnthropicStreamDeltaTypeInputJSON: // Handle tool use streaming - accumulate partial JSON. if chunk.Delta.PartialJSON != nil { // Resolve which tool-call this delta belongs to via the content-block index. - toolCallIdx := state.contentBlockToToolCallIdx[*chunk.Index] + toolCallIdx, ok := state.contentBlockToToolCallIdx[*chunk.Index] + if !ok { + return nil, providerUtils.NewBifrostOperationError( + "received tool input delta before tool_use start", + fmt.Errorf("missing tool_call index for content block %d", *chunk.Index), + ), false + } // Create streaming response for tool input delta streamResponse := &schemas.BifrostChatResponse{🤖 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 1202 - 1223, When handling AnthropicStreamDeltaTypeInputJSON in the Anthropic stream path, guard access to state.contentBlockToToolCallIdx before dereferencing *chunk.Index: check that chunk.Index is non-nil and that state.contentBlockToToolCallIdx contains the key, and if not, do not default to 0 — instead log or surface an error/skip the delta so we fail fast rather than attaching arguments to the wrong tool call; update the code around the AnthropicStreamDeltaTypeInputJSON case (where toolCallIdx is computed from state.contentBlockToToolCallIdx[*chunk.Index]) to perform this existence check and handle the missing mapping explicitly.
🤖 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 `@core/providers/anthropic/responses.go`:
- Around line 1319-1324: The finish-reason remap misses cases because the
earlier "computer"/tool_use handling returns before setting
state.SeenRealToolCall and because a later Anthropic-integration block
recomputes stopReason from the raw upstream value (causing message_delta
passthrough of the old reason); to fix, ensure the "computer"/tool_use branch
sets state.SeenRealToolCall (so ConvertAnthropicFinishReasonToBifrost remap
logic can detect real tool calls) and move or apply the remap after any early
returns, and modify the Anthropic-integration recompute to honor
state.StopReason (use the already-mapped mapped value if state.StopReason is
set) rather than overwriting it with the raw upstream stop reason (adjust
references in ConvertAnthropicFinishReasonToBifrost, the "computer"/tool_use
branch, mapped variable, and where state.StopReason is assigned).
---
Outside diff comments:
In `@core/providers/anthropic/chat.go`:
- Around line 1202-1223: When handling AnthropicStreamDeltaTypeInputJSON in the
Anthropic stream path, guard access to state.contentBlockToToolCallIdx before
dereferencing *chunk.Index: check that chunk.Index is non-nil and that
state.contentBlockToToolCallIdx contains the key, and if not, do not default to
0 — instead log or surface an error/skip the delta so we fail fast rather than
attaching arguments to the wrong tool call; update the code around the
AnthropicStreamDeltaTypeInputJSON case (where toolCallIdx is computed from
state.contentBlockToToolCallIdx[*chunk.Index]) to perform this existence check
and handle the missing mapping explicitly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f9b00e5-0dfd-42d5-acbf-8dfbdb47d31a
📒 Files selected for processing (4)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/bedrock/chat.go
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 22, 2026 06:37
79657b9 to
70cf79f
Compare
Contributor
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/responses.go (1)
366-466:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark
state.SeenRealToolCallfor all Anthropicserver_tool_useblocks, not onlyweb_search/web_fetch.
Anthropic server-side tools sent asserver_tool_useincludeweb_search,web_fetch,tool_search, andcode_execution; restricting the flag to justweb_search/web_fetchcan mis-remap streams when other server tools are invoked.Suggested fix
+ if chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse { + state.SeenRealToolCall = true + } + // Handle web_search server_tool_use (query block) if chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse && chunk.ContentBlock.Name != nil && *chunk.ContentBlock.Name == string(AnthropicToolNameWebSearch) && chunk.ContentBlock.ID != nil { - - state.SeenRealToolCall = true // Start accumulating web search query (reuse shared accumulation fields) state.ChunkIndex = chunk.Index state.AccumulatedJSON = "" @@ if chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse && chunk.ContentBlock.Name != nil && *chunk.ContentBlock.Name == string(AnthropicToolNameWebFetch) && chunk.ContentBlock.ID != nil { - - state.SeenRealToolCall = true state.ChunkIndex = chunk.Index state.AccumulatedJSON = ""🤖 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/responses.go` around lines 366 - 466, The code currently sets state.SeenRealToolCall only inside specific server_tool_use branches (e.g., when chunk.ContentBlock.Name == AnthropicToolNameWebSearch or WebFetch); instead, when chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse you should mark state.SeenRealToolCall = true unconditionally before checking the specific tool name so any server-side tool (tool_search, code_execution, etc.) flips the flag; update the logic around chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse in responses.go (referencing state.SeenRealToolCall and chunk.ContentBlock) to set the flag at the start of that branch rather than only inside web_search/web_fetch-specific blocks.
🤖 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/responses.go`:
- Around line 366-466: The code currently sets state.SeenRealToolCall only
inside specific server_tool_use branches (e.g., when chunk.ContentBlock.Name ==
AnthropicToolNameWebSearch or WebFetch); instead, when chunk.ContentBlock.Type
== AnthropicContentBlockTypeServerToolUse you should mark state.SeenRealToolCall
= true unconditionally before checking the specific tool name so any server-side
tool (tool_search, code_execution, etc.) flips the flag; update the logic around
chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse in
responses.go (referencing state.SeenRealToolCall and chunk.ContentBlock) to set
the flag at the start of that branch rather than only inside
web_search/web_fetch-specific blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29001cd0-4c08-4474-95e0-d2b9ddcd86bb
📒 Files selected for processing (4)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/responses.gocore/providers/bedrock/chat.go
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 22, 2026 09:25
70cf79f to
916c169
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/bedrock/chat.go (1)
199-202:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against overwriting structured-output content.
If the response contains both a structured-output tool use (which sets
contentStrat line 101) and exactly one text content block, line 200 will overwritecontentStrwith the text block, discarding the structured-output JSON. While this scenario may be rare, the code should guard against it.🛡️ Proposed fix
-if len(contentBlocks) == 1 && contentBlocks[0].Type == schemas.ChatContentBlockTypeText { +if contentStr == nil && len(contentBlocks) == 1 && contentBlocks[0].Type == schemas.ChatContentBlockTypeText { contentStr = contentBlocks[0].Text contentBlocks = nil }🤖 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/bedrock/chat.go` around lines 199 - 202, The current block unconditionally replaces contentStr when there is a single text contentBlock, which can overwrite previously set structured-output content; change the condition that sets contentStr from contentBlocks to only run when contentStr is empty (i.e., no structured-output has already been assigned). Update the logic around contentBlocks, contentStr and the ChatContentBlockTypeText check so you guard against overwriting an existing structured-output value (leave contentBlocks untouched if contentStr is already set).
🧹 Nitpick comments (2)
core/providers/anthropic/chat_test.go (2)
966-992: ⚡ Quick winAssert synthetic
bf_so_*tool presence in the MaxTokens thinking path.This test currently proves only that
ToolChoiceis unset. It can still pass if the structured-output tool is accidentally dropped, which would break schema guidance while thinking is enabled.Proposed test hardening
func TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens(t *testing.T) { @@ result, err := ToAnthropicChatRequest(ctx, bifrostReq) if err != nil { t.Fatalf("unexpected error: %v", err) } + found := false + for _, tool := range result.Tools { + if strings.HasPrefix(tool.Name, "bf_so_") { + found = true + break + } + } + if !found { + t.Fatal("expected synthetic bf_so_* tool to be present even with thinking enabled") + } + if result.ToolChoice != nil { t.Errorf("expected ToolChoice to be nil when thinking (MaxTokens) is enabled, got %+v", result.ToolChoice) } }🤖 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_test.go` around lines 966 - 992, The test TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens only asserts ToolChoice is nil; update it to assert the structured-output tool is present when Reasoning.MaxTokens is set by checking result.ToolChoice is non-nil and that result.ToolChoice.Name matches the expected structured-output tool identifier (e.g., starts with or equals "bf_so_" or the exact name used elsewhere), and optionally verify result.ToolChoice.Parameters includes the response schema/format; change the assertion logic in this test to fail if the bf_so_* tool is missing instead of only checking for nil.
1090-1101: ⚡ Quick winTighten mixed-tool assertions to ensure SO tool is filtered out.
len(ToolCalls) > 0is too broad; this still passes if the synthetic SO tool leaks into surfaced tool calls.Proposed assertion upgrade
// The real tool call must be surfaced. msg := choice.ChatNonStreamResponseChoice.Message if msg.ChatAssistantMessage == nil || len(msg.ChatAssistantMessage.ToolCalls) == 0 { t.Fatal("expected real tool calls to be present") } + if len(msg.ChatAssistantMessage.ToolCalls) != 1 { + t.Fatalf("expected exactly 1 surfaced real tool call, got %d", len(msg.ChatAssistantMessage.ToolCalls)) + } + gotName := "" + if msg.ChatAssistantMessage.ToolCalls[0].Function.Name != nil { + gotName = *msg.ChatAssistantMessage.ToolCalls[0].Function.Name + } + if gotName != "get_weather" { + t.Fatalf("expected surfaced tool call to be get_weather, got %q", gotName) + }🤖 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_test.go` around lines 1090 - 1101, The test currently only checks len(choice.ChatNonStreamResponseChoice.Message.ChatAssistantMessage.ToolCalls) > 0 which still passes if the synthetic SO tool is present; update the assertion to (1) ensure there is at least one tool call whose ToolName is not the synthetic SO tool and (2) explicitly assert that no ToolCalls have ToolName equal to the synthetic SO identifier (replace "SO" with the actual synthetic tool name used in tests), using the existing choice.ChatNonStreamResponseChoice.Message.ChatAssistantMessage.ToolCalls slice to filter, and keep the existing FinishReason assertions against schemas.BifrostFinishReasonToolCalls unchanged.
🤖 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 `@core/providers/anthropic/chat_test.go`:
- Around line 999-1000: The fixture creation currently ignores errors from
json.Marshal (jsonInput, _ := json.Marshal(...)), which can mask failures;
change to capture the error from json.Marshal (jsonInput, err :=
json.Marshal(...)) and fail the test on error (e.g., t.Fatalf or
require.NoError) wherever this pattern appears in
core/providers/anthropic/chat_test.go (including the occurrences that set
jsonInput at the shown locations and at 1053-1054) so any marshal failure
surfaces immediately.
In `@core/providers/anthropic/responses.go`:
- Line 372: The code only sets state.SeenRealToolCall for web_search and
web_fetch but should mark SeenRealToolCall for any Anthropic "server_tool_use"
block; modify the handler(s) that inspect block.Type == "server_tool_use" (where
you currently set state.SeenRealToolCall only for specific tool names) to set
state.SeenRealToolCall unconditionally whenever a server_tool_use block is
encountered, and apply the same change in the other occurrences referenced
around the existing assignments (the spots that currently special-case
web_search/web_fetch and the similar checks near the tool-call finish-reason
handling).
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 5073-5080: The test currently accesses result.Choices[0] and
choice.ChatNonStreamResponseChoice.Message without guards; update the test after
calling response.ToBifrostChatResponse to assert result != nil and
len(result.Choices) > 0 before reading result.Choices[0], then assign choice :=
result.Choices[0] and assert choice.ChatNonStreamResponseChoice != nil (or check
the field exists) before accessing choice.ChatNonStreamResponseChoice.Message to
avoid panics and match the defensive patterns used elsewhere in this file.
- Around line 5017-5024: The test blindly indexes result.Choices and
dereferences nested fields; modify the test that calls
response.ToBifrostChatResponse(ctx, "claude-opus-4-6") to first assert the slice
length (e.g., require.Len(t, result.Choices, 1)) and then require.NotNil/t.Nil
checks on result.Choices[0], result.Choices[0].ChatNonStreamResponseChoice and
its Message before accessing them; ensure the same defensive pattern used
elsewhere in this file is applied so subsequent lines that read choice :=
result.Choices[0] and msg := choice.ChatNonStreamResponseChoice.Message are
safe.
---
Outside diff comments:
In `@core/providers/bedrock/chat.go`:
- Around line 199-202: The current block unconditionally replaces contentStr
when there is a single text contentBlock, which can overwrite previously set
structured-output content; change the condition that sets contentStr from
contentBlocks to only run when contentStr is empty (i.e., no structured-output
has already been assigned). Update the logic around contentBlocks, contentStr
and the ChatContentBlockTypeText check so you guard against overwriting an
existing structured-output value (leave contentBlocks untouched if contentStr is
already set).
---
Nitpick comments:
In `@core/providers/anthropic/chat_test.go`:
- Around line 966-992: The test
TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens only
asserts ToolChoice is nil; update it to assert the structured-output tool is
present when Reasoning.MaxTokens is set by checking result.ToolChoice is non-nil
and that result.ToolChoice.Name matches the expected structured-output tool
identifier (e.g., starts with or equals "bf_so_" or the exact name used
elsewhere), and optionally verify result.ToolChoice.Parameters includes the
response schema/format; change the assertion logic in this test to fail if the
bf_so_* tool is missing instead of only checking for nil.
- Around line 1090-1101: The test currently only checks
len(choice.ChatNonStreamResponseChoice.Message.ChatAssistantMessage.ToolCalls) >
0 which still passes if the synthetic SO tool is present; update the assertion
to (1) ensure there is at least one tool call whose ToolName is not the
synthetic SO tool and (2) explicitly assert that no ToolCalls have ToolName
equal to the synthetic SO identifier (replace "SO" with the actual synthetic
tool name used in tests), using the existing
choice.ChatNonStreamResponseChoice.Message.ChatAssistantMessage.ToolCalls slice
to filter, and keep the existing FinishReason assertions against
schemas.BifrostFinishReasonToolCalls unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70dc8c4f-0bae-4104-9446-0aa69ccccdf9
📒 Files selected for processing (6)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/anthropic/responses.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/chat.go
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 22, 2026 11:12
916c169 to
6a32cc3
Compare
Contributor
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)
1202-1231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip
input_json_deltawhen no tool-call start was emitted.Line 1138 skips the
content_block_startfor structured-output tools, so this lookup falls back to0for the same block. The nextinput_json_deltathen gets emitted as a tool-call delta for index 0, which creates a phantom tool call or corrupts the first real tool call in mixed streams.Suggested fix
case AnthropicStreamDeltaTypeInputJSON: // Handle tool use streaming - accumulate partial JSON. if chunk.Delta.PartialJSON != nil { // Resolve which tool-call this delta belongs to via the content-block index. - toolCallIdx := state.contentBlockToToolCallIdx[*chunk.Index] + toolCallIdx, ok := state.contentBlockToToolCallIdx[*chunk.Index] + if !ok { + return nil, nil, false + } // Create streaming response for tool input delta streamResponse := &schemas.BifrostChatResponse{🤖 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 1202 - 1231, In the AnthropicStreamDeltaTypeInputJSON handler, avoid emitting a tool-call delta when no content-block→tool-call mapping exists: check that state.contentBlockToToolCallIdx has an entry for *chunk.Index before using it (i.e., ensure the lookup for state.contentBlockToToolCallIdx[*chunk.Index] succeeds); if there's no mapping (meaning no prior content_block_start for a structured-output tool), skip/ignore the input_json_delta instead of defaulting to index 0. Update the logic around chunk.Delta.PartialJSON in the AnthropicStreamDeltaTypeInputJSON case to perform this existence check and only construct/return the schemas.BifrostChatResponse when a valid toolCallIdx is present.
♻️ Duplicate comments (2)
core/providers/bedrock/bedrock_test.go (1)
5026-5027:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd nil guards before dereferencing
Message/Contentin these new tests.These accesses can panic before assertions fire if conversion returns a nil
Message(or nilContentin the first test), which makes failures harder to diagnose.Suggested patch
// Content must be the JSON from the SO tool. msg := choice.ChatNonStreamResponseChoice.Message + require.NotNil(t, msg, "expected message to be present") + require.NotNil(t, msg.Content, "expected message content to be present") assert.NotNil(t, msg.Content.ContentStr, "expected ContentStr to be set from SO tool input") @@ // The real tool call must be surfaced. msg := choice.ChatNonStreamResponseChoice.Message + require.NotNil(t, msg, "expected message to be present") require.NotNil(t, msg.ChatAssistantMessage) assert.NotEmpty(t, msg.ChatAssistantMessage.ToolCalls, "expected real tool calls to be present")Also applies to: 5084-5086
🤖 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/bedrock/bedrock_test.go` around lines 5026 - 5027, Add nil guards before dereferencing the test values: check that choice.ChatNonStreamResponseChoice is not nil, then that msg := choice.ChatNonStreamResponseChoice.Message is not nil, and that msg.Content is not nil before asserting on msg.Content.ContentStr; replace direct dereferences with assert.NotNil checks (or require.NotNil) for those symbols and then perform the existing assertions, and apply the same pattern for the other occurrence around the lines referencing msg/Content at the second location (the block covering the 5084-5086 equivalent).core/providers/anthropic/responses.go (1)
334-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark every
server_tool_useas a real tool call.
SeenRealToolCallis only set inside theweb_search/web_fetchbranches here. Any otherserver_tool_usewill still let Line 1321 downgrade a realtool_callsfinish reason tostop, while the non-streaming path at Lines 2773-2779 already treats allserver_tool_useblocks as real.Suggested fix
if chunk.ContentBlock != nil && chunk.Index != nil { outputIndex := state.getOrCreateOutputIndex(chunk.Index) + if chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse { + state.SeenRealToolCall = true + } if chunk.ContentBlock.Type == AnthropicContentBlockTypeToolUse && chunk.ContentBlock.Name != nil && *chunk.ContentBlock.Name == string(AnthropicToolNameComputer) && chunk.ContentBlock.ID != nil { @@ - state.SeenRealToolCall = true - // Start accumulating web search query (reuse shared accumulation fields) @@ - state.SeenRealToolCall = true - state.ChunkIndex = chunk.IndexAlso applies to: 460-466
🤖 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/responses.go` around lines 334 - 372, The code only sets state.SeenRealToolCall for specific server tool names (web_search/web_fetch); update the handling for AnthropicContentBlockTypeServerToolUse in the response processing (the branch that checks chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse) to unconditionally set state.SeenRealToolCall = true (and preserve any existing per-tool logic after that) so that any server_tool_use is treated as a real tool call; apply the same change to the other occurrence around the second server_tool_use branch (the block referenced at lines 460-466) so both places consistently mark all server_tool_use blocks as real tool calls.
🧹 Nitpick comments (1)
core/providers/anthropic/chat_test.go (1)
966-992: ⚡ Quick winAdd the missing synthetic-tool assertion in the MaxTokens thinking test.
This test currently verifies only that
ToolChoiceis nil. It should also assert thebf_so_*tool is still present (same contract as the effort-based test), otherwise this path can regress silently.Proposed patch
func TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens(t *testing.T) { @@ result, err := ToAnthropicChatRequest(ctx, bifrostReq) if err != nil { t.Fatalf("unexpected error: %v", err) } + // Synthetic tool must still be present so the model sees the schema. + found := false + for _, tool := range result.Tools { + if len(tool.Name) > 6 && tool.Name[:6] == "bf_so_" { + found = true + break + } + } + if !found { + t.Fatal("expected synthetic bf_so_* tool to be present even with thinking enabled") + } + if result.ToolChoice != nil { t.Errorf("expected ToolChoice to be nil when thinking (MaxTokens) is enabled, got %+v", result.ToolChoice) } }🤖 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_test.go` around lines 966 - 992, The test TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens only asserts result.ToolChoice is nil; update it to also verify the synthetic structured-output tool is included (same contract as the effort-based test). After calling ToAnthropicChatRequest, iterate the returned result.Tools (or equivalent slice) and assert there exists a tool whose Name matches the bf_so_ prefix (e.g., startsWith "bf_so_") and that its schema/metadata corresponds to the "my_schema" response format; keep the existing ToolChoice nil assertion intact. This change should be done in the TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens test function.
🤖 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 1202-1231: In the AnthropicStreamDeltaTypeInputJSON handler, avoid
emitting a tool-call delta when no content-block→tool-call mapping exists: check
that state.contentBlockToToolCallIdx has an entry for *chunk.Index before using
it (i.e., ensure the lookup for state.contentBlockToToolCallIdx[*chunk.Index]
succeeds); if there's no mapping (meaning no prior content_block_start for a
structured-output tool), skip/ignore the input_json_delta instead of defaulting
to index 0. Update the logic around chunk.Delta.PartialJSON in the
AnthropicStreamDeltaTypeInputJSON case to perform this existence check and only
construct/return the schemas.BifrostChatResponse when a valid toolCallIdx is
present.
---
Duplicate comments:
In `@core/providers/anthropic/responses.go`:
- Around line 334-372: The code only sets state.SeenRealToolCall for specific
server tool names (web_search/web_fetch); update the handling for
AnthropicContentBlockTypeServerToolUse in the response processing (the branch
that checks chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse)
to unconditionally set state.SeenRealToolCall = true (and preserve any existing
per-tool logic after that) so that any server_tool_use is treated as a real tool
call; apply the same change to the other occurrence around the second
server_tool_use branch (the block referenced at lines 460-466) so both places
consistently mark all server_tool_use blocks as real tool calls.
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 5026-5027: Add nil guards before dereferencing the test values:
check that choice.ChatNonStreamResponseChoice is not nil, then that msg :=
choice.ChatNonStreamResponseChoice.Message is not nil, and that msg.Content is
not nil before asserting on msg.Content.ContentStr; replace direct dereferences
with assert.NotNil checks (or require.NotNil) for those symbols and then perform
the existing assertions, and apply the same pattern for the other occurrence
around the lines referencing msg/Content at the second location (the block
covering the 5084-5086 equivalent).
---
Nitpick comments:
In `@core/providers/anthropic/chat_test.go`:
- Around line 966-992: The test
TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens only
asserts result.ToolChoice is nil; update it to also verify the synthetic
structured-output tool is included (same contract as the effort-based test).
After calling ToAnthropicChatRequest, iterate the returned result.Tools (or
equivalent slice) and assert there exists a tool whose Name matches the bf_so_
prefix (e.g., startsWith "bf_so_") and that its schema/metadata corresponds to
the "my_schema" response format; keep the existing ToolChoice nil assertion
intact. This change should be done in the
TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens test
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cfe2ff8-c788-4f69-9601-18e8f212c3b7
📒 Files selected for processing (6)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/anthropic/responses.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/chat.go
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 22, 2026 13:15
6a32cc3 to
a28912f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/bedrock/bedrock.go (1)
1750-1760:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay
UsedStructuredOutputTooluntil structured-output text is actually emitted.This now flips the flag on the tool-start event, before any
ToolUse.Inputdelta has been folded into text. If the stream starts the structured-output tool but emits no input delta, downstream finalization can still rewritetool_callstostopeven though nothing was consumed.Suggested fix
if streamEvent.Start != nil && streamEvent.Start.ToolUse != nil { if streamEvent.Start.ToolUse.Name == structuredOutputToolName { // This is the structured output tool - start accumulating, don't forward isAccumulatingStructuredOutput = true - streamState.UsedStructuredOutputTool = true continue } } @@ if streamEvent.Delta != nil && streamEvent.Delta.ToolUse != nil && isAccumulatingStructuredOutput { + streamState.UsedStructuredOutputTool = true // Convert tool use delta to text delta content := streamEvent.Delta.ToolUse.Input🤖 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/bedrock/bedrock.go` around lines 1750 - 1760, The code currently sets streamState.UsedStructuredOutputTool when a Start.ToolUse for structuredOutputToolName is seen; change this so UsedStructuredOutputTool is only set when actual structured-output text is emitted: keep setting isAccumulatingStructuredOutput = true on the Start.ToolUse branch (and do not set UsedStructuredOutputTool there), then in the branch handling streamEvent.Delta.ToolUse (where isAccumulatingStructuredOutput is true) set streamState.UsedStructuredOutputTool = true only when the delta contains actual input text (check streamEvent.Delta.ToolUse.Input is non-nil and non-empty) so the flag reflects real consumption rather than just a started tool.
🧹 Nitpick comments (1)
core/providers/anthropic/chat_test.go (1)
1098-1102: ⚡ Quick winStrengthen mixed-tool assertion to avoid false positives.
This currently passes if any tool call is present. Please assert that the surfaced tool call includes
get_weather(and ideally excludesbf_so_*) so SO-filter regressions are caught.Proposed diff
- // The real tool call must be surfaced. - msg := choice.ChatNonStreamResponseChoice.Message - if msg.ChatAssistantMessage == nil || len(msg.ChatAssistantMessage.ToolCalls) == 0 { - t.Fatal("expected real tool calls to be present") - } + // The real tool call must be surfaced (and SO tool should not be surfaced). + msg := choice.ChatNonStreamResponseChoice.Message + if msg.ChatAssistantMessage == nil || len(msg.ChatAssistantMessage.ToolCalls) == 0 { + t.Fatal("expected real tool calls to be present") + } + foundReal := false + for _, tc := range msg.ChatAssistantMessage.ToolCalls { + if tc.Function.Name != nil && *tc.Function.Name == "get_weather" { + foundReal = true + } + if tc.Function.Name != nil && strings.HasPrefix(*tc.Function.Name, "bf_so_") { + t.Fatalf("expected SO tool call to be folded into content, got surfaced tool %q", *tc.Function.Name) + } + } + if !foundReal { + t.Fatalf("expected surfaced tool calls to include %q, got %+v", "get_weather", msg.ChatAssistantMessage.ToolCalls) + }🤖 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_test.go` around lines 1098 - 1102, The test currently only checks that any tool call exists on choice.ChatNonStreamResponseChoice.Message.ChatAssistantMessage.ToolCalls, which can mask SO-filter regressions; update the assertion to verify that the tool calls include an entry whose name equals "get_weather" and does not include tool names starting with "bf_so_" (or specifically assert absence of known bf_so_* names) so the surfaced tool call is the real one expected; locate the check around choice.ChatNonStreamResponseChoice.Message and replace the broad non-empty assertion with explicit includes/excludes on the ToolCalls slice.
🤖 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 `@core/providers/bedrock/responses.go`:
- Line 40: The pooled object returns stale per-request state because the
UsedStructuredOutputTool boolean (defined at UsedStructuredOutputTool) isn't
cleared in flush() before the object is returned to the pool; update flush() to
explicitly reset UsedStructuredOutputTool = false (and any other per-request
flags referenced in the 146-210 range) before calling pool.Put so no
request-local state leaks to the next user of the pooled instance.
- Around line 2530-2543: The logic that downgrades stopReason from
BifrostFinishReasonToolCalls to Stop only checks for
schemas.ResponsesMessageTypeFunctionCall; extend the detection in the loop over
bifrostResp.Output (and where stopReason is set) to treat other tool-call types
as real tool calls too—e.g., include schemas.ResponsesMessageTypeWebSearchCall
and schemas.ResponsesMessageTypeCodeInterpreterCall or detect any message.Type
string that ends with "_call" (in the check inside the for loop) so that when
ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName) is present you
don't incorrectly remap tool_calls to stop.
---
Outside diff comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1750-1760: The code currently sets
streamState.UsedStructuredOutputTool when a Start.ToolUse for
structuredOutputToolName is seen; change this so UsedStructuredOutputTool is
only set when actual structured-output text is emitted: keep setting
isAccumulatingStructuredOutput = true on the Start.ToolUse branch (and do not
set UsedStructuredOutputTool there), then in the branch handling
streamEvent.Delta.ToolUse (where isAccumulatingStructuredOutput is true) set
streamState.UsedStructuredOutputTool = true only when the delta contains actual
input text (check streamEvent.Delta.ToolUse.Input is non-nil and non-empty) so
the flag reflects real consumption rather than just a started tool.
---
Nitpick comments:
In `@core/providers/anthropic/chat_test.go`:
- Around line 1098-1102: The test currently only checks that any tool call
exists on
choice.ChatNonStreamResponseChoice.Message.ChatAssistantMessage.ToolCalls, which
can mask SO-filter regressions; update the assertion to verify that the tool
calls include an entry whose name equals "get_weather" and does not include tool
names starting with "bf_so_" (or specifically assert absence of known bf_so_*
names) so the surfaced tool call is the real one expected; locate the check
around choice.ChatNonStreamResponseChoice.Message and replace the broad
non-empty assertion with explicit includes/excludes on the ToolCalls slice.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e86517d-8bf0-4c56-ae3d-8f2fc062d566
📒 Files selected for processing (9)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/anthropic/responses.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.go
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 22, 2026 13:26
a28912f to
e1c5929
Compare
Contributor
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 (2)
core/providers/bedrock/bedrock.go (1)
1748-1780:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let the structured-output state swallow later real-tool deltas.
Once Line 1753 sets
isAccumulatingStructuredOutput = true, the Line 1760 branch converts every subsequentdelta.toolUseinto text until the stream ends. If Bedrock emits a real tool block after the structured-output block, its args will still be sent asoutput_text_deltainstead of a tool call, so the mixed real-tool + structured-output case is still corrupted.🤖 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/bedrock/bedrock.go` around lines 1748 - 1780, The code sets isAccumulatingStructuredOutput=true when seeing a ToolUse start for structuredOutputToolName but then blindly converts every subsequent streamEvent.Delta.ToolUse into an output_text_delta; change the delta handling so you only convert ToolUse deltas when the delta actually belongs to the structured-output tool (compare streamEvent.Delta.ToolUse.Name with structuredOutputToolName), and if you encounter a ToolUse for a different tool while isAccumulatingStructuredOutput is true, clear isAccumulatingStructuredOutput (stop accumulating) and allow normal tool-call processing to proceed (don't convert that delta to text or continue); update the branch around streamEvent.Delta.ToolUse and use the existing isAccumulatingStructuredOutput and structuredOutputToolName symbols to implement this logic.core/providers/anthropic/chat.go (1)
799-836:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly remap to
stopafter the structured-output payload actually wins the outgoing content slot.
usedStructuredOutputToolflips as soon as the tool block is seen, but Lines 876-878 can still overwritecontentStrwith a normal text block afterward. In atext + structured_output_toolresponse, this leaves the client with plain text while Lines 927-930 still remaptool_callstostop, so the structured JSON was never actually returned.Based on learnings,
schemas.ChatMessageContent.ContentStrandContentBlocksare mutually exclusive content sources, so this remap needs to follow whichever source is finally kept.Also applies to: 925-930
🤖 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 799 - 836, The code sets usedStructuredOutputTool as soon as a structured-output tool block is seen, but contentStr can later be overwritten by normal text blocks, so move the decision to remap tool_calls to "stop" until after you've processed all response.Content: do not flip usedStructuredOutputTool inside the loop; instead after the loop decide which content source won (if contentStr != nil and contentBlocks is empty or contentStr is the chosen outgoing content) and only then set usedStructuredOutputTool (or perform the remap of toolCalls -> "stop"); update the logic that currently inspects usedStructuredOutputTool before remapping (the block that converts toolCalls to stop) to check the final content choice (contentStr vs contentBlocks) so tool_calls are remapped only when the structured-output payload is actually the outgoing content.
♻️ Duplicate comments (1)
core/providers/anthropic/responses.go (1)
366-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark every
server_tool_useas a real tool call.Only
web_searchandweb_fetchsetSeenRealToolCalltoday. Any other Anthropicserver_tool_useblock will still hit the remap at Line 1321 and incorrectly downgrade a realtool_callsfinish reason tostop, while the non-streaming path already treats allserver_tool_useblocks as real.Suggested fix
if chunk.ContentBlock != nil && chunk.Index != nil { outputIndex := state.getOrCreateOutputIndex(chunk.Index) + if chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse { + state.SeenRealToolCall = true + } if chunk.ContentBlock.Type == AnthropicContentBlockTypeToolUse && chunk.ContentBlock.Name != nil && *chunk.ContentBlock.Name == string(AnthropicToolNameComputer) && chunk.ContentBlock.ID != nil { @@ - state.SeenRealToolCall = true - // Start accumulating web search query (reuse shared accumulation fields) @@ - state.SeenRealToolCall = true - state.ChunkIndex = chunk.IndexAs per coding guidelines, always see all changes in the light of the whole stack of PRs.
Also applies to: 460-466, 1320-1325
🤖 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/responses.go` around lines 366 - 372, The code only sets state.SeenRealToolCall = true for server_tool_use when the ContentBlock.Name equals web_search (and separately web_fetch elsewhere); change the logic so any chunk where chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse sets state.SeenRealToolCall = true regardless of Name/ID. Update the three places that currently check Name (the blocks around the current snippet and the other occurrences referencing AnthropicContentBlockTypeServerToolUse at the other two spots) to remove the Name check and unconditionally mark SeenRealToolCall for that type so tool_calls finish reasons aren't remapped incorrectly.
🤖 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 799-836: The code sets usedStructuredOutputTool as soon as a
structured-output tool block is seen, but contentStr can later be overwritten by
normal text blocks, so move the decision to remap tool_calls to "stop" until
after you've processed all response.Content: do not flip
usedStructuredOutputTool inside the loop; instead after the loop decide which
content source won (if contentStr != nil and contentBlocks is empty or
contentStr is the chosen outgoing content) and only then set
usedStructuredOutputTool (or perform the remap of toolCalls -> "stop"); update
the logic that currently inspects usedStructuredOutputTool before remapping (the
block that converts toolCalls to stop) to check the final content choice
(contentStr vs contentBlocks) so tool_calls are remapped only when the
structured-output payload is actually the outgoing content.
In `@core/providers/bedrock/bedrock.go`:
- Around line 1748-1780: The code sets isAccumulatingStructuredOutput=true when
seeing a ToolUse start for structuredOutputToolName but then blindly converts
every subsequent streamEvent.Delta.ToolUse into an output_text_delta; change the
delta handling so you only convert ToolUse deltas when the delta actually
belongs to the structured-output tool (compare streamEvent.Delta.ToolUse.Name
with structuredOutputToolName), and if you encounter a ToolUse for a different
tool while isAccumulatingStructuredOutput is true, clear
isAccumulatingStructuredOutput (stop accumulating) and allow normal tool-call
processing to proceed (don't convert that delta to text or continue); update the
branch around streamEvent.Delta.ToolUse and use the existing
isAccumulatingStructuredOutput and structuredOutputToolName symbols to implement
this logic.
---
Duplicate comments:
In `@core/providers/anthropic/responses.go`:
- Around line 366-372: The code only sets state.SeenRealToolCall = true for
server_tool_use when the ContentBlock.Name equals web_search (and separately
web_fetch elsewhere); change the logic so any chunk where
chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse sets
state.SeenRealToolCall = true regardless of Name/ID. Update the three places
that currently check Name (the blocks around the current snippet and the other
occurrences referencing AnthropicContentBlockTypeServerToolUse at the other two
spots) to remove the Name check and unconditionally mark SeenRealToolCall for
that type so tool_calls finish reasons aren't remapped incorrectly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d77cb1c9-e646-41d1-ad2b-4ffb8bf6d829
📒 Files selected for processing (9)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/anthropic/responses.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.go
akshaydeo
dismissed
coderabbitai[bot]’s stale review
May 22, 2026 15:16
The merge-base changed after approval.
TejasGhatte
force-pushed
the
05-22-fix_stop_reason_with_structured_output_tools
branch
from
May 25, 2026 08:46
e1c5929 to
8939a89
Compare
Contributor
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 (2)
core/providers/anthropic/anthropic.go (1)
853-855:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrack SO consumption only when content was actually emitted.
consumedStructuredOutputis set oncontent_block_stopeven if noinput_json_deltawas converted todelta.content. That can still remaptool_calls -> stopwithout real content consumption.Suggested fix
- var isAccumulatingStructuredOutput bool - var consumedStructuredOutput bool // true once the SO tool block has been fully streamed as content + var isAccumulatingStructuredOutput bool + var consumedStructuredOutput bool // true once SO content was actually emitted + var emittedStructuredOutputContent bool @@ if event.Type == AnthropicStreamEventTypeContentBlockStart { if event.ContentBlock != nil && event.ContentBlock.Type == AnthropicContentBlockTypeToolUse { if event.ContentBlock.Name != nil && *event.ContentBlock.Name == structuredOutputToolName { isAccumulatingStructuredOutput = true + emittedStructuredOutputContent = false continue } } } @@ if event.Type == AnthropicStreamEventTypeContentBlockDelta && isAccumulatingStructuredOutput { if event.Delta != nil && event.Delta.Type == AnthropicStreamDeltaTypeInputJSON && event.Delta.PartialJSON != nil { + emittedStructuredOutputContent = true // Convert tool use delta to content delta content := *event.Delta.PartialJSON @@ if event.Type == AnthropicStreamEventTypeContentBlockStop && isAccumulatingStructuredOutput { isAccumulatingStructuredOutput = false - consumedStructuredOutput = true + consumedStructuredOutput = consumedStructuredOutput || emittedStructuredOutputContent continue }Also applies to: 888-890
🤖 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/anthropic.go` around lines 853 - 855, The code sets consumedStructuredOutput on content-block stop even when no input_json_delta was converted into actual content, which can misattribute consumption; update the handlers around AnthropicStreamEventTypeContentBlockDelta (the branch where isAccumulatingStructuredOutput and event.Delta.Type == AnthropicStreamDeltaTypeInputJSON && event.Delta.PartialJSON) and the content-block-stop branch (the other location currently setting consumedStructuredOutput) to only mark consumedStructuredOutput = true after you have actually converted/added non-empty content (i.e., the conversion produced real delta content or appended to the content buffer), so check the conversion result (non-empty/wasConverted flag) before setting consumedStructuredOutput. Ensure both places reference the same condition so tool_calls -> stop is only remapped when content was truly emitted.core/providers/anthropic/chat.go (1)
1202-1231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard input-json deltas for unregistered tool blocks.
Line 1138 skips registering the structured-output tool on
content_block_start, but this branch still falls back to tool-call index0when the map has no entry. In mixed real-tool + structured-output streams, that can attach structured-output JSON to the first real tool call without any preceding name/id chunk.Suggested fix
case AnthropicStreamDeltaTypeInputJSON: // Handle tool use streaming - accumulate partial JSON. if chunk.Delta.PartialJSON != nil { // Resolve which tool-call this delta belongs to via the content-block index. - toolCallIdx := state.contentBlockToToolCallIdx[*chunk.Index] + toolCallIdx, ok := state.contentBlockToToolCallIdx[*chunk.Index] + if !ok { + return nil, nil, false + } // Create streaming response for tool input delta streamResponse := &schemas.BifrostChatResponse{🤖 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 1202 - 1231, The handler for AnthropicStreamDeltaTypeInputJSON currently assumes state.contentBlockToToolCallIdx[*chunk.Index] exists and falls back to index 0; change it to check the map for presence (e.g., retrieve with ok := state.contentBlockToToolCallIdx[*chunk.Index]) and only build/return the tool-call streaming response when ok is true; if the entry is missing, do not emit a tool-call chunk (skip/return nil for the stream response) so structured-output JSON doesn't get attached to the wrong tool call. Ensure you update the branch that creates the schemas.BifrostChatResponse to reference the looked-up toolCallIdx variable only when the map lookup succeeded.
♻️ Duplicate comments (2)
core/providers/bedrock/bedrock_test.go (1)
5026-5027:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
Messagenil guards before dereference.
msg := choice.ChatNonStreamResponseChoice.Messageis dereferenced immediately in both tests. Add an explicit guard first to avoid panic risk if conversion shape regresses (Line 5026 and Line 5084).Suggested fix
choice := result.Choices[0] require.NotNil(t, choice.ChatNonStreamResponseChoice, "expected non-streaming response choice") // Content must be the JSON from the SO tool. msg := choice.ChatNonStreamResponseChoice.Message + require.NotNil(t, msg, "expected non-streaming message") assert.NotNil(t, msg.Content.ContentStr, "expected ContentStr to be set from SO tool input")choice := result.Choices[0] require.NotNil(t, choice.ChatNonStreamResponseChoice, "expected non-streaming response choice") // The real tool call must be surfaced. msg := choice.ChatNonStreamResponseChoice.Message + require.NotNil(t, msg, "expected non-streaming message") require.NotNil(t, msg.ChatAssistantMessage) assert.NotEmpty(t, msg.ChatAssistantMessage.ToolCalls, "expected real tool calls to be present")Also applies to: 5084-5086
🤖 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/bedrock/bedrock_test.go` around lines 5026 - 5027, The test dereferences choice.ChatNonStreamResponseChoice.Message into msg and then immediately reads msg.Content.ContentStr, which can panic if Message is nil; update both occurrences (the assignment to msg and subsequent checks at the two spots) to first assert/require that msg is not nil (e.g., assert.NotNil(t, msg, "expected Message to be set") or require.NotNil to stop the test), then proceed to assert.NotNil(t, msg.Content.ContentStr, "expected ContentStr to be set from SO tool input"); this adds a proper nil guard for choice.ChatNonStreamResponseChoice.Message and prevents a panic if the conversion shape regresses.core/providers/anthropic/responses.go (1)
334-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack every
server_tool_usebefore remapping finish reasons.This still only flips
SeenRealToolCallforweb_searchandweb_fetch. Any otherserver_tool_useblock will leave the flag false, so Line 1321 can still downgrade a legitimatetool_callsfinish reason tostopafter structured output is consumed. The non-streaming path now treats anyserver_tool_useas real tool use, so the stream path should match it.Suggested fix
if chunk.ContentBlock != nil && chunk.Index != nil { outputIndex := state.getOrCreateOutputIndex(chunk.Index) + if chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse { + state.SeenRealToolCall = true + } if chunk.ContentBlock.Type == AnthropicContentBlockTypeToolUse && chunk.ContentBlock.Name != nil && *chunk.ContentBlock.Name == string(AnthropicToolNameComputer) && chunk.ContentBlock.ID != nil { @@ - state.SeenRealToolCall = true - // Start accumulating web search query (reuse shared accumulation fields) @@ - state.SeenRealToolCall = true - state.ChunkIndex = chunk.IndexAs per coding guidelines, always see all changes in the light of the whole stack of PRs.
Also applies to: 461-466
🤖 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/responses.go` around lines 334 - 372, The code only sets state.SeenRealToolCall for specific server tool names (web_search / web_fetch), so other AnthropicContentBlockTypeServerToolUse blocks are not marked as real tool use and may cause finish-reason remapping bugs; update the server-tool handling in the response parsing (the branch checking AnthropicContentBlockTypeServerToolUse / chunk.ContentBlock.Name / *chunk.ContentBlock.Name == string(AnthropicToolNameWebSearch) / AnthropicToolNameWebFetch) to set state.SeenRealToolCall = true for any server_tool_use (i.e., set it when chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse and chunk.ContentBlock.ID != nil), and mirror the same change in the other similar handler (the other block around the same pattern) so both streaming and non-streaming paths treat all server_tool_use as real tool calls; reference state.SeenRealToolCall, AnthropicContentBlockTypeServerToolUse, chunk.ContentBlock.ID, and AnthropicToolNameWebSearch/WebFetch to locate the branches to change.
🧹 Nitpick comments (2)
core/providers/anthropic/chat_test.go (2)
966-991: ⚡ Quick winAssert SO tool presence in the max-tokens thinking path.
This test only verifies
ToolChoice == nil; it can still pass if the syntheticbf_so_*tool is accidentally not added.Suggested diff
func TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens(t *testing.T) { @@ result, err := ToAnthropicChatRequest(ctx, bifrostReq) if err != nil { t.Fatalf("unexpected error: %v", err) } + // Synthetic tool must still be present so the model knows the schema. + found := false + for _, tool := range result.Tools { + if strings.HasPrefix(tool.Name, "bf_so_") { + found = true + break + } + } + if !found { + t.Fatal("expected synthetic bf_so_* tool to be present even with thinking enabled") + } + if result.ToolChoice != nil { t.Errorf("expected ToolChoice to be nil when thinking (MaxTokens) is enabled, got %+v", result.ToolChoice) } }🤖 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_test.go` around lines 966 - 991, The test TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens currently only asserts result.ToolChoice == nil which can pass even if the synthetic structured-output tool (bf_so_*) was not added; update the assertion after calling ToAnthropicChatRequest (using bifrostReq and result) to also verify that the returned result.Tools slice includes the expected structured-output tool (name starts with "bf_so_" or matches the SO tool identifier your code uses) and that its schema/response format matches rf, while still asserting ToolChoice is nil when reasoning.MaxTokens is set.
1098-1102: ⚡ Quick winTighten mixed-tools assertions to ensure SO tool is not surfaced as a callable tool.
The current check passes with any non-empty tool list, including accidental leakage of the synthetic
bf_so_*tool.Suggested diff
// The real tool call must be surfaced. msg := choice.ChatNonStreamResponseChoice.Message if msg.ChatAssistantMessage == nil || len(msg.ChatAssistantMessage.ToolCalls) == 0 { t.Fatal("expected real tool calls to be present") } + if len(msg.ChatAssistantMessage.ToolCalls) != 1 { + t.Fatalf("expected exactly 1 surfaced real tool call, got %d", len(msg.ChatAssistantMessage.ToolCalls)) + } + tc := msg.ChatAssistantMessage.ToolCalls[0] + if tc.Function.Name == nil || *tc.Function.Name != "get_weather" { + t.Fatalf("expected surfaced tool to be get_weather, got %+v", tc.Function.Name) + } // Finish reason must remain "tool_calls". if choice.FinishReason == nil { t.Fatal("expected FinishReason to be set") }🤖 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_test.go` around lines 1098 - 1102, The test currently only asserts ToolCalls is non-empty but can mistakenly allow synthetic StackOverflow tools; update the assertion after reading choice.ChatNonStreamResponseChoice.Message and msg := ...ChatAssistantMessage to iterate msg.ChatAssistantMessage.ToolCalls and (1) assert there is at least one real callable tool with a valid name, and (2) assert none of the ToolCalls have names starting with the synthetic prefix (e.g., "bf_so_") or equal to that pattern; use the existing choice.ChatNonStreamResponseChoice.Message and msg.ChatAssistantMessage.ToolCalls identifiers to locate the code and fail the test if any tool name matches the synthetic prefix.
🤖 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/anthropic.go`:
- Around line 853-855: The code sets consumedStructuredOutput on content-block
stop even when no input_json_delta was converted into actual content, which can
misattribute consumption; update the handlers around
AnthropicStreamEventTypeContentBlockDelta (the branch where
isAccumulatingStructuredOutput and event.Delta.Type ==
AnthropicStreamDeltaTypeInputJSON && event.Delta.PartialJSON) and the
content-block-stop branch (the other location currently setting
consumedStructuredOutput) to only mark consumedStructuredOutput = true after you
have actually converted/added non-empty content (i.e., the conversion produced
real delta content or appended to the content buffer), so check the conversion
result (non-empty/wasConverted flag) before setting consumedStructuredOutput.
Ensure both places reference the same condition so tool_calls -> stop is only
remapped when content was truly emitted.
In `@core/providers/anthropic/chat.go`:
- Around line 1202-1231: The handler for AnthropicStreamDeltaTypeInputJSON
currently assumes state.contentBlockToToolCallIdx[*chunk.Index] exists and falls
back to index 0; change it to check the map for presence (e.g., retrieve with ok
:= state.contentBlockToToolCallIdx[*chunk.Index]) and only build/return the
tool-call streaming response when ok is true; if the entry is missing, do not
emit a tool-call chunk (skip/return nil for the stream response) so
structured-output JSON doesn't get attached to the wrong tool call. Ensure you
update the branch that creates the schemas.BifrostChatResponse to reference the
looked-up toolCallIdx variable only when the map lookup succeeded.
---
Duplicate comments:
In `@core/providers/anthropic/responses.go`:
- Around line 334-372: The code only sets state.SeenRealToolCall for specific
server tool names (web_search / web_fetch), so other
AnthropicContentBlockTypeServerToolUse blocks are not marked as real tool use
and may cause finish-reason remapping bugs; update the server-tool handling in
the response parsing (the branch checking AnthropicContentBlockTypeServerToolUse
/ chunk.ContentBlock.Name / *chunk.ContentBlock.Name ==
string(AnthropicToolNameWebSearch) / AnthropicToolNameWebFetch) to set
state.SeenRealToolCall = true for any server_tool_use (i.e., set it when
chunk.ContentBlock.Type == AnthropicContentBlockTypeServerToolUse and
chunk.ContentBlock.ID != nil), and mirror the same change in the other similar
handler (the other block around the same pattern) so both streaming and
non-streaming paths treat all server_tool_use as real tool calls; reference
state.SeenRealToolCall, AnthropicContentBlockTypeServerToolUse,
chunk.ContentBlock.ID, and AnthropicToolNameWebSearch/WebFetch to locate the
branches to change.
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 5026-5027: The test dereferences
choice.ChatNonStreamResponseChoice.Message into msg and then immediately reads
msg.Content.ContentStr, which can panic if Message is nil; update both
occurrences (the assignment to msg and subsequent checks at the two spots) to
first assert/require that msg is not nil (e.g., assert.NotNil(t, msg, "expected
Message to be set") or require.NotNil to stop the test), then proceed to
assert.NotNil(t, msg.Content.ContentStr, "expected ContentStr to be set from SO
tool input"); this adds a proper nil guard for
choice.ChatNonStreamResponseChoice.Message and prevents a panic if the
conversion shape regresses.
---
Nitpick comments:
In `@core/providers/anthropic/chat_test.go`:
- Around line 966-991: The test
TestToAnthropicChatRequest_StructuredOutput_Vertex_ThinkingMaxTokens currently
only asserts result.ToolChoice == nil which can pass even if the synthetic
structured-output tool (bf_so_*) was not added; update the assertion after
calling ToAnthropicChatRequest (using bifrostReq and result) to also verify that
the returned result.Tools slice includes the expected structured-output tool
(name starts with "bf_so_" or matches the SO tool identifier your code uses) and
that its schema/response format matches rf, while still asserting ToolChoice is
nil when reasoning.MaxTokens is set.
- Around line 1098-1102: The test currently only asserts ToolCalls is non-empty
but can mistakenly allow synthetic StackOverflow tools; update the assertion
after reading choice.ChatNonStreamResponseChoice.Message and msg :=
...ChatAssistantMessage to iterate msg.ChatAssistantMessage.ToolCalls and (1)
assert there is at least one real callable tool with a valid name, and (2)
assert none of the ToolCalls have names starting with the synthetic prefix
(e.g., "bf_so_") or equal to that pattern; use the existing
choice.ChatNonStreamResponseChoice.Message and
msg.ChatAssistantMessage.ToolCalls identifiers to locate the code and fail the
test if any tool name matches the synthetic prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 161ce2b7-ca0c-43a3-a347-b354268e74d9
📒 Files selected for processing (9)
core/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/anthropic/responses.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.go
✅ Files skipped from review due to trivial changes (1)
- core/providers/bedrock/bedrock.go
akshaydeo
approved these changes
May 25, 2026
Contributor
Merge activity
|
akshaydeo
pushed a commit
that referenced
this pull request
May 26, 2026
### TL;DR Fix incorrect `tool_calls` finish reason being returned when structured output (response format) is used with extended thinking enabled on Anthropic and Bedrock providers. ### What changed? - When extended thinking (reasoning) is active on Anthropic, forcing `tool_choice` to the structured output tool is now skipped, since Anthropic rejects that combination. The tool is still appended and the model may call it voluntarily. - The finish reason override from `tool_calls` → `stop` for structured output is now gated on whether the structured output tool block was **actually consumed into text content**, rather than just whether a structured output tool name was configured. This prevents incorrectly overriding the finish reason in cases where the tool was never invoked. - This fix is applied consistently across the chat completion (non-streaming and streaming), responses (non-streaming and streaming), and Bedrock chat completion paths. - A `UsedStructuredOutputTool` / `consumedStructuredOutput` flag is tracked per-response and per-stream to record when the SO tool block is folded back into text content, and the finish reason override only fires when that flag is set. - For the non-streaming responses path, the override logic inspects the response content blocks directly to confirm no real (non-SO) tool calls are present before remapping the stop reason. ### How to test? 1. Send a chat completion or responses request with a `response_format` (structured output) and extended thinking/reasoning enabled against an Anthropic model. Verify the finish reason is `stop` and no API rejection occurs due to conflicting `tool_choice`. 2. Send a request with `response_format` but **without** extended thinking. Verify the finish reason is still `stop` and the structured output JSON is returned as content. 3. Send a request that uses both real tool calls and structured output simultaneously. Verify the finish reason correctly reflects `tool_calls` for the real tools. 4. Repeat the above for streaming and non-streaming variants, and for the Bedrock provider. ### Why make this change? Anthropic rejects requests that combine extended thinking with a forced `tool_choice`, causing failures when structured output was requested alongside reasoning. Additionally, the previous finish reason override was too broad — it fired whenever a structured output tool name was present in context, even if the tool was never actually used, which could mask legitimate `tool_calls` finish reasons in mixed-tool scenarios.
Merged
akshaydeo
added a commit
that referenced
this pull request
May 26, 2026
## ✨ Features
- **Azure v1 API Migration** — Migrated Azure provider to the v1 API:
removed the `api-version` query parameter and the
`/openai/deployments/{model}/...` URL pattern in favor of
`/openai/v1/{operation}`; the `api_version` field has been dropped from
`AzureKeyConfig` (#3661, #3756)
- **EnvVar Support for OTEL & Prometheus Configs** — `CollectorURL`,
`MetricsEndpoint`, headers, push gateway URL, and basic auth credentials
can now be sourced from environment variables (e.g.,
`env.OTEL_COLLECTOR_URL`); added a new `ConfigMarshallerPlugin`
interface that lets plugins control storage/redaction round-trips
(#3651)
- **OTel Extra Header Forwarding** — `x-bf-eh-*` extra headers forwarded
to upstream providers are now also emitted on the request span under
`gen_ai.request.extra_header.*` for end-to-end tracing (#3730)
- **OTel Semantic Conventions** — Aligned OTel attribute keys with the
OpenTelemetry GenAI spec (canonical `gen_ai.*` and new `bifrost.*`
attributes); legacy attributes are retained in parallel to avoid
breaking existing dashboards (#3732)
- **VK Quota with Provider Configs** — `GetVirtualKeyQuotaByValue` and
the `getVirtualKeyQuota` HTTP response now include `provider_configs`
with their budgets and rate limits (#3721)
- **MCP Temp Token Non-Auth Toggle** — Added
`mcp_enable_temp_token_auth` client config flag to gate short-lived MCP
token minting for non-authenticated users (#3720)
- **Responses Stream in JSON Parser** — `jsonparser` plugin now handles
OpenAI Responses API streaming (`ResponsesStreamRequest`) in addition to
chat completions (#3749)
- **Session API Rework** — Logout now calls both the password-based
session logout and OAuth logout endpoints and resets all RTK Query cache
state (#3698)
## 🐞 Fixed
- **Streaming Latency for Observability** — Deferred root span
termination to the trace completer callback for streaming requests so
request latency is no longer inflated by header-flush time (#3762)
- **Stream Cancellation Race** — Set `BifrostContextKeyConnectionClosed`
before closing the stream and short-circuit `idleTimeoutReader.Read`
when the connection is already closed to avoid panics and hangs on
cancellation (#3733)
- **Bedrock Cache Points** — Strip cache points from Bedrock requests
for models that do not support prompt caching (e.g., GLM, Llama) to
avoid Converse API errors (#3754)
- **Bedrock Empty Text Blocks** — Skip empty/nil text blocks during
Bedrock response conversion to avoid invalid messages (#3747)
- **Bedrock Reasoning + Tools** — Preserve reasoning content blocks on
assistant turns that also contain tool calls in the Bedrock chat
converter (#3690)
- **Bedrock Search Content & Video** — Restored search content and video
parts that were being dropped from Bedrock-native passthrough requests
(#3729)
- **Structured Output Stop Reason** — Fixed an incorrect `tool_calls`
finish reason when structured output is combined with extended-thinking
tools (#3685)
- **Gemini Tool Schema Passthrough** — Forward full tool parameter
schemas via `parametersJsonSchema` instead of the lossy `parameters`
form; corrected tool response role to `user`; resolved structured output
+ tools conflict (#3761)
- **Anthropic Stop Reason & Tool Versions** — Normalized stop reason
mapping (`end_turn` to `stop`, `tool_use` to `tool_calls`, `max_tokens`
to `length`) and upgraded `text_editor_20250124`/`str_replace_editor` to
`text_editor_20250728` for computer-use tools (#3761)
- **Azure Endpoint Redaction** — Fixed a panic when
`AzureKeyConfig.Endpoint` is a literal value rather than an env
reference (#3761)
- **Auth Middleware Path Match** — Match temp-token auth middleware
whitelist against the request path only, not the full URI with query
parameters (#3737)
- **Governance Blocked Models UI** — Restored the missing Blocked Models
create/edit UI in the VK provider config sheet (#3750)
- **Logging Plugin Cleanup Drain** — Fixed a shutdown race where
`batchWriter` could drop in-flight log entries; `Cleanup` now drains
both the recovered batch and remaining queue within a 30-second budget
(#3717)
- **Model Rankings Empty Entries** — Excluded entries with empty `model`
values from model rankings matview queries so blank rows no longer
surface in the UI (#3758)
- **User Filter Duplicates** — Recreated `mv_filter_users` matview to
require non-empty `user_name`, eliminating duplicate filter dropdown
entries (#3764)
- **User Filter Display Name** — Use `user_name` instead of `user_id` as
the display label for users in logging filters (#3691)
- **Large Numeric ID Precision** — Preserve large numeric IDs in URL
search params by skipping JSON parsing for plain strings (#3692)
## 🔧 Refactors & Chores
- **Error Propagation for GetAvailable\* APIs** — `GetAvailable*`
methods on `LoggerPlugin`/`LogManager` now return wrapped errors instead
of silently logging and returning empty slices (#3759)
- **Governance Blocklist Matching** — Use `slices.Contains` for VK
blocked-model matching for clearer code with identical semantics (#3727)
- **Exported `ResolvePeriod`** — Renamed `resolvePeriod` to
`ResolvePeriod` so external packages can reuse the period parsing
(#3763)
## 📚 Docs
- **OTEL Env Var Documentation** — Documented `env.VAR_NAME` support for
`collector_url`, `metrics_endpoint`, and headers in OTEL/Prometheus
plugin docs
- **OTEL OSS Features & Examples** — Added OTEL documentation to the OSS
features list with usage examples (#3731)
- **Anthropic Auth Recommendation** — Recommend `ANTHROPIC_AUTH_TOKEN`
over `ANTHROPIC_CUSTOM_HEADERS` for Claude Code authentication (#3686)
This was referenced May 26, 2026
18 tasks
akshaydeo
added a commit
that referenced
this pull request
May 27, 2026
## Summary
This PR releases Bifrost OSS `v1.5.5` and Enterprise `v1.4.4`, bumping all module pins from `v1.5.12`/`v1.3.12` to `v1.5.13`/`v1.3.13` across core, framework, and all plugins. It also hardens the Docker manifest shell scripts, expands CI egress allowlists, and updates documentation to reflect the new SCIM-based user provisioning feature.
## Changes
- **Module version bumps**: All `go.mod`/`go.sum` files updated from `core v1.5.12` → `v1.5.13`, `framework v1.3.12` → `v1.3.13`, and all plugin versions incremented accordingly (`compat`, `governance`, `jsonparser`, `logging`, `maxim`, `mocker`, `otel`, `prompts`, `semanticcache`, `telemetry`).
- **Docker manifest scripts**: Added `#!/usr/bin/env bash` shebang and `set -euo pipefail` to `create-docker-manifest.sh` and `create-docker-manifest-ubi9.sh`; quoted all variable expansions and switched `jq -r` to `jq -er` to fail on null digests.
- **CI egress allowlist**: Added `production.cloudfront.docker.com:443` to Docker-related job allowlists, and added `_https._tcp.dl.google.com:443` and `motd.ubuntu.com:443` to the Ubuntu package job allowlist.
- **Changelog files**: Cleared per-module `changelog.md` files (content moved into the new versioned docs). Added `docs/changelogs/v1.5.5.mdx` and `docs/changelogs/ent-v1.4.4.mdx` with full release notes, and registered both in `docs/docs.json`.
- **Documentation**: Replaced the SSO Integration link with a User Provisioning (SCIM) link in both `README.md` and `transports/README.md`.
- **Enterprise v1.4.4 highlights** (documented): Kafka and Google Cloud Pub/Sub observability sinks, chunked streaming with a 100 MB inter-node message ceiling, BigQuery custom labels via env vars using the new `ConfigMarshallerPlugin` interface, temporary access token expiry extensions, and a multi-node cluster integration harness.
- **OSS v1.5.5 highlights** (documented): Azure v1 API migration, env-var support for OTel/Prometheus configs, OTel extra-header forwarding and semantic-convention alignment, virtual key quota including provider configs, Responses API streaming in `jsonparser`, and a batch of Bedrock, Gemini, Anthropic, Azure, and logging plugin fixes.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [x] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [x] Docs
## How to test
```sh
# Core/Transports
go version
go test ./...
# Verify Docker manifest scripts exit on error
bash -n .github/workflows/scripts/create-docker-manifest.sh
bash -n .github/workflows/scripts/create-docker-manifest-ubi9.sh
```
Validate that the new changelog pages (`changelogs/v1.5.5` and `changelogs/ent-v1.4.4`) render correctly in the docs site.
## Screenshots/Recordings
N/A
## Breaking changes
- [x] Yes
- [ ] No
The Azure provider no longer accepts `api_version` in `AzureKeyConfig` and has migrated to the `/openai/v1/{operation}` URL pattern. See the [v1.4.0 Migration Guide](https://docs.getbifrost.ai/enterprise/migration-guides/v1.4.0) for full details.
## Related issues
#3661, #3756, #3651, #3730, #3732, #3754, #3747, #3690, #3729, #3685, #3733, #3761, #3735, #3721, #3720, #3749, #3698, #3762, #3750, #3727, #3717, #3759, #3758, #3764, #3691, #3692, #3737, #3763
## Security considerations
- The `ConfigMarshallerPlugin` interface redacts secrets (OTel collector URLs, Prometheus push gateway credentials, BigQuery labels) at config storage time and rehydrates them at load time, preventing plaintext secret persistence.
- Docker manifest scripts now use `set -euo pipefail`, preventing silent failures that could result in malformed or missing image manifests being pushed.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
2 tasks
This was referenced Jun 18, 2026
This was referenced Jul 1, 2026
This was referenced Jul 10, 2026
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
### TL;DR Fix incorrect `tool_calls` finish reason being returned when structured output (response format) is used with extended thinking enabled on Anthropic and Bedrock providers. ### What changed? - When extended thinking (reasoning) is active on Anthropic, forcing `tool_choice` to the structured output tool is now skipped, since Anthropic rejects that combination. The tool is still appended and the model may call it voluntarily. - The finish reason override from `tool_calls` → `stop` for structured output is now gated on whether the structured output tool block was **actually consumed into text content**, rather than just whether a structured output tool name was configured. This prevents incorrectly overriding the finish reason in cases where the tool was never invoked. - This fix is applied consistently across the chat completion (non-streaming and streaming), responses (non-streaming and streaming), and Bedrock chat completion paths. - A `UsedStructuredOutputTool` / `consumedStructuredOutput` flag is tracked per-response and per-stream to record when the SO tool block is folded back into text content, and the finish reason override only fires when that flag is set. - For the non-streaming responses path, the override logic inspects the response content blocks directly to confirm no real (non-SO) tool calls are present before remapping the stop reason. ### How to test? 1. Send a chat completion or responses request with a `response_format` (structured output) and extended thinking/reasoning enabled against an Anthropic model. Verify the finish reason is `stop` and no API rejection occurs due to conflicting `tool_choice`. 2. Send a request with `response_format` but **without** extended thinking. Verify the finish reason is still `stop` and the structured output JSON is returned as content. 3. Send a request that uses both real tool calls and structured output simultaneously. Verify the finish reason correctly reflects `tool_calls` for the real tools. 4. Repeat the above for streaming and non-streaming variants, and for the Bedrock provider. ### Why make this change? Anthropic rejects requests that combine extended thinking with a forced `tool_choice`, causing failures when structured output was requested alongside reasoning. Additionally, the previous finish reason override was too broad — it fired whenever a structured output tool name was present in context, even if the tool was never actually used, which could mask legitimate `tool_calls` finish reasons in mixed-tool scenarios.
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
## ✨ Features
- **Azure v1 API Migration** — Migrated Azure provider to the v1 API:
removed the `api-version` query parameter and the
`/openai/deployments/{model}/...` URL pattern in favor of
`/openai/v1/{operation}`; the `api_version` field has been dropped from
`AzureKeyConfig` (maximhq#3661, maximhq#3756)
- **EnvVar Support for OTEL & Prometheus Configs** — `CollectorURL`,
`MetricsEndpoint`, headers, push gateway URL, and basic auth credentials
can now be sourced from environment variables (e.g.,
`env.OTEL_COLLECTOR_URL`); added a new `ConfigMarshallerPlugin`
interface that lets plugins control storage/redaction round-trips
(maximhq#3651)
- **OTel Extra Header Forwarding** — `x-bf-eh-*` extra headers forwarded
to upstream providers are now also emitted on the request span under
`gen_ai.request.extra_header.*` for end-to-end tracing (maximhq#3730)
- **OTel Semantic Conventions** — Aligned OTel attribute keys with the
OpenTelemetry GenAI spec (canonical `gen_ai.*` and new `bifrost.*`
attributes); legacy attributes are retained in parallel to avoid
breaking existing dashboards (maximhq#3732)
- **VK Quota with Provider Configs** — `GetVirtualKeyQuotaByValue` and
the `getVirtualKeyQuota` HTTP response now include `provider_configs`
with their budgets and rate limits (maximhq#3721)
- **MCP Temp Token Non-Auth Toggle** — Added
`mcp_enable_temp_token_auth` client config flag to gate short-lived MCP
token minting for non-authenticated users (maximhq#3720)
- **Responses Stream in JSON Parser** — `jsonparser` plugin now handles
OpenAI Responses API streaming (`ResponsesStreamRequest`) in addition to
chat completions (maximhq#3749)
- **Session API Rework** — Logout now calls both the password-based
session logout and OAuth logout endpoints and resets all RTK Query cache
state (maximhq#3698)
## 🐞 Fixed
- **Streaming Latency for Observability** — Deferred root span
termination to the trace completer callback for streaming requests so
request latency is no longer inflated by header-flush time (maximhq#3762)
- **Stream Cancellation Race** — Set `BifrostContextKeyConnectionClosed`
before closing the stream and short-circuit `idleTimeoutReader.Read`
when the connection is already closed to avoid panics and hangs on
cancellation (maximhq#3733)
- **Bedrock Cache Points** — Strip cache points from Bedrock requests
for models that do not support prompt caching (e.g., GLM, Llama) to
avoid Converse API errors (maximhq#3754)
- **Bedrock Empty Text Blocks** — Skip empty/nil text blocks during
Bedrock response conversion to avoid invalid messages (maximhq#3747)
- **Bedrock Reasoning + Tools** — Preserve reasoning content blocks on
assistant turns that also contain tool calls in the Bedrock chat
converter (maximhq#3690)
- **Bedrock Search Content & Video** — Restored search content and video
parts that were being dropped from Bedrock-native passthrough requests
(maximhq#3729)
- **Structured Output Stop Reason** — Fixed an incorrect `tool_calls`
finish reason when structured output is combined with extended-thinking
tools (maximhq#3685)
- **Gemini Tool Schema Passthrough** — Forward full tool parameter
schemas via `parametersJsonSchema` instead of the lossy `parameters`
form; corrected tool response role to `user`; resolved structured output
+ tools conflict (maximhq#3761)
- **Anthropic Stop Reason & Tool Versions** — Normalized stop reason
mapping (`end_turn` to `stop`, `tool_use` to `tool_calls`, `max_tokens`
to `length`) and upgraded `text_editor_20250124`/`str_replace_editor` to
`text_editor_20250728` for computer-use tools (maximhq#3761)
- **Azure Endpoint Redaction** — Fixed a panic when
`AzureKeyConfig.Endpoint` is a literal value rather than an env
reference (maximhq#3761)
- **Auth Middleware Path Match** — Match temp-token auth middleware
whitelist against the request path only, not the full URI with query
parameters (maximhq#3737)
- **Governance Blocked Models UI** — Restored the missing Blocked Models
create/edit UI in the VK provider config sheet (maximhq#3750)
- **Logging Plugin Cleanup Drain** — Fixed a shutdown race where
`batchWriter` could drop in-flight log entries; `Cleanup` now drains
both the recovered batch and remaining queue within a 30-second budget
(maximhq#3717)
- **Model Rankings Empty Entries** — Excluded entries with empty `model`
values from model rankings matview queries so blank rows no longer
surface in the UI (maximhq#3758)
- **User Filter Duplicates** — Recreated `mv_filter_users` matview to
require non-empty `user_name`, eliminating duplicate filter dropdown
entries (maximhq#3764)
- **User Filter Display Name** — Use `user_name` instead of `user_id` as
the display label for users in logging filters (maximhq#3691)
- **Large Numeric ID Precision** — Preserve large numeric IDs in URL
search params by skipping JSON parsing for plain strings (maximhq#3692)
## 🔧 Refactors & Chores
- **Error Propagation for GetAvailable\* APIs** — `GetAvailable*`
methods on `LoggerPlugin`/`LogManager` now return wrapped errors instead
of silently logging and returning empty slices (maximhq#3759)
- **Governance Blocklist Matching** — Use `slices.Contains` for VK
blocked-model matching for clearer code with identical semantics (maximhq#3727)
- **Exported `ResolvePeriod`** — Renamed `resolvePeriod` to
`ResolvePeriod` so external packages can reuse the period parsing
(maximhq#3763)
## 📚 Docs
- **OTEL Env Var Documentation** — Documented `env.VAR_NAME` support for
`collector_url`, `metrics_endpoint`, and headers in OTEL/Prometheus
plugin docs
- **OTEL OSS Features & Examples** — Added OTEL documentation to the OSS
features list with usage examples (maximhq#3731)
- **Anthropic Auth Recommendation** — Recommend `ANTHROPIC_AUTH_TOKEN`
over `ANTHROPIC_CUSTOM_HEADERS` for Claude Code authentication (maximhq#3686)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Fix incorrect
tool_callsfinish reason being returned when structured output (response format) is used with extended thinking enabled on Anthropic and Bedrock providers.What changed?
tool_choiceto the structured output tool is now skipped, since Anthropic rejects that combination. The tool is still appended and the model may call it voluntarily.tool_calls→stopfor structured output is now gated on whether the structured output tool block was actually consumed into text content, rather than just whether a structured output tool name was configured. This prevents incorrectly overriding the finish reason in cases where the tool was never invoked.UsedStructuredOutputTool/consumedStructuredOutputflag is tracked per-response and per-stream to record when the SO tool block is folded back into text content, and the finish reason override only fires when that flag is set.How to test?
response_format(structured output) and extended thinking/reasoning enabled against an Anthropic model. Verify the finish reason isstopand no API rejection occurs due to conflictingtool_choice.response_formatbut without extended thinking. Verify the finish reason is stillstopand the structured output JSON is returned as content.tool_callsfor the real tools.Why make this change?
Anthropic rejects requests that combine extended thinking with a forced
tool_choice, causing failures when structured output was requested alongside reasoning. Additionally, the previous finish reason override was too broad — it fired whenever a structured output tool name was present in context, even if the tool was never actually used, which could mask legitimatetool_callsfinish reasons in mixed-tool scenarios.