feat(anthropic): preserve thinking signatures in compat APIs#178
feat(anthropic): preserve thinking signatures in compat APIs#178SantiagoDePolonia wants to merge 6 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds OpenAI-style "reasoning" support and signature propagation: new UnknownJSONFields merge helper, Responses model/JSON variants for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ResponsesAdapter
participant Core
participant AnthropicProvider
participant StreamOut
Client->>ResponsesAdapter: Send Responses request (may include `reasoning` items)
ResponsesAdapter->>Core: Parse & validate `reasoning` -> merge into Message.ExtraFields (`reasoning_details`)
ResponsesAdapter->>AnthropicProvider: Send Chat request with Messages (may include reasoning_details)
AnthropicProvider->>Core: Return streaming events (content_block_start/delta/stop, signature_delta)
Core->>ResponsesAdapter: Convert events -> reasoning_text.delta / reasoning_signature / reasoning_index events
ResponsesAdapter->>StreamOut: Emit SSE / response.output_item.added/done with reasoning payloads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/providers/anthropic/anthropic_test.go`:
- Around line 556-617: Update the test(s) to also assert the default-off
behavior by adding a disabled-path assertion: in
TestStreamChatCompletion_EmitsReasoningSignatureBehindFeatureFlag (and the other
similar test blocks referenced) add a run where the environment variable
openAICompatBreakingAnthropicThinkingSignaturesEnv is unset or set to "" before
calling provider.StreamChatCompletion, read the response body, and assert that
the response does NOT contain `"reasoning_signature":"..."` and does NOT contain
`"reasoning_index":...`; ensure you mirror the existing test flow (using
NewWithHTTPClient, SetBaseURL, io.ReadAll on body) but verify absence of those
substrings to lock in the opt-in-only behavior.
In `@internal/providers/anthropic/anthropic.go`:
- Around line 799-805: The current code uses the raw Anthropic block index
(event.Index) for reasoning_index/signatures which misaligns when empty
preambles create gaps; change to track a dense reasoning ordinal counter (e.g.,
a per-stream map like the Responses stream converter uses) that increments only
when event.Delta.Thinking contains non-empty reasoning content and emit that
dense index instead of event.Index when sc.emitReasoningSignatures is true;
update both the block around formatChatChunk(delta, nil, nil) and the analogous
site at lines ~808-812 to read from this reasoning-index map (keyed by
stream/response id) rather than event.Index so reasoning_signature and
reasoning_index remain compact and sequential.
- Around line 1695-1746: The code currently emits empty reasoning events for
placeholder thinking blocks; update reasoningTextDoneEvent (used when emitting
per-block done events) to return "" unless the referenced block has non-empty
trimmed text (i.e., skip emitting when strings.TrimSpace(block.Text.String()) ==
""), and update completeReasoningOutputIfNeeded (used to finalize the reasoning
item) to either (a) build the content slice by filtering out blocks whose
trimmed Text is empty and if the filtered content is empty, do not mark
state.Done or call WriteEvent (return ""), or (b) lazily create the reasoning
state only when the first non-empty thinking delta arrives; implement the
simpler fix (filter empty blocks on finalization and skip emitting per-block
done events for empty blocks) referencing the methods reasoningTextDoneEvent and
completeReasoningOutputIfNeeded and ensure signature emission logic remains
unchanged for non-empty blocks.
In `@internal/providers/anthropic/reasoning_compat.go`:
- Around line 126-158: The handler currently silently drops malformed/empty
reasoning payloads; update the logic so that when "reasoning_details" is present
but parseOpenAIReasoningDetails returns zero usable items, you return a
core.NewInvalidRequestError (not nil,nil), and likewise if either
"reasoning_content" or "reasoning_signature" is present but the other is missing
you return an invalid request error instead of nil,nil; specifically, after
fields.Lookup("reasoning_details") call check whether
parseOpenAIReasoningDetails yields an empty slice and return an error, and
change the early return when one of reasoningContentRaw/reasoningSignatureRaw is
empty to return core.NewInvalidRequestError describing the missing partner
field; also ensure strings.TrimSpace checks on reasoningContent and
reasoningSignature still return an error (not nil,nil) when normalized-empty,
and preserve use of parseOpenAIReasoningDetails,
openAIReasoningDetailsToAnthropicBlocks, and the anthropicContentBlock
construction.
In `@internal/providers/responses_adapter_test.go`:
- Around line 542-585: The test
TestConvertResponsesRequestToChat_RejectsNonReasoningTextParts only checks the
error string; update it to assert the returned error is a core.GatewayError and
that its ClientFacingCategory (or equivalent field/method on core.GatewayError)
equals "invalid_request_error" when calling ConvertResponsesRequestToChat;
locate the assertions after the ConvertResponsesRequestToChat call, perform a
type assertion to the core.GatewayError type (or call the accessor) and fail the
test if the error is not that typed GatewayError or its category is not
"invalid_request_error".
In `@internal/providers/responses_adapter.go`:
- Around line 173-179: The mergeResponsesReasoningExtraFields path currently
returns a successful merge even when buildResponsesReasoningExtraFields yields
no preserved reasoning_details, which allows a ContentNull assistant turn with
no payload; update mergeResponsesReasoningExtraFields to check the resulting
reasoningExtras (from buildResponsesReasoningExtraFields) and if it contains
zero preserved reasoning_details entries (i.e., is empty after filtering) return
an error (or a non-nil error) instead of merging; apply the same check to the
analogous merge logic in the nearby function(s) in the 181-233 region so callers
like convertResponsesInputItems never receive an empty reasoning payload to
synthesize an assistant message.
- Around line 280-284: mergeReasoningIntoAssistantMessage currently rebuilds
reasoning_details only from dst, which causes src.ExtraFields.reasoning_details
to be overwritten; to fix this, extract reasoning details from both dst and src
using reasoningDetailsFromUnknownFields, append/concatenate the slices
(preserving both payloads) into a single slice, then call
buildReasoningUnknownFields on that combined slice and pass the result to
core.MergeUnknownJSONFields when merging dst.ExtraFields and src.ExtraFields
(keeping the cloneResponsesMessage(dst) / merged assignment logic intact).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0d802bfa-44d0-492d-bb9c-d98714a78462
📒 Files selected for processing (11)
internal/core/json_fields.gointernal/core/json_fields_test.gointernal/core/responses.gointernal/core/responses_json.gointernal/core/responses_json_test.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/anthropic/reasoning_compat.gointernal/providers/anthropic/request_translation.gointernal/providers/responses_adapter.gointernal/providers/responses_adapter_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/providers/anthropic/reasoning_compat.go`:
- Around line 61-64: The code currently swallows json.Marshal errors when
creating reasoningContent from joinAnthropicReasoningText(details); update the
logic in the block around reasoningContent so that if json.Marshal returns an
error you either log the error (using the existing logger) or return the error
upstream instead of silently ignoring it; reference the variables and functions
reasoningContent, json.Marshal, joinAnthropicReasoningText, and the fields map
when implementing the change so the marshal failure is handled explicitly (log
or propagate) before attempting to set fields["reasoning_content"].
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7e97a2aa-aa9c-4ad2-8214-19207e09e4ab
📒 Files selected for processing (5)
internal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/anthropic/reasoning_compat.gointernal/providers/responses_adapter.gointernal/providers/responses_adapter_test.go
| reasoningContent, err := json.Marshal(joinAnthropicReasoningText(details)) | ||
| if err == nil { | ||
| fields["reasoning_content"] = reasoningContent | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Marshal error silently swallowed.
If json.Marshal fails for reasoningContent, the error is discarded and the field is simply not added. While unlikely to fail for a simple string, this hides potential issues. Consider logging the error or returning early since a marshal failure for such simple data suggests a deeper problem.
🔧 Suggested improvement
reasoningContent, err := json.Marshal(joinAnthropicReasoningText(details))
- if err == nil {
- fields["reasoning_content"] = reasoningContent
+ if err != nil {
+ // String marshal should not fail; log if it does
+ return core.UnknownJSONFields{}
}
+ fields["reasoning_content"] = reasoningContent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/anthropic/reasoning_compat.go` around lines 61 - 64, The
code currently swallows json.Marshal errors when creating reasoningContent from
joinAnthropicReasoningText(details); update the logic in the block around
reasoningContent so that if json.Marshal returns an error you either log the
error (using the existing logger) or return the error upstream instead of
silently ignoring it; reference the variables and functions reasoningContent,
json.Marshal, joinAnthropicReasoningText, and the fields map when implementing
the change so the marshal failure is handled explicitly (log or propagate)
before attempting to set fields["reasoning_content"].
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/providers/anthropic/anthropic.go (1)
1696-1705:⚠️ Potential issue | 🟠 MajorDelay reasoning item creation until the first real
thinking_delta.The Responses stream still allocates/emits reasoning state before it knows the block contains text. A placeholder
thinkingblock can therefore leave a danglingresponse.output_item.added, and if the next block is the first real one the stream emitscontent_index: 1whileresponse.output_item.donecompacts that same block tocontent[0]. Create the block lazily on the first non-whitespacethinking_delta, and only attach a signature once that block exists.Possible fix
func (sc *responsesStreamConverter) setReasoningSignature(anthropicIndex int, signature string) { if signature == "" { return } - state := sc.ensureResponsesReasoningState() - contentIndex := sc.reasoningContentIndex(anthropicIndex) - if contentIndex >= len(state.Blocks) { + state := sc.reasoning + if state == nil { + return + } + contentIndex, ok := sc.thinkingContentIndices[anthropicIndex] + if !ok || contentIndex >= len(state.Blocks) { return } state.Blocks[contentIndex].Signature = signature } @@ case "content_block_start": if event.ContentBlock != nil && event.ContentBlock.Type == "thinking" { - if !sc.emitReasoningSignatures { - return "" - } - sc.reasoningContentIndex(event.Index) - return sc.startReasoningOutput() + return "" } @@ case "thinking_delta": if !sc.emitReasoningSignatures || event.Delta.Thinking == "" { return "" } - prefix := sc.startReasoningOutput() - sc.appendReasoningText(event.Index, event.Delta.Thinking) - contentIndex := sc.reasoningContentIndex(event.Index) + contentIndex, ok := sc.thinkingContentIndices[event.Index] + prefix := "" + if !ok { + if strings.TrimSpace(event.Delta.Thinking) == "" { + return "" + } + prefix = sc.startReasoningOutput() + contentIndex = sc.reasoningContentIndex(event.Index) + } + sc.appendReasoningText(event.Index, event.Delta.Thinking) return prefix + sc.output.WriteEvent("response.reasoning_text.delta", map[string]any{ "type": "response.reasoning_text.delta", "item_id": sc.reasoning.ItemID,Also applies to: 1740-1755, 1802-1807, 1829-1848
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/anthropic/anthropic.go` around lines 1696 - 1705, The reasoning block is being created eagerly which can emit a placeholder thinking block before any real text; update setReasoningSignature and the code paths that create reasoning blocks (e.g., where ensureResponsesReasoningState(), reasoningContentIndex(), and block append logic are used) to defer creating the reasoning Block until you see the first non-whitespace thinking_delta; specifically, only call ensureResponsesReasoningState()/append a new Block when the incoming thinking_delta contains non-whitespace content and then attach Signature with setReasoningSignature only if the corresponding block exists (guard against contentIndex >= len(state.Blocks) before setting Signature). Apply the same lazy-creation guard to the other similar sites noted (around the blocks indicated by the reviewer: ~1740-1755, ~1802-1807, ~1829-1848) so no placeholder block is emitted for empty/whitespace thinking deltas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/json_fields.go`:
- Around line 45-70: The current parsing in internal/core/json_fields.go
inconsistently handles errors: initial token/delimiter errors use continue but
mid-object errors return UnknownJSONFields{}, causing previously merged data
(merged map) to be discarded; change the mid-object error handling in the json
decoding loop (where readJSONObjectKey and dec.Decode(&value) are called) to
mirror the initial behavior by skipping the entire corrupt fieldSet instead of
returning empty — i.e., on failing readJSONObjectKey or dec.Decode, continue to
the next fieldSet, preserve merged data, and only return UnknownJSONFields{} if
no valid entries were ever merged (or alternatively change the function to
return an error so callers can decide), update callers (e.g.,
responses_adapter.go usage of dst.ExtraFields) accordingly to handle either a
nil/unchanged result or an error.
---
Duplicate comments:
In `@internal/providers/anthropic/anthropic.go`:
- Around line 1696-1705: The reasoning block is being created eagerly which can
emit a placeholder thinking block before any real text; update
setReasoningSignature and the code paths that create reasoning blocks (e.g.,
where ensureResponsesReasoningState(), reasoningContentIndex(), and block append
logic are used) to defer creating the reasoning Block until you see the first
non-whitespace thinking_delta; specifically, only call
ensureResponsesReasoningState()/append a new Block when the incoming
thinking_delta contains non-whitespace content and then attach Signature with
setReasoningSignature only if the corresponding block exists (guard against
contentIndex >= len(state.Blocks) before setting Signature). Apply the same
lazy-creation guard to the other similar sites noted (around the blocks
indicated by the reviewer: ~1740-1755, ~1802-1807, ~1829-1848) so no placeholder
block is emitted for empty/whitespace thinking deltas.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 353c203d-7b48-4f27-a0b9-d4b09d7da98b
📒 Files selected for processing (2)
internal/core/json_fields.gointernal/providers/anthropic/anthropic.go
| dec := json.NewDecoder(bytes.NewReader(fieldSet.raw)) | ||
| tok, err := dec.Token() | ||
| if err != nil { | ||
| continue | ||
| } | ||
| delim, ok := tok.(json.Delim) | ||
| if !ok || delim != '{' { | ||
| continue | ||
| } | ||
|
|
||
| for dec.More() { | ||
| key, ok := readJSONObjectKey(dec) | ||
| if !ok { | ||
| return UnknownJSONFields{} | ||
| } | ||
|
|
||
| var value json.RawMessage | ||
| if err := dec.Decode(&value); err != nil { | ||
| return UnknownJSONFields{} | ||
| } | ||
|
|
||
| if merged == nil { | ||
| merged = make(map[string]json.RawMessage) | ||
| } | ||
| merged[key] = CloneRawJSON(value) | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling causes silent data loss.
The error handling strategy is inconsistent:
- Lines 47-52: Initial tokenization/delimiter errors use
continue, preserving previously merged data - Lines 58 and 63: Mid-object parse errors use
return UnknownJSONFields{}, discarding all accumulated data
If fieldSet #1 merges successfully but `fieldSet `#2 has a corrupt key/value mid-object, the function silently returns empty, losing fieldSet #1``'s data. The call sites in responses_adapter.go assign results directly to `dst.ExtraFields` without validation, so this data loss goes undetected.
Consider making error handling consistent—either skip corrupt field sets entirely (like initial errors) or return an error so callers can react:
Option A: Skip corrupt field sets consistently
for dec.More() {
key, ok := readJSONObjectKey(dec)
if !ok {
- return UnknownJSONFields{}
+ break // skip this corrupt fieldSet, keep accumulated data
}
var value json.RawMessage
if err := dec.Decode(&value); err != nil {
- return UnknownJSONFields{}
+ break // skip this corrupt fieldSet, keep accumulated data
}
if merged == nil {
merged = make(map[string]json.RawMessage)
}
merged[key] = CloneRawJSON(value)
}Option B: Return error for caller awareness
-func MergeUnknownJSONFields(fields ...UnknownJSONFields) UnknownJSONFields {
+func MergeUnknownJSONFields(fields ...UnknownJSONFields) (UnknownJSONFields, error) {
if len(fields) == 0 {
- return UnknownJSONFields{}
+ return UnknownJSONFields{}, nil
}
// ... update error sites to return (UnknownJSONFields{}, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dec := json.NewDecoder(bytes.NewReader(fieldSet.raw)) | |
| tok, err := dec.Token() | |
| if err != nil { | |
| continue | |
| } | |
| delim, ok := tok.(json.Delim) | |
| if !ok || delim != '{' { | |
| continue | |
| } | |
| for dec.More() { | |
| key, ok := readJSONObjectKey(dec) | |
| if !ok { | |
| return UnknownJSONFields{} | |
| } | |
| var value json.RawMessage | |
| if err := dec.Decode(&value); err != nil { | |
| return UnknownJSONFields{} | |
| } | |
| if merged == nil { | |
| merged = make(map[string]json.RawMessage) | |
| } | |
| merged[key] = CloneRawJSON(value) | |
| } | |
| dec := json.NewDecoder(bytes.NewReader(fieldSet.raw)) | |
| tok, err := dec.Token() | |
| if err != nil { | |
| continue | |
| } | |
| delim, ok := tok.(json.Delim) | |
| if !ok || delim != '{' { | |
| continue | |
| } | |
| for dec.More() { | |
| key, ok := readJSONObjectKey(dec) | |
| if !ok { | |
| break // skip this corrupt fieldSet, keep accumulated data | |
| } | |
| var value json.RawMessage | |
| if err := dec.Decode(&value); err != nil { | |
| break // skip this corrupt fieldSet, keep accumulated data | |
| } | |
| if merged == nil { | |
| merged = make(map[string]json.RawMessage) | |
| } | |
| merged[key] = CloneRawJSON(value) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/json_fields.go` around lines 45 - 70, The current parsing in
internal/core/json_fields.go inconsistently handles errors: initial
token/delimiter errors use continue but mid-object errors return
UnknownJSONFields{}, causing previously merged data (merged map) to be
discarded; change the mid-object error handling in the json decoding loop (where
readJSONObjectKey and dec.Decode(&value) are called) to mirror the initial
behavior by skipping the entire corrupt fieldSet instead of returning empty —
i.e., on failing readJSONObjectKey or dec.Decode, continue to the next fieldSet,
preserve merged data, and only return UnknownJSONFields{} if no valid entries
were ever merged (or alternatively change the function to return an error so
callers can decide), update callers (e.g., responses_adapter.go usage of
dst.ExtraFields) accordingly to handle either a nil/unchanged result or an
error.
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 `@internal/providers/anthropic/request_translation.go`:
- Around line 207-210: When extracting Anthropic reasoning blocks via
extractAnthropicReasoningBlocksFromExtraFields(msg.ExtraFields), validate that
any presence of compat reasoning fields (reasoning_details / reasoning_content /
reasoning_signature) is only allowed for assistant turns when the Anthropic
compatibility flag is enabled; otherwise return a client-facing invalid request
error using core.NewInvalidRequestError (a core.GatewayError). Concretely: after
calling extractAnthropicReasoningBlocksFromExtraFields, if it found any compat
reasoning fields and msg.Role is not "assistant" OR the Anthropic compatibility
flag (the module/struct bool that controls compat translation) is false,
immediately fail with core.NewInvalidRequestError describing the unsupported
fields instead of continuing.
In `@internal/providers/responses_adapter.go`:
- Around line 470-475: When PreserveAnthropicReasoningCompat is true we still
must parse/validate message-level reasoning extras before allowing them through:
update the branch around containsAnthropicReasoningCompatFields to, even when
opts.PreserveAnthropicReasoningCompat is true, attempt to decode/validate the
assistant message extras keys
(reasoning_details/reasoning_content/reasoning_signature) using the same
validation used later in mergeReasoningIntoAssistantMessage and, on any
parse/validation failure, return a core.NewInvalidRequestError (i.e., a
GatewayError with category invalid_request_error) describing the invalid
reasoning payload; apply the same change to the other similar block referenced
(lines ~560-565) so malformed reasoning JSON is rejected at request validation
time rather than during merge.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 89c0c8b4-adb8-4c9f-b884-53a16918469d
📒 Files selected for processing (9)
.env.templateREADME.mddocs/advanced/configuration.mdxinternal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/anthropic/reasoning_compat.gointernal/providers/anthropic/request_translation.gointernal/providers/responses_adapter.gointernal/providers/responses_adapter_test.go
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
Tests