fix(llm-client): drop unsigned thinking blocks on the Anthropic leg - #285
Open
sabhatinas wants to merge 1 commit into
Open
fix(llm-client): drop unsigned thinking blocks on the Anthropic leg#285sabhatinas wants to merge 1 commit into
sabhatinas wants to merge 1 commit into
Conversation
Signed-off-by: sabhatinas <sabhatinas@nvidia.com>
WalkthroughAnthropic request preparation now removes unsigned ChangesAnthropic request cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/libsy-llm-client/src/client.rs`:
- Around line 1315-1332: Update the matcher in the test around the blocks and
thinking checks to assert that the message whose content contained only the
unsigned block remains in messages with content exactly equal to an empty
string. Keep the existing signed-thinking preservation checks, and ensure the
assertion distinguishes this required replacement from dropping the message or
omitting its content.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: f4c5cbcd-a954-4555-926e-efd06e46a38c
📒 Files selected for processing (1)
crates/libsy-llm-client/src/client.rs
Comment on lines
+1315
to
+1332
| // The unsigned block is gone, the signed one survives, and the | ||
| // message whose only block was unsigned is not left with an empty | ||
| // content array. | ||
| let blocks: Vec<&Value> = messages | ||
| .iter() | ||
| .filter_map(|message| message.get("content")) | ||
| .filter_map(Value::as_array) | ||
| .flatten() | ||
| .collect(); | ||
| let thinking: Vec<&&Value> = blocks | ||
| .iter() | ||
| .filter(|block| block.get("type").and_then(Value::as_str) == Some("thinking")) | ||
| .collect(); | ||
| thinking.len() == 1 | ||
| && thinking[0].get("signature").and_then(Value::as_str) == Some("sig-abc") | ||
| && messages | ||
| .iter() | ||
| .all(|message| message.get("content") != Some(&json!([]))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the required empty-string replacement.
The matcher does not verify that the message with only an unsigned block remains in messages with content: "". It also passes if a regression drops that message or omits its content, which changes conversation history.
Proposed test assertion
thinking.len() == 1
&& thinking[0].get("signature").and_then(Value::as_str) == Some("sig-abc")
+ && messages.len() == 4
+ && messages
+ .get(1)
+ .and_then(|message| message.get("content"))
+ == Some(&json!(""))
&& messages
.iter()
.all(|message| message.get("content") != Some(&json!([])))📝 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.
Suggested change
| // The unsigned block is gone, the signed one survives, and the | |
| // message whose only block was unsigned is not left with an empty | |
| // content array. | |
| let blocks: Vec<&Value> = messages | |
| .iter() | |
| .filter_map(|message| message.get("content")) | |
| .filter_map(Value::as_array) | |
| .flatten() | |
| .collect(); | |
| let thinking: Vec<&&Value> = blocks | |
| .iter() | |
| .filter(|block| block.get("type").and_then(Value::as_str) == Some("thinking")) | |
| .collect(); | |
| thinking.len() == 1 | |
| && thinking[0].get("signature").and_then(Value::as_str) == Some("sig-abc") | |
| && messages | |
| .iter() | |
| .all(|message| message.get("content") != Some(&json!([]))) | |
| // The unsigned block is gone, the signed one survives, and the | |
| // message whose only block was unsigned is not left with an empty | |
| // content array. | |
| let blocks: Vec<&Value> = messages | |
| .iter() | |
| .filter_map(|message| message.get("content")) | |
| .filter_map(Value::as_array) | |
| .flatten() | |
| .collect(); | |
| let thinking: Vec<&&Value> = blocks | |
| .iter() | |
| .filter(|block| block.get("type").and_then(Value::as_str) == Some("thinking")) | |
| .collect(); | |
| thinking.len() == 1 | |
| && thinking[0].get("signature").and_then(Value::as_str) == Some("sig-abc") | |
| && messages.len() == 4 | |
| && messages | |
| .get(1) | |
| .and_then(|message| message.get("content")) | |
| == Some(&json!("")) | |
| && messages | |
| .iter() | |
| .all(|message| message.get("content") != Some(&json!([]))) |
🤖 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 `@crates/libsy-llm-client/src/client.rs` around lines 1315 - 1332, Update the
matcher in the test around the blocks and thinking checks to assert that the
message whose content contained only the unsigned block remains in messages with
content exactly equal to an empty string. Keep the existing signed-thinking
preservation checks, and ensure the assertion distinguishes this required
replacement from dropping the message or omitting its content.
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
1 participant
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.
What
thinkingblocks that have no signature, on the Anthropic leg oflibsy-llm-client.switchyard-componentsalready does this;libsy-llm-client(what the Rust server uses) did not. Same parity gap as fix(llm-client): strip Anthropic-incompatible fields on the Anthropic leg #248, next item on the list.Why
{"type": "thinking", ..., "signature": ""}, and the first escalation toaws/anthropic/bedrock-claude-opus-4-8fails with403 The request signature we calculated does not match the signature you provided.Same key, same body otherwise. Bedrock enforces the signed-thinking rule; Azure-hosted Anthropic currently does not, which is why only Bedrock routes broke.
output_config(200), and credentials (Bedrock passthrough with no weak tier scored reward 1.0).Testing
anthropic_requests_drop_unsigned_thinking_blocks: unsigned block dropped, signed block kept, no message left with an empty content array. Verified it fails without the strip and passes with it.cargo test -p switchyard-llm-client(46) and-p switchyard-server(47) pass. fmt and clippy clean.Summary by CodeRabbit