Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a modular analysis-and-processing path for assistant content thinking/tool_use blocks: per-block analysis, signature resolution/caching, optional in-place (surgical) signature updates, block dropping/reordering, and non‑streaming handling; updates tests and renames the internal version package to Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
omarluq
force-pushed
the
fix/thinking-block-signature-priority
branch
from
97fd66d to
486a0ca
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an Anthropic API error ("thinking blocks in the latest assistant message cannot be modified") by implementing two key changes: (1) prioritizing client-provided signatures over cached signatures to match Anthropic's exact signature expectations, and (2) introducing a "surgical update" approach that preserves unknown fields in thinking blocks byte-for-byte rather than reconstructing them from scratch.
Changes:
- Refactored signature resolution to prioritize client signatures first, then cache as fallback (reversed from previous cache-first approach)
- Added surgical update optimization that only modifies signature fields in-place, preserving all other fields like 'data' and 'redacted_thinking'
- Extracted analysis logic into separate functions to reduce cognitive complexity
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/proxy/thinking.go | Implements signature priority change and surgical update approach with new helper functions (analyzeContentBlocks, surgicalUpdate, resolveThinkingSignature) |
| internal/proxy/thinking_test.go | Adds regression test verifying unknown fields are preserved in thinking blocks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestProcessRequestThinkingPreservesUnknownFields(t *testing.T) { | ||
| // Regression test for Anthropic API error: | ||
| // "thinking blocks in the latest assistant message cannot be modified" | ||
| // The proxy was reconstructing thinking blocks from scratch, losing fields like | ||
| // 'data' and 'redacted_thinking' that Anthropic now requires to be preserved. | ||
| ctx := context.Background() | ||
|
|
||
| sig := "valid_signature_that_is_definitely_long_enough_for_validation" | ||
|
|
||
| // Thinking block with additional fields that must be preserved | ||
| body := `{ | ||
| "model": "claude-sonnet-4", | ||
| "messages": [ | ||
| { | ||
| "role": "assistant", | ||
| "content": [ | ||
| { | ||
| "type": "thinking", | ||
| "thinking": "Some thinking...", | ||
| "signature": "` + sig + `", | ||
| "data": {"some": "metadata", "tokens": 1234} | ||
| }, | ||
| { | ||
| "type": "thinking", | ||
| "thinking": "Redacted thinking...", | ||
| "signature": "` + sig + `", | ||
| "redacted_thinking": true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }` | ||
|
|
||
| modifiedBody, thinkingCtx, err := ProcessRequestThinking(ctx, []byte(body), "claude-sonnet-4", nil) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 0, thinkingCtx.DroppedBlocks) | ||
|
|
||
| // Verify first thinking block preserves 'data' field | ||
| dataField := gjson.GetBytes(modifiedBody, "messages.0.content.0.data") | ||
| assert.True(t, dataField.Exists(), "data field should be preserved") | ||
| assert.Equal(t, "metadata", dataField.Get("some").String(), "data.some should be preserved") | ||
| assert.Equal(t, float64(1234), dataField.Get("tokens").Float(), "data.tokens should be preserved") | ||
|
|
||
| // Verify second thinking block preserves 'redacted_thinking' field | ||
| redactedField := gjson.GetBytes(modifiedBody, "messages.0.content.1.redacted_thinking") | ||
| assert.True(t, redactedField.Exists(), "redacted_thinking field should be preserved") | ||
| assert.True(t, redactedField.Bool(), "redacted_thinking should be true") | ||
| } |
There was a problem hiding this comment.
The PR title mentions "prioritize client signature over cache to match Anthropic's exact signature", but there's no test coverage for this critical behavior change. Consider adding a test that:
- Sets up a cache with one signature for a thinking block
- Sends a request with the same thinking text but a DIFFERENT valid client-provided signature
- Verifies that the client signature is used (not the cached one)
This would ensure the priority order in resolveThinkingSignature (lines 283-318 in thinking.go) is working correctly.
internal/proxy/thinking.go
Outdated
| } | ||
|
|
||
| needsDrop := keptCount < analysis.totalBlocks | ||
| needsReorder := !needsDrop && checkReorderNeeded(analysis.blockTypes) |
There was a problem hiding this comment.
There's a logic bug when both drops and reordering are needed. The condition needsReorder := !needsDrop && checkReorderNeeded(analysis.blockTypes) on line 228 means that if any blocks are dropped, reordering is never performed, even if the remaining blocks still need to be reordered.
Example scenario:
- Input:
[{"type": "text"}, {"type": "thinking", "signature": ""}, {"type": "thinking", "signature": "valid_sig..."}] - After dropping unsigned thinking block:
[{"type": "text"}, {"type": "thinking", "signature": "valid_sig..."}] - Expected:
[{"type": "thinking", "signature": "valid_sig..."}, {"type": "text"}](thinking should come first) - Actual:
[{"type": "text"}, {"type": "thinking", "signature": "valid_sig..."}](not reordered)
The old code checked reordering AFTER collection (using collector.modifiedTypes), but the new code checks BEFORE collection (using analysis.blockTypes) and short-circuits when drops are needed.
Fix: Either always reorder in rebuildContent regardless of needsReorder, or calculate reordering need based on the collected blocks rather than original blocks.
| needsReorder := !needsDrop && checkReorderNeeded(analysis.blockTypes) | |
| needsReorder := checkReorderNeeded(analysis.blockTypes) |
omarluq
force-pushed
the
fix/thinking-block-signature-priority
branch
from
486a0ca to
f240e27
Compare
omarluq
force-pushed
the
fix/thinking-block-signature-priority
branch
from
f240e27 to
ac073e5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
==========================================
- Coverage 78.30% 78.15% -0.15%
==========================================
Files 94 94
Lines 5743 5800 +57
==========================================
+ Hits 4497 4533 +36
- Misses 961 980 +19
- Partials 285 287 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
2 participants
No description provided.