feat: advisor tool compatibility claude - #4357
Merged
Merged
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds full support for Anthropic's advisor server-tool across the neutral Responses schema and Anthropic provider: introduces advisor message/tool types, Anthropic advisor types and JSON wiring, provider gating and beta-header handling, bidirectional streaming and non-stream conversions preserving advisor call/result pairs and tool config, OpenAI filtering, and comprehensive tests. ChangesAnthropic Advisor Tool Implementation
Sequence Diagram(s)sequenceDiagram
participant AnthropicSSE as Anthropic SSE
participant StreamConv as Stream Converter
participant Bifrost as Bifrost Stream
AnthropicSSE->>StreamConv: content_block_start (server_tool_use name="advisor")
StreamConv->>Bifrost: output_item.added (ResponsesMessageTypeAdvisorCall)
AnthropicSSE->>StreamConv: content_block_start/stop (advisor_tool_result with result payload)
StreamConv->>Bifrost: output_item.done (advisor result_type + advisor_text/encrypted/error)
Bifrost->>StreamConv: ResponsesMessageTypeAdvisorCall (completion or result)
StreamConv->>AnthropicSSE: content_block_stop (server_tool_use) + content_block_start/stop (advisor_tool_result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
TejasGhatte
marked this pull request as ready for review
June 13, 2026 05:32
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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
🤖 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/types.go`:
- Around line 1285-1290: The Anthropic advisor model must be non-empty before
sending: in the Responses-to-Anthropic conversion path (advisor_20260301 branch)
check tool.ResponsesToolAdvisor.Model and if it is the empty string either
omit/strip the AnthropicToolAdvisor (do not assign an empty Model) or reject the
tool early; update the conversion logic that assigns AnthropicToolAdvisor.Model
so it only sets the field when non-empty (rather than relying on
ValidateToolsForProvider), ensuring the resulting request JSON never omits a
required-but-empty model field.
In `@core/providers/anthropic/utils_test.go`:
- Around line 850-873: The table-driven test needs Bedrock and Azure coverage
for advisor header non-injection: add two test cases similar to the "Vertex
skips advisor header" entry but with provider set to schemas.Bedrock and
schemas.Azure respectively, using the same req (AnthropicMessageRequest with an
AnthropicTool whose Type is AnthropicToolTypeAdvisor20260301, Name is
AnthropicToolNameAdvisor and AnthropicToolAdvisor.Model "claude-opus-4-8") and
assert unexpectHeaders contains AnthropicAdvisorBetaHeader so
AddMissingBetaHeadersToContext behavior is fully covered.
In `@core/schemas/responses.go`:
- Line 1797: ResponsesTool.MarshalJSON and ResponsesTool.UnmarshalJSON currently
omit the new advisor variant, causing advisor-specific fields to be dropped;
update both methods to detect and handle the ResponsesToolAdvisor variant (type
ResponsesToolAdvisor) so that the advisor discriminator is emitted/consumed and
its fields model, max_uses, max_tokens, and caching are preserved during
marshaling and unmarshaling (add case logic where other tool types are handled,
serialize the advisor struct into the same wrapper used by other tools, and on
UnmarshalJSON detect the advisor discriminator and populate a
ResponsesToolAdvisor instance with those fields).
In `@transports/bifrost-http/integrations/anthropic.go`:
- Around line 89-94: Remove the stale commented-out passthrough block (the lines
that reference soToolName, isClaudeModel, and resp.ExtraFields.RawResponse) from
the core converter path in anthropic.go so the dead code is deleted entirely;
this means deleting the commented lines that begin with "// soToolName, _ :=
ctx.Value(...)" through the closing commented if-block to avoid ambiguity and
accidental reintroduction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb69afc7-6025-4ec1-940e-4cfac47e8a07
📒 Files selected for processing (9)
core/providers/anthropic/advisor_response_test.gocore/providers/anthropic/advisor_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/openai/advisor_filter_test.gocore/schemas/responses.gotransports/bifrost-http/integrations/anthropic.go
Contributor
Confidence Score: 5/5Safe to merge; the advisor tool path is fully isolated behind provider gating and the existing passthrough path for direct Anthropic responses is untouched. The change adds new functionality on a well-gated code path. State fields are properly reset in both the pool-acquire and flush functions. The advisor_tool_result.content bare-object constraint is handled correctly in every code path. Streaming index arithmetic stays correct because getOrCreateOutputIndex increments once per SSE content block, keeping logical output indices aligned with SSE content block indices for the content_block_stop reconstruction. Provider gating rejects advisor on Vertex, Bedrock, and Azure through both the structured and raw-passthrough paths. Test coverage is thorough across all major scenarios. No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "feat: advisor tool compatibility claude" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
TejasGhatte
force-pushed
the
06-13-feat_advisor_tool_compatibility_claude
branch
from
June 13, 2026 13:50
9ea022a to
70762bc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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/utils.go (1)
1247-1258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign raw-path advisor rejection with typed-path provider gating.
ValidateToolsForProviderrejects advisor tools whenfeatures.AdvisorToolis false, butunsupportedRawToolTypesonly blocksadvisor_for Vertex/Bedrock. This leaves Azure raw-body requests on a divergent path where advisor tools can slip through and fail later upstream instead of being rejected consistently at the provider boundary.Suggested fix
var unsupportedRawToolTypes = map[schemas.ModelProvider][]string{ schemas.Vertex: { "web_fetch_", // No web fetch support on Vertex "code_execution", // No code execution on Vertex "advisor_", // Advisor tool is Anthropic API only }, schemas.Bedrock: { "web_search_", // No web search on Bedrock "web_fetch_", // No web fetch on Bedrock "code_execution", // No code execution on Bedrock "advisor_", // Advisor tool is Anthropic API only }, + schemas.Azure: { + "advisor_", // Advisor tool is Anthropic API only + }, }🤖 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/utils.go` around lines 1247 - 1258, The provider-level gating is inconsistent: ValidateToolsForProvider rejects advisor tools when features.AdvisorTool is false but unsupportedRawToolTypes only lists "advisor_" for Vertex/Bedrock, allowing Azure raw-path advisor requests to slip through; update the unsupportedRawToolTypes map in core/providers/anthropic/utils.go to include "advisor_" for the Azure provider (schemas.Azure) — or add a shared entry that applies to all providers that should block advisor tools — so advisor_* raw tool types are rejected at the provider boundary consistent with ValidateToolsForProvider.transports/bifrost-http/integrations/anthropic.go (1)
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove active passthrough logic to match streaming behavior.
The unary
ResponsesResponseConverterstill returns raw responses for Claude models (lines 89-93), but the streaming path (lines 117-134) has had the same logic commented out. This inconsistency contradicts the PR's stated goal of "ensuring all responses use standard conversion logic." If the advisor feature requires conversion to handle advisor message types correctly, leaving this passthrough active means advisor calls in unary responses will bypass the converter and won't be processed.Remove the passthrough block entirely so both unary and streaming responses flow through
anthropic.ToAnthropicResponsesResponse(...).🔧 Proposed fix to remove unary passthrough
ResponsesResponseConverter: func(ctx *schemas.BifrostContext, resp *schemas.BifrostResponsesResponse) (interface{}, error) { - soToolName, _ := ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName).(string) - if soToolName == "" && isClaudeModel(resp.ExtraFields.OriginalModelRequested, resp.ExtraFields.ResolvedModelUsed, string(resp.ExtraFields.Provider)) { - if resp.ExtraFields.RawResponse != nil { - return resp.ExtraFields.RawResponse, nil - } - } return anthropic.ToAnthropicResponsesResponse(ctx, resp), 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 `@transports/bifrost-http/integrations/anthropic.go` around lines 89 - 93, Remove the active passthrough in the unary ResponsesResponseConverter: delete the if-block that checks soToolName == "" && isClaudeModel(...) and returns resp.ExtraFields.RawResponse, and let the code flow into anthropic.ToAnthropicResponsesResponse(...) so unary responses use the same conversion logic as streaming; ensure no other early returns bypass the converter for Claude/advisor responses.
♻️ Duplicate comments (1)
transports/bifrost-http/integrations/anthropic.go (1)
117-134: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove stale commented-out passthrough logic.
The commented-out streaming passthrough block should be deleted entirely to keep the converter path clean and unambiguous.
Suggested cleanup
ResponsesStreamResponseConverter: func(ctx *schemas.BifrostContext, resp *schemas.BifrostResponsesStreamResponse) (string, interface{}, error) { - // soToolName, _ := ctx.Value(schemas.BifrostContextKeyStructuredOutputToolName).(string) - // if soToolName == "" && shouldUsePassthrough(ctx, resp.ExtraFields.Provider, resp.ExtraFields.OriginalModelRequested, resp.ExtraFields.ResolvedModelUsed) { - // // Skip passthrough for ContentPartAdded: it's a synthetic bifrost event whose - // // RawResponse carries the parent content_block_start already emitted by OutputItemAdded. - // // Passing through here would produce a duplicate content_block_start that causes - // // the Anthropic SDK to error and drop all subsequent content_block_delta events. - // if resp.ExtraFields.RawResponse != nil && resp.Type != schemas.ResponsesStreamResponseTypeContentPartAdded { - // raw, ok := resp.ExtraFields.RawResponse.(string) - // if !ok { - // return "", nil, fmt.Errorf("expected RawResponse string, got %T", resp.ExtraFields.RawResponse) - // } - // if t := gjson.Get(raw, "type"); t.Exists() { - // return t.String(), raw, nil - // } - // } - // // Fallback: if RawResponse is not available, use bifrost-to-anthropic conversion - // // instead of silently dropping all events - // } anthropicResponse := anthropic.ToAnthropicResponsesStreamResponse(ctx, resp)🤖 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 `@transports/bifrost-http/integrations/anthropic.go` around lines 117 - 134, Remove the entire stale commented-out passthrough block (the lines referencing soToolName, shouldUsePassthrough(...), the RawResponse handling with fmt.Errorf, the ResponsesStreamResponseTypeContentPartAdded check, and the gjson.Get raw type inspection) so the Anthropic converter path contains no inactive passthrough logic; locate the commented section by searching for shouldUsePassthrough, RawResponse, and ResponsesStreamResponseTypeContentPartAdded and delete that commented block cleanly.
🤖 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/advisor_test.go`:
- Around line 105-121: The test currently checks only type/name/model in
TestAdvisor_ResponsesRoundTrip; extend the assertions to verify that the advisor
configuration fields are round-tripped too by asserting
bifrostTool.ResponsesToolAdvisor.{MaxUses,MaxTokens,Caching} have the expected
values after convertBifrostToolFromAnthropic and then asserting
back.AnthropicToolAdvisor.{MaxUses,MaxTokens,Caching} match those same expected
values after convertBifrostToolToAnthropic; use the existing identifiers
bifrostTool, convertBifrostToolToAnthropic and back to locate where to add these
checks and fail the test if any field is missing or changed.
In `@core/providers/anthropic/responses.go`:
- Around line 1693-1705: The streaming code currently sets the advisor tool-use
ID from bifrostResp.Item.ID; change those branches to use
ResponsesToolMessage.CallID (e.g., use bifrostResp.CallID or
bifrostResp.ResponsesToolMessage.CallID as appropriate) when populating
AnthropicContentBlock.ID and the advisor_tool_result.tool_use_id so the
streaming path matches the non-stream helper that treats CallID as
authoritative; update both the Advisor "server_tool_use" content-block branch
(where streamResp.ContentBlock.ID is set) and the other streaming branch that
emits advisor_tool_result.tool_use_id to read CallID instead of Item.ID.
- Around line 37-40: The pooled struct fields AdvisorToolID, AdvisorOutputIndex
and AdvisorResult on AnthropicResponsesStreamState are not being cleared on
reuse; update acquireAnthropicResponsesStreamState() to initialize those fields
to nil/zero when handing out a state and modify
(*AnthropicResponsesStreamState).flush() to explicitly reset AdvisorToolID =
nil, AdvisorOutputIndex = nil (or 0 with pointer nil) and AdvisorResult = nil
before returning the object to the pool so no previous advisor data can leak
into the next request.
- Around line 1180-1214: The branch that emits an advisor_call done (inside the
advisor result handling where state.AdvisorResult/state.AdvisorToolID are
checked) currently returns the completed ResponsesMessage without persisting it;
add the same persistence used for other completed items by appending the
constructed item to state.OutputItems (use the existing outputIdx as the
index/key consistent with other codepaths) before clearing
state.AdvisorToolID/AdvisorResult and returning the output_item.done so
message_stop will include this advisor_call in response.Output.
In `@core/providers/openai/advisor_filter_test.go`:
- Line 15: Remove the unused local variable declaration model :=
"claude-opus-4-8" from the test in advisor_filter_test.go (or, if the intent was
to use it, replace its literal usage with this variable); ensure no other
references to model remain so the test compiles without the unused variable.
In `@core/schemas/serialization_test.go`:
- Around line 1245-1247: The table-driven tests that include the rows named
"advisor canonical" and "advisor_20260301" currently only assert tool.Type
(ResponsesToolTypeAdvisor); update those cases to also assert that the
unmarshaled tool.ResponsesToolAdvisor is non-nil and that its representative
field (e.g., Model or model) equals the expected value ("claude-opus-4-8").
Concretely, after checking tool.Type == ResponsesToolTypeAdvisor, add assertions
like tool.ResponsesToolAdvisor != nil and tool.ResponsesToolAdvisor.Model ==
"claude-opus-4-8" for both the canonical and versioned inputs so the advisor
payload branch is validated, not just the normalized type.
---
Outside diff comments:
In `@core/providers/anthropic/utils.go`:
- Around line 1247-1258: The provider-level gating is inconsistent:
ValidateToolsForProvider rejects advisor tools when features.AdvisorTool is
false but unsupportedRawToolTypes only lists "advisor_" for Vertex/Bedrock,
allowing Azure raw-path advisor requests to slip through; update the
unsupportedRawToolTypes map in core/providers/anthropic/utils.go to include
"advisor_" for the Azure provider (schemas.Azure) — or add a shared entry that
applies to all providers that should block advisor tools — so advisor_* raw tool
types are rejected at the provider boundary consistent with
ValidateToolsForProvider.
In `@transports/bifrost-http/integrations/anthropic.go`:
- Around line 89-93: Remove the active passthrough in the unary
ResponsesResponseConverter: delete the if-block that checks soToolName == "" &&
isClaudeModel(...) and returns resp.ExtraFields.RawResponse, and let the code
flow into anthropic.ToAnthropicResponsesResponse(...) so unary responses use the
same conversion logic as streaming; ensure no other early returns bypass the
converter for Claude/advisor responses.
---
Duplicate comments:
In `@transports/bifrost-http/integrations/anthropic.go`:
- Around line 117-134: Remove the entire stale commented-out passthrough block
(the lines referencing soToolName, shouldUsePassthrough(...), the RawResponse
handling with fmt.Errorf, the ResponsesStreamResponseTypeContentPartAdded check,
and the gjson.Get raw type inspection) so the Anthropic converter path contains
no inactive passthrough logic; locate the commented section by searching for
shouldUsePassthrough, RawResponse, and
ResponsesStreamResponseTypeContentPartAdded and delete that commented block
cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8d4c2e16-d0a7-4d54-a266-34173aa63d04
📒 Files selected for processing (9)
core/providers/anthropic/advisor_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/openai/advisor_filter_test.gocore/schemas/responses.gocore/schemas/serialization_test.gotransports/bifrost-http/integrations/anthropic.go
TejasGhatte
force-pushed
the
06-13-feat_advisor_tool_compatibility_claude
branch
from
June 13, 2026 14:34
70762bc to
43f63d8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/schemas/serialization_test.go (1)
744-783: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a round-trip case for
advisor_call.This file now exercises advisor tool serialization, but the new
ResponsesMessageTypeAdvisorCall/ResponsesAdvisorCallsurface incore/schemas/responses.gostill has no direct marshal/unmarshal coverage. Add a case that asserts the embedded advisor-call payload and a representative field likeadvisor_stop_reasonsurvive the JSON boundary.As per coding guidelines,
**/*.go: apply standard Go review practices, including table-driven coverage for behavior changes.🤖 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/schemas/serialization_test.go` around lines 744 - 783, Add a round-trip test case for ResponsesMessageTypeAdvisorCall by extending the inputs table in TestResponsesTool_MarshalJSON_RoundTrip to include an advisor_call JSON fixture that embeds a ResponsesAdvisorCall payload (with an embedded advisor tool object and a representative field such as advisor_stop_reason). The new input should mirror the existing advisor tool example structure (model/max_uses/max_tokens/caching) wrapped under the "advisor_call" message type and include "advisor_stop_reason"; then run the same unmarshal→marshal→unmarshal assertions (the same Unmarshal/Marshal calls and equality checks) so the ResponsesAdvisorCall and its advisor payload survive the JSON boundary.Source: Coding guidelines
🤖 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/advisor_test.go`:
- Around line 308-339: The test currently only checks presence flags and reads
advisor text from ContentBlocks[0], which misses ordering regressions and the
alternate ContentStr representation; update the loop over emitted to (1) record
the sequence of content block types (using
AnthropicContentBlockTypeServerToolUse,
AnthropicContentBlockTypeAdvisorToolResult, AnthropicContentBlockTypeText) and
assert the expected order (at minimum that advisor_tool_result appears before
plain text and that server_tool_use precedes advisor_tool_result), and (2) when
extracting advisor text (from the code that sets advisorText), accept either
representation by checking ContentStr first and falling back to
ContentBlocks[0].Text (and if using ContentBlocks, search all blocks not just
index 0) so the test is robust to reordering and to ResponsesMessageContent
emitting ContentStr instead of ContentBlocks; update references to
sawAdvisorServerToolUse, sawAdvisorResult, sawText, advisorText and the emitted
scan logic accordingly.
In `@core/providers/anthropic/responses.go`:
- Around line 3684-3709: The grouped reverse conversion
convertAnthropicContentBlocksToResponsesMessagesGrouped currently treats
advisor-type blocks as generic tool calls and loses advisor-specific fields;
update convertAnthropicContentBlocksToResponsesMessagesGrouped to detect the
advisor-specific AnthropicContentBlock shape (e.g., block type or presence of
advisor result fields created by convertBifrostAdvisorCallToAnthropicBlocks),
and when found emit a ResponsesMessage with type
schemas.ResponsesMessageTypeAdvisorCall preserving all advisor-specific fields
(server_tool_use/advisor_tool_result) and tool IDs rather than mapping them to a
generic tool-call message; ensure the mapping logic mirrors the forward helper
convertBifrostAdvisorCallToAnthropicBlocks so round-trip fidelity is maintained
and include preservation of any ID bookkeeping used by
currentToolCallIDs/pendingToolCalls.
- Around line 4485-4498: The code appends an advisor_call for blocks with Name
== AnthropicToolNameAdvisor without verifying the block is an output message;
update the branch that creates bifrostMsg (using AnthropicToolNameAdvisor,
ResponsesMessage, ResponsesToolMessage, bifrostMessages, block) to also require
the same isOutputMessage check used by the web_search/web_fetch branches (i.e.,
only construct/append the advisor_call when isOutputMessage is true), so
non-output server_tool_use blocks are not converted into assistant advisor
calls.
In `@core/providers/anthropic/types.go`:
- Around line 1346-1354: In MarshalJSON in types.go (the method that constructs
the tool JSON), preserve explicitly provided empty domain arrays by changing the
conditions that inject allowed_domains and blocked_domains from "if len(allowed)
> 0" / "if len(blocked) > 0" to checks that the slices are non-nil (e.g., "if
allowed != nil" and "if blocked != nil"); this ensures an explicitly set empty
slice ([]), not just non-empty lists, will be serialized into the JSON rather
than dropped.
In `@core/schemas/responses.go`:
- Around line 2559-2565: ResponsesToolAdvisor.Model is marked required but its
JSON tag uses omitempty which allows an empty model to be omitted; remove
omitempty from the struct tag on the Model field in the ResponsesToolAdvisor
struct (change `json:"model,omitempty"` to `json:"model"`) so an empty/missing
model serializes as null/empty and surfaces validation errors upstream;
optionally run any existing schema/validation tests for ResponsesToolAdvisor to
ensure consumers react to the stricter serialization.
---
Outside diff comments:
In `@core/schemas/serialization_test.go`:
- Around line 744-783: Add a round-trip test case for
ResponsesMessageTypeAdvisorCall by extending the inputs table in
TestResponsesTool_MarshalJSON_RoundTrip to include an advisor_call JSON fixture
that embeds a ResponsesAdvisorCall payload (with an embedded advisor tool object
and a representative field such as advisor_stop_reason). The new input should
mirror the existing advisor tool example structure
(model/max_uses/max_tokens/caching) wrapped under the "advisor_call" message
type and include "advisor_stop_reason"; then run the same
unmarshal→marshal→unmarshal assertions (the same Unmarshal/Marshal calls and
equality checks) so the ResponsesAdvisorCall and its advisor payload survive the
JSON boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7846435-fffe-4722-b98b-527fd7cb6583
📒 Files selected for processing (9)
core/providers/anthropic/advisor_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/openai/advisor_filter_test.gocore/schemas/responses.gocore/schemas/serialization_test.gotransports/bifrost-http/integrations/anthropic.go
TejasGhatte
force-pushed
the
06-13-feat_advisor_tool_compatibility_claude
branch
from
June 14, 2026 11:33
43f63d8 to
2a6f59b
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 (2)
core/providers/anthropic/types.go (2)
852-877:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve bare-object
contenton decode.The new
ContentObjarm is only honored on marshal. Lines 869-874 still decode a single block object intoContentBlocks, so{"content": {...}}round-trips back out as{"content": [{...}]}. That changes the wire shape foradvisor_tool_result.contentand defeats the new single-object support.Suggested fix
func (mc *AnthropicContent) UnmarshalJSON(data []byte) error { + mc.ContentStr = nil + mc.ContentBlocks = nil + mc.ContentObj = nil + // First, try to unmarshal as a direct string var stringContent string if err := sonic.Unmarshal(data, &stringContent); err == nil { mc.ContentStr = &stringContent return nil @@ // Try to unmarshal as a single ContentBlock object (e.g., web_search_tool_result_error) - // If successful, wrap it in an array + // Preserve the bare-object form so marshal/unmarshal round-trips are stable. var singleBlock AnthropicContentBlock if err := sonic.Unmarshal(data, &singleBlock); err == nil && singleBlock.Type != "" { - mc.ContentBlocks = []AnthropicContentBlock{singleBlock} + mc.ContentObj = &singleBlock return 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/anthropic/types.go` around lines 852 - 877, The UnmarshalJSON method in AnthropicContent currently wraps a single ContentBlock object into an array and assigns it to ContentBlocks (lines 869-874), which causes round-trip issues where bare objects become arrays. When unmarshalling a single block object (in the third condition where singleBlock.Type is not empty), assign it directly to the ContentObj field instead of wrapping it in an array and assigning to ContentBlocks. This preserves the bare object format and ensures the wire shape remains unchanged during round-trip serialization and deserialization.
1383-1412:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset stale union arms before unmarshaling a tool.
sonic.Unmarshalleaves absent fields untouched. If the sameAnthropicToolinstance is reused, an oldMCPToolset,AnthropicToolWebSearch, orAnthropicToolWebFetcharm can survive into the next decode. The staleMCPToolsetis especially bad becauseMarshalJSONwill then serialize it instead of the newly decoded tool.Suggested fix
func (t *AnthropicTool) UnmarshalJSON(data []byte) error { + next := AnthropicTool{} + // Peek at the type field to detect mcp_toolset entries var peek struct { Type string `json:"type"` } if err := sonic.Unmarshal(data, &peek); err == nil && peek.Type == "mcp_toolset" { var toolset AnthropicMCPToolsetTool if err := sonic.Unmarshal(data, &toolset); err != nil { return err } - t.MCPToolset = &toolset + next.MCPToolset = &toolset + *t = next return nil } // Default unmarshaling for all other tool types type Alias AnthropicTool - if err := sonic.Unmarshal(data, (*Alias)(t)); err != nil { + if err := sonic.Unmarshal(data, (*Alias)(&next)); err != nil { return err } @@ - t.applySharedServerToolFields(shared.MaxUses, shared.AllowedDomains, shared.BlockedDomains) + next.applySharedServerToolFields(shared.MaxUses, shared.AllowedDomains, shared.BlockedDomains) + *t = next return 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/anthropic/types.go` around lines 1383 - 1412, The UnmarshalJSON method for AnthropicTool can leave stale union arm data (MCPToolset, AnthropicToolWebSearch, AnthropicToolWebFetch) from previous decodes because sonic.Unmarshal does not clear absent fields. Add code at the very beginning of the UnmarshalJSON method to reset all union arms (MCPToolset, AnthropicToolWebSearch, AnthropicToolWebFetch) to nil before proceeding with the unmarshaling logic. This ensures that only the newly decoded tool data is present and prevents stale arms from being serialized by MarshalJSON.
♻️ Duplicate comments (1)
core/providers/anthropic/responses.go (1)
3684-3709:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMirror advisor semantics in the grouped Anthropic→Bifrost converter.
This adds the forward
ResponsesMessageTypeAdvisorCallpath, but the grouped reverse path used from Line 3165 still routesserver_tool_usegenerically inconvertAnthropicContentBlocksToResponsesMessagesGroupedand never reattachesadvisor_tool_result. In grouped flows, advisor calls still degrade to generic function calls and loseResponsesAdvisorCallpayloads. As per coding guidelines, SDK integration layers must stay drop-in compatible with OpenAI, Anthropic, Bedrock, Google GenAI, LangChain, LiteLLM, and PydanticAI request/response shapes where relevant.🤖 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 3684 - 3709, The forward path correctly handles advisor calls via ResponsesMessageTypeAdvisorCall by converting them to Anthropic blocks, but the reverse path in convertAnthropicContentBlocksToResponsesMessagesGrouped (around line 3165) treats all server_tool_use blocks generically without reattaching advisor_tool_result, causing advisor calls to degrade to generic function calls. In the grouped converter, when processing server_tool_use blocks, check for the presence of advisor_tool_result; if present, construct a ResponsesAdvisorCall message structure to preserve advisor semantics instead of routing it as a generic function call, ensuring the forward and reverse paths maintain consistent advisor call handling.Source: Coding guidelines
🤖 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/advisor_test.go`:
- Around line 309-344: Modify the test loop to record the sequence of block
types as they are encountered in the emitted events, rather than just setting
boolean flags. After the loop, add an assertion that verifies the blocks appear
in the expected order (server_tool_use before advisor_tool_result before text).
Additionally, make the advisor text extraction (where advisorText is assigned
from e.ContentBlock.Content.ContentObj.Text) more robust by adding fallback
logic to check alternative content paths if the primary ContentObj.Text path is
nil, ensuring the test doesn't fail due to schema representation changes.
In `@core/providers/anthropic/responses.go`:
- Around line 2177-2241: The code in this branch synthesizes an
advisor_tool_result block at serverIdx + 1, but when subsequent bifrostResp
items arrive with their original OutputIndex or ContentIndex values, they
collide with the inserted synthetic block, breaking Anthropic content ordering.
Implement per-request index translation that tracks when an extra block has been
injected (via the advisor call handler) and shifts all subsequent real events'
indexes by +1 to maintain unique indexes. This requires either a request-scoped
counter tracking inserted blocks or a translation map that offsets OutputIndex
and ContentIndex values from bifrostResp after the advisor call block injection
point.
---
Outside diff comments:
In `@core/providers/anthropic/types.go`:
- Around line 852-877: The UnmarshalJSON method in AnthropicContent currently
wraps a single ContentBlock object into an array and assigns it to ContentBlocks
(lines 869-874), which causes round-trip issues where bare objects become
arrays. When unmarshalling a single block object (in the third condition where
singleBlock.Type is not empty), assign it directly to the ContentObj field
instead of wrapping it in an array and assigning to ContentBlocks. This
preserves the bare object format and ensures the wire shape remains unchanged
during round-trip serialization and deserialization.
- Around line 1383-1412: The UnmarshalJSON method for AnthropicTool can leave
stale union arm data (MCPToolset, AnthropicToolWebSearch, AnthropicToolWebFetch)
from previous decodes because sonic.Unmarshal does not clear absent fields. Add
code at the very beginning of the UnmarshalJSON method to reset all union arms
(MCPToolset, AnthropicToolWebSearch, AnthropicToolWebFetch) to nil before
proceeding with the unmarshaling logic. This ensures that only the newly decoded
tool data is present and prevents stale arms from being serialized by
MarshalJSON.
---
Duplicate comments:
In `@core/providers/anthropic/responses.go`:
- Around line 3684-3709: The forward path correctly handles advisor calls via
ResponsesMessageTypeAdvisorCall by converting them to Anthropic blocks, but the
reverse path in convertAnthropicContentBlocksToResponsesMessagesGrouped (around
line 3165) treats all server_tool_use blocks generically without reattaching
advisor_tool_result, causing advisor calls to degrade to generic function calls.
In the grouped converter, when processing server_tool_use blocks, check for the
presence of advisor_tool_result; if present, construct a ResponsesAdvisorCall
message structure to preserve advisor semantics instead of routing it as a
generic function call, ensuring the forward and reverse paths maintain
consistent advisor call handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9068fac7-d448-4e0d-98a8-870bdeddd007
📒 Files selected for processing (9)
core/providers/anthropic/advisor_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/openai/advisor_filter_test.gocore/schemas/responses.gocore/schemas/serialization_test.gotransports/bifrost-http/integrations/anthropic.go
This was referenced Jun 14, 2026
TejasGhatte
force-pushed
the
06-13-feat_advisor_tool_compatibility_claude
branch
from
June 15, 2026 04:46
2a6f59b to
4ac7964
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/types.go (1)
833-843:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
ContentObjin the content-union guard.
ContentObjis a third mutually exclusive arm, but the guard only checksContentStrvsContentBlocks. A mixed state likeContentObj + ContentBlockssilently drops the blocks during marshal.Suggested fix
func (mc AnthropicContent) MarshalJSON() ([]byte, error) { // Validation: ensure only one field is set at a time - if mc.ContentStr != nil && mc.ContentBlocks != nil { - return nil, fmt.Errorf("both ContentStr and ContentBlocks are set; only one should be non-nil") + active := 0 + if mc.ContentStr != nil { + active++ + } + if mc.ContentBlocks != nil { + active++ + } + if mc.ContentObj != nil { + active++ + } + if active > 1 { + return nil, fmt.Errorf("only one of ContentStr, ContentBlocks, or ContentObj should be non-nil") } if mc.ContentStr != nil { return providerUtils.MarshalSorted(*mc.ContentStr) }🤖 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/types.go` around lines 833 - 843, The validation guard clause in the MessageContent type only checks that ContentStr and ContentBlocks are mutually exclusive, but neglects ContentObj which is also a mutually exclusive field. Update the guard clause to validate that only one of the three fields—ContentStr, ContentBlocks, and ContentObj—is set at a time. Ensure the error check prevents any combination of these three fields from being simultaneously non-nil, so that invalid states like ContentObj + ContentBlocks cannot silently drop data during marshaling.
🤖 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 1189-1196: The advisor result reading logic currently only
extracts data from ContentBlocks[0] in the state.AdvisorResult.Content
structure, but the forward path writes advisor results to
AnthropicContent.ContentObj. Modify the code to also check and read advisor
results from ContentObj in addition to ContentBlocks, ensuring that the Text,
EncryptedContent, ErrorCode, and StopReason fields are properly extracted from
either ContentObj or the first ContentBlock. This issue appears at multiple
locations (line 1189-1196, 2219-2227, and 4514-4523), and the same fix pattern
should be applied at all three sites to handle object-form advisor results that
are currently losing their fields.
---
Outside diff comments:
In `@core/providers/anthropic/types.go`:
- Around line 833-843: The validation guard clause in the MessageContent type
only checks that ContentStr and ContentBlocks are mutually exclusive, but
neglects ContentObj which is also a mutually exclusive field. Update the guard
clause to validate that only one of the three fields—ContentStr, ContentBlocks,
and ContentObj—is set at a time. Ensure the error check prevents any combination
of these three fields from being simultaneously non-nil, so that invalid states
like ContentObj + ContentBlocks cannot silently drop data during marshaling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a205eb2a-cd33-4841-90fa-e436cfbc79fe
📒 Files selected for processing (9)
core/providers/anthropic/advisor_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/openai/advisor_filter_test.gocore/schemas/responses.gocore/schemas/serialization_test.gotransports/bifrost-http/integrations/anthropic.go
TejasGhatte
force-pushed
the
06-13-feat_advisor_tool_compatibility_claude
branch
from
June 15, 2026 05:10
4ac7964 to
5cca2ec
Compare
Contributor
Merge activity
|
akshaydeo
pushed a commit
that referenced
this pull request
Jun 15, 2026
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes #123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added end-to-end Anthropic “advisor” tool support across streaming and non-streaming conversions, including call/result pairing, preserved content ordering, and full tool configuration mapping. * **Behavior** * Auto-injects the advisor beta header for Anthropic requests only; omits it for other providers. * Enforces provider/tool gating and rejects unsupported raw advisor tools. * **Tests** * Expanded unit and regression coverage for advisor JSON round-trips, conversion correctness, streaming preservation, gating, and beta-header behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jun 16, 2026
Merged
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added end-to-end Anthropic “advisor” tool support across streaming and non-streaming conversions, including call/result pairing, preserved content ordering, and full tool configuration mapping. * **Behavior** * Auto-injects the advisor beta header for Anthropic requests only; omits it for other providers. * Enforces provider/tool gating and rejects unsupported raw advisor tools. * **Tests** * Expanded unit and regression coverage for advisor JSON round-trips, conversion correctness, streaming preservation, gating, and beta-header behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added end-to-end Anthropic “advisor” tool support across streaming and non-streaming conversions, including call/result pairing, preserved content ordering, and full tool configuration mapping. * **Behavior** * Auto-injects the advisor beta header for Anthropic requests only; omits it for other providers. * Enforces provider/tool gating and rejects unsupported raw advisor tools. * **Tests** * Expanded unit and regression coverage for advisor JSON round-trips, conversion correctness, streaming preservation, gating, and beta-header behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Behavior
Tests