openai -> anthropic -> openai thinking fixes#2184
Merged
akshaydeo merged 3 commits intoMar 20, 2026
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds encoding/decoding of per-segment content-block boundaries between Bifrost reasoning details and provider content; updates Anthropic and Bedrock conversions to emit and consume those boundaries to collapse or reconstruct multi-part assistant content where applicable. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Anthropic
participant Bifrost
participant ReasoningMeta
Client->>Anthropic: Send assistant content blocks (thinking + text blocks)
Anthropic->>Bifrost: ToBifrostChatResponse (contentBlocks + reasoningDetails)
Bifrost->>Bifrost: Detect all contentBlocks are Text
Bifrost->>ReasoningMeta: Marshal per-segment contentBlockMeta (type, length)
Bifrost->>Bifrost: Concatenate thinking/texts into contentStr (joined by "\n\n")
Bifrost->>Client: Return collapsed contentStr and ReasoningDetails(TypeContentBlocks)
sequenceDiagram
participant Client
participant Bifrost
participant Anthropic
participant ReasoningMeta
Client->>Bifrost: Send combined ContentStr + ReasoningDetails(TypeContentBlocks)
Bifrost->>ReasoningMeta: Extract and unmarshal contentBlockMeta list
Bifrost->>Anthropic: ToAnthropicChatRequest with signatures and blockMeta
Anthropic->>Anthropic: Slice ContentStr using recorded lengths and separators
alt lengths match
Anthropic->>Client: Reconstructed sequence: thinking blocks + text blocks
else mismatch
Anthropic->>Client: Emit thinking signature-only + single text block with full ContentStr
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
18 tasks
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
11 tasks
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
akshaydeo
force-pushed
the
03-20-openai_-_anthropic_-_openai_thinking_fixes
branch
from
d278115 to
3553832
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/anthropic/chat.go`:
- Around line 591-597: The loop over reasoningDetails in ToAnthropicChatRequest
currently only appends to blockMeta when rd.Text != nil, which loses
signature-only thinking blocks and misaligns later signatures; update the loop
that builds parts and blockMeta so that for every rd with Type ==
schemas.BifrostReasoningDetailsTypeText you always append a metadata entry (even
when rd.Text is nil) — use an explicit nullability marker or record an empty
string with length 0 in contentBlockMeta (or add a flag like HasText:false) so
the later reconstruction code can preserve and map signatures correctly; ensure
ToAnthropicChatRequest and any rebuild logic consume that metadata field to
avoid skipping or shifting blocks.
- Around line 365-398: The current reconstruction trusts expectedLen ==
len(combined) but doesn't validate individual block lengths or separators,
allowing negative/overflow bm.L or missing "\n\n" to cause a panic or corrupt
blocks; change the guard to validate each blockMeta entry before slicing by
ensuring every bm.L is positive, that pos+bm.L does not exceed len(combined) for
each block, and that for every inter-block boundary the two-byte separator
equals "\n\n" (or accept no separator for single blocks); if any check fails,
skip the reconstruction and keep the fallback content instead of slicing; update
the logic around expectedLen, blockMeta, combined, pos, and bm.L and ensure
sigIdx handling for AnthropicContentBlock Type Thinking remains bounded.
- Around line 342-345: In ToAnthropicChatRequest, replace the use of
encoding/json with sonic for hot-path JSON work: where rd.Type ==
schemas.BifrostReasoningDetailsTypeContentBlocks currently calls
json.Unmarshal([]byte(*rd.Text), &blockMeta), switch that to
sonic.Unmarshal([]byte(*rd.Text), &blockMeta); likewise replace the json.Marshal
call later in the same function (used to serialize the reasoning/details payload
around the previously-mentioned line ~612) with sonic.Marshal. Import and use
the sonic package and keep the same target variables (blockMeta and the existing
payload variable) so behavior is identical but uses sonic for performance.
In `@core/providers/bedrock/chat.go`:
- Around line 196-227: The current two-pass logic (appending all
reasoningDetails first then all contentBlocks) reorders segments; instead
preserve provider order by building the flattened parts and blockMeta in the
same single-pass that iterates the original sequence of segments (or by
persisting an ordered segment slice when you first parse the Bedrock response)
so the appended contentBlocks metadata matches the actual content sequence.
Replace the separate loops that build parts/blockMeta with a single ordered
iteration that, for each original item, appends the text to parts and a
contentBlockMeta (T: "thinking" or "text", L: len(...)) to blockMeta so
contentStr and the schemas.ChatReasoningDetails
(BifrostReasoningDetailsTypeContentBlocks) metadata remain lossless and in
provider order.
- Line 5: Replace the use of encoding/json's json.Marshal in the response
conversion path with sonic.Marshal: remove "encoding/json" from imports, add the
sonic package import (github.com/bytedance/sonic), and change the json.Marshal
call at the response conversion (around line 221) to sonic.Marshal; ensure the
surrounding error handling and variable names (the same as used in the function
that converts Bedrock responses) remain 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: 8cec0733-a97f-47e3-a0d4-b6cb7a6b7b0d
📒 Files selected for processing (5)
core/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/bedrock/chat.gocore/schemas/chatcompletions.gotests/integrations/python/tests/test_openai.py
✅ Files skipped from review due to trivial changes (1)
- core/providers/anthropic/chat_test.go
akshaydeo
force-pushed
the
03-20-openai_-_anthropic_-_openai_thinking_fixes
branch
from
3553832 to
f6e7f96
Compare
akshaydeo
force-pushed
the
03-20-anthropic_header_selection_across_providers
branch
from
df9a5e1 to
2e01dbc
Compare
akshaydeo
force-pushed
the
03-20-openai_-_anthropic_-_openai_thinking_fixes
branch
from
f6e7f96 to
bb28dfb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/providers/bedrock/chat.go (1)
196-214:⚠️ Potential issue | 🟠 MajorPreserve the original Bedrock block order when flattening.
partsandblockMetaare rebuilt as “all thinking, then all text”, even thoughresponse.Output.Message.Contentwas parsed in provider order above. If Bedrock ever returns interleaved reasoning/text blocks, this metadata will encode the reordered sequence, andcore/providers/anthropic/chat.goat Lines 361-405 will faithfully reconstruct that wrong order instead of falling back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/chat.go` around lines 196 - 214, The code currently appends all reasoningDetails then all contentBlocks, reordering blocks; instead iterate the original Bedrock block sequence (response.Output.Message.Content or the variable holding parsed blocks) and for each block append to parts and blockMeta in that exact order, mapping reasoning blocks to contentBlockMeta{T:"thinking", L:...} and text blocks to contentBlockMeta{T:"text", L:...}; use the existing contentBlocks, reasoningDetails and contentBlockMeta types and the schemas.BifrostReasoningDetailsTypeText check to decide which source/text to use so the flattened parts and blockMeta preserve the original provider order.core/providers/anthropic/chat.go (1)
361-405:⚠️ Potential issue | 🔴 CriticalTreat invalid
bifrost.content_blocksmetadata as a full fallback.
expectedLen == len(combined)is not a safe guard. A crafted payload can still use negative/oversized lengths or bad separators and panic at Line 379, and the fallback branch leaves the pre-appended thinking blocks incontent, so stale signatures/thinking are sent back alongside the modified text. Validate each entry (bm.T,bm.L, separator) before slicing, and clearcontentbefore any fallback to the single text block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/chat.go` around lines 361 - 405, The code currently assumes blockMeta entries are sane and slices combined blindly, which can panic and also leaves pre-appended thinking blocks in content on fallback; update the reconstruction in the handler that reads msg.Content.ContentStr/combined to fully validate each bm before slicing: ensure bm.L is a positive integer, bm.T is one of the expected values (e.g., "thinking" or text), check that pos+bm.L <= len(combined) for each block and that separators ("\n\n") exist between blocks at the expected positions, and only append reconstructed AnthropicContentBlock entries (AnthropicContentBlockTypeThinking/AnthropicContentBlockTypeText) after all checks pass; clear content (content = content[:0]) before attempting reconstruction and also clear it immediately before taking the single-text fallback so no stale thinking/signature blocks remain; handle signature indexing safely (only consume signatures when bm.T == "thinking" and guard sigIdx < len(signatures)), and if any validation fails, skip reconstruction and use the cleared-content single AnthropicContentBlock fallback from msg.Content.ContentStr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integrations/python/tests/test_openai.py`:
- Around line 2145-2148: The except block in the test currently only skips when
the error message contains "does not support" or "not available"; broaden this
check to also detect "not supported" (and any similar common phrasing used for
chat reasoning rejection) so the test will skip the same providers as
test_38_responses_reasoning; update the conditional that examines error_str
(from except Exception as e) to include "not supported" (case-insensitive)
alongside the existing checks and keep the pytest.skip call for consistency.
---
Duplicate comments:
In `@core/providers/anthropic/chat.go`:
- Around line 361-405: The code currently assumes blockMeta entries are sane and
slices combined blindly, which can panic and also leaves pre-appended thinking
blocks in content on fallback; update the reconstruction in the handler that
reads msg.Content.ContentStr/combined to fully validate each bm before slicing:
ensure bm.L is a positive integer, bm.T is one of the expected values (e.g.,
"thinking" or text), check that pos+bm.L <= len(combined) for each block and
that separators ("\n\n") exist between blocks at the expected positions, and
only append reconstructed AnthropicContentBlock entries
(AnthropicContentBlockTypeThinking/AnthropicContentBlockTypeText) after all
checks pass; clear content (content = content[:0]) before attempting
reconstruction and also clear it immediately before taking the single-text
fallback so no stale thinking/signature blocks remain; handle signature indexing
safely (only consume signatures when bm.T == "thinking" and guard sigIdx <
len(signatures)), and if any validation fails, skip reconstruction and use the
cleared-content single AnthropicContentBlock fallback from
msg.Content.ContentStr.
In `@core/providers/bedrock/chat.go`:
- Around line 196-214: The code currently appends all reasoningDetails then all
contentBlocks, reordering blocks; instead iterate the original Bedrock block
sequence (response.Output.Message.Content or the variable holding parsed blocks)
and for each block append to parts and blockMeta in that exact order, mapping
reasoning blocks to contentBlockMeta{T:"thinking", L:...} and text blocks to
contentBlockMeta{T:"text", L:...}; use the existing contentBlocks,
reasoningDetails and contentBlockMeta types and the
schemas.BifrostReasoningDetailsTypeText check to decide which source/text to use
so the flattened parts and blockMeta preserve the original provider order.
🪄 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: 90ce7a9a-47f0-441f-8ebf-f3d0b5c71268
📒 Files selected for processing (5)
core/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/bedrock/chat.gocore/schemas/chatcompletions.gotests/integrations/python/tests/test_openai.py
✅ Files skipped from review due to trivial changes (1)
- core/providers/anthropic/chat_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/chatcompletions.go
akshaydeo
force-pushed
the
03-20-openai_-_anthropic_-_openai_thinking_fixes
branch
from
bb28dfb to
c9f2418
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/bedrock/chat.go`:
- Around line 219-227: The code currently appends a synthetic
bifrost.content_blocks entry (via blockMeta ->
schemas.BifrostReasoningDetailsTypeContentBlocks) which makes reasoningDetails
non-empty and later causes assistantMessage.Reasoning to be set to an empty
string; keep the metadata append (the blockMeta -> reasoningDetails logic) but
change the assignment to assistantMessage.Reasoning to only occur when
reasoningText != "" (i.e., only populate assistantMessage.Reasoning if
reasoningText is non-empty), ensuring the check happens after building
reasoningText and before assigning to assistantMessage.Reasoning in the function
that processes reasoningDetails/assistantMessage.
- Around line 16-17: Update the struct field comment for L to accurately reflect
that it stores byte length: change the comment for the L field (the struct
containing fields T and L) from "// length in characters" to "// length in UTF-8
bytes" in both locations where that struct is defined; ensure you update the
comments in the definitions referenced (the struct with fields T string
`json:"t"` and L int `json:"l"`) so they match the rest of the implementation
that uses len() and byte offsets.
🪄 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: 4a320e0c-981d-4a44-af00-0a1990b156e3
📒 Files selected for processing (5)
core/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/bedrock/chat.gocore/schemas/chatcompletions.gotests/integrations/python/tests/test_openai.py
✅ Files skipped from review due to trivial changes (2)
- core/providers/anthropic/chat_test.go
- core/providers/anthropic/chat.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/chatcompletions.go
akshaydeo
force-pushed
the
03-20-openai_-_anthropic_-_openai_thinking_fixes
branch
from
c9f2418 to
136f38b
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integrations/python/tests/test_openai.py (1)
2116-2120: Avoid gating this cross-provider test on only the OpenAI key.
@skip_if_no_api_key("openai")can skip Line 2120’s provider-parametrized test even when other providers are configured and testable. Prefer provider-specific skip logic inside the test body.Proposed adjustment
-@skip_if_no_api_key("openai") `@pytest.mark.parametrize`( "provider,model,vk_enabled", get_cross_provider_params_with_vk_for_scenario("thinking") ) def test_37a_chat_reasoning_content_is_string(self, test_config, provider, model, vk_enabled): """Test Case 37a: Chat completion with reasoning returns content as string, not array""" + try: + get_api_key(provider) + except ValueError: + pytest.skip(f"No API key available for {provider}") + client = get_provider_openai_client(provider, vk_enabled=vk_enabled)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrations/python/tests/test_openai.py` around lines 2116 - 2120, The test decorator `@skip_if_no_api_key`("openai") on test_37a_chat_reasoning_content_is_string incorrectly gates a cross-provider parametrized test; remove that decorator and perform provider-specific skipping inside the test body instead (use the existing helper by calling skip_if_no_api_key(provider) or pytest.skip when the given provider lacks credentials). Update the test that uses get_cross_provider_params_with_vk_for_scenario("thinking") to check the current provider at the top of test_37a_chat_reasoning_content_is_string and bail only for the missing-provider case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integrations/python/tests/test_openai.py`:
- Around line 2116-2120: The test decorator `@skip_if_no_api_key`("openai") on
test_37a_chat_reasoning_content_is_string incorrectly gates a cross-provider
parametrized test; remove that decorator and perform provider-specific skipping
inside the test body instead (use the existing helper by calling
skip_if_no_api_key(provider) or pytest.skip when the given provider lacks
credentials). Update the test that uses
get_cross_provider_params_with_vk_for_scenario("thinking") to check the current
provider at the top of test_37a_chat_reasoning_content_is_string and bail only
for the missing-provider case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99d51c1b-1df6-4f75-85b4-75889db1900d
📒 Files selected for processing (6)
core/providers/anthropic/chat.gocore/providers/anthropic/chat_test.gocore/providers/bedrock/chat.gocore/schemas/chatcompletions.gotests/integrations/python/config.jsontests/integrations/python/tests/test_openai.py
✅ Files skipped from review due to trivial changes (3)
- core/schemas/chatcompletions.go
- core/providers/anthropic/chat.go
- core/providers/anthropic/chat_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/bedrock/chat.go
Contributor
Author
Merge activity
|
akshaydeo
changed the base branch from
03-20-anthropic_header_selection_across_providers
to
graphite-base/2184
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
2 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
Implements multi-block content reconstruction for Anthropic and Bedrock providers to preserve the original structure of thinking and text blocks across multi-turn conversations. When responses contain multiple content blocks (thinking + text), they are combined into a single string with boundary metadata to enable accurate reconstruction in subsequent requests.
Changes
contentBlockMetastruct to track block types and lengths for reconstruction\n\nseparatorsBifrostReasoningDetailsTypeContentBlocksreasoning detailsType of change
Affected areas
How to test
Validate the multi-block content handling with the new test cases:
Test scenarios include:
Screenshots/Recordings
N/A - Backend functionality only
Breaking changes
This change maintains backward compatibility while adding new functionality for multi-block content handling.
Related issues
N/A
Security considerations
The boundary metadata is stored as JSON in reasoning details and validated during reconstruction. No sensitive data is exposed, and the fallback mechanism handles tampered content gracefully.
Checklist
docs/contributing/README.mdand followed the guidelines