fix(anthropic): drop top_p when both temperature and top_p are set + flatten response#99
Merged
Merged
Conversation
Anthropic's API rejects requests that include both parameters. When both are provided, we now keep temperature (Anthropic's recommended primary knob) and drop top_p with a warning log. This also covers the Bedrock Anthropic path which shares the same request conversion. Co-Authored-By: Claude Opus 4.6 <[email protected]>
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:
📝 WalkthroughWalkthroughAnthropic provider conversions updated: when both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Comment |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - arm64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub - arm64)Click to expand results |
Adds 6 tests for the Anthropic provider: 2 cassette-based integration tests (basic chat + tool calls) and 4 unit tests for content block conversion and response translation. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…mpatibility Anthropic returns content as a list of content blocks, but OpenAI-compatible clients expect a plain string. This broke RAGAS, DeepEval, Instructor, and any library that calls .find() or json.loads() on message.content. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
![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: 2
🧹 Nitpick comments (2)
src/providers/anthropic/test.rs (2)
148-177: Add a regression test for the actualtop_pfix.Both request builders here set
top_p: None, so the newAnthropicChatCompletionRequest::from()branch from this PR never gets exercised. A small unit test should cover both(temperature, top_p)andtop_p-only requests so we lock in “droptop_ponly when both are set”.Also applies to: 278-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/anthropic/test.rs` around lines 148 - 177, The test currently constructs ChatCompletionRequest with top_p: None so the new AnthropicChatCompletionRequest::from() branch isn't exercised; add unit tests that create requests covering both cases — one with (temperature: Some(...), top_p: Some(...)) and one with temperature: None and top_p: Some(...) — then call AnthropicChatCompletionRequest::from() (and the equivalent test at the other location around lines 278-308) and assert the resulting Anthropic request drops top_p only when both temperature and top_p are set, and preserves top_p when temperature is absent.
370-379: Tighten these content-shape assertions.Both tests pass for multiple incompatible
contentserializations (string,array, and sometimesnull), so they won't catch regressions in the OpenAI-compatible wire format. If this is intentional characterization coverage, rename the tests/comments accordingly; otherwise assert the single supported shape.Also applies to: 424-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/anthropic/test.rs` around lines 370 - 379, The test currently accepts multiple shapes for the response body (string, array, null) which hides regressions — update the assertions in src/providers/anthropic/test.rs so they only accept the single supported shape (a string). Replace the conditional branch that checks content.is_string() / content.is_array() with a strict assertion that content.is_string() and then assert content.as_str().unwrap() == "Hello world!"; make the same change in the analogous block referenced around lines 424-433 to ensure both tests validate the OpenAI-compatible wire format only. Ensure you only modify the test assertions (the content variable and its checks) and not other test scaffolding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/anthropic/test.rs`:
- Around line 300-301: The test uses a non-deterministic tool selection
(ToolChoice::Simple(SimpleToolChoice::Auto)) which allows the model to answer
without invoking the tool; change the configuration to force a tool call (e.g.,
use the forced/simple explicit variant instead of Auto) for the tool created by
create_weather_tool_definition(), or alternatively update the test prompt to
explicitly require calling get_weather so the cassette is deterministic; apply
the same fix to the other occurrence around the block that spans the second
instance (lines referencing the same
create_weather_tool_definition()/ToolChoice::Simple usage).
- Around line 23-34: The cassette writer is appending to any existing array
which breaks tests that expect exactly one interaction; change save_to_cassette
so it overwrites instead of appending: stop reading and merging existing entries
(or clear interactions) and write only the current response as a single-element
array (e.g., serialize vec![response.clone()]) to cassette_path, using the
existing interactions/response/cassette_path symbols to locate and update the
logic.
---
Nitpick comments:
In `@src/providers/anthropic/test.rs`:
- Around line 148-177: The test currently constructs ChatCompletionRequest with
top_p: None so the new AnthropicChatCompletionRequest::from() branch isn't
exercised; add unit tests that create requests covering both cases — one with
(temperature: Some(...), top_p: Some(...)) and one with temperature: None and
top_p: Some(...) — then call AnthropicChatCompletionRequest::from() (and the
equivalent test at the other location around lines 278-308) and assert the
resulting Anthropic request drops top_p only when both temperature and top_p are
set, and preserves top_p when temperature is absent.
- Around line 370-379: The test currently accepts multiple shapes for the
response body (string, array, null) which hides regressions — update the
assertions in src/providers/anthropic/test.rs so they only accept the single
supported shape (a string). Replace the conditional branch that checks
content.is_string() / content.is_array() with a strict assertion that
content.is_string() and then assert content.as_str().unwrap() == "Hello world!";
make the same change in the analogous block referenced around lines 424-433 to
ensure both tests validate the OpenAI-compatible wire format only. Ensure you
only modify the test assertions (the content variable and its checks) and not
other test scaffolding.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ae37d03-bd5d-46f9-ab9e-cba95a2a7952
📒 Files selected for processing (4)
src/providers/anthropic/mod.rssrc/providers/anthropic/test.rstests/cassettes/anthropic/chat_completions_basic.jsontests/cassettes/anthropic/chat_completions_with_tool_calls.json
✅ Files skipped from review due to trivial changes (3)
- src/providers/anthropic/mod.rs
- tests/cassettes/anthropic/chat_completions_basic.json
- tests/cassettes/anthropic/chat_completions_with_tool_calls.json
- Overwrite cassettes on re-record instead of appending - Force tool call with ToolChoice::Required for deterministic cassettes - Strict assertions: content must be string, null when tool-only - Add unit tests for temperature/top_p interaction Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/providers/anthropic/test.rs (2)
122-123: Add an explicit non-emptychoicesassertion before indexing.Both cassette-validation paths index
choices[0]after checking only thatchoicesis an array. A direct non-empty assertion gives clearer failures when cassette shape regresses.Suggested patch
- let message = &response["choices"][0]["message"]; + let choices = response["choices"].as_array().expect("Choices must be an array"); + assert!(!choices.is_empty(), "Choices array should not be empty"); + let message = &choices[0]["message"];Apply similarly in the tool-calls validation block.
Also applies to: 227-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/anthropic/test.rs` around lines 122 - 123, Add an explicit non-empty assertion for "choices" before any indexing into choices[0] in the Anthropics tests: ensure you assert that response["choices"].as_array().map(|a| !a.is_empty()).unwrap_or(false) (or equivalent) is true immediately before the lines that set let message = &response["choices"][0]["message"]; and the analogous tool-calls validation block; update both occurrences (the cassette-validation path and the tool-calls path) so failures clearly report an empty choices array instead of an out‑of‑bounds panic.
21-33: Fail fast when cassette persistence fails.
save_to_cassettecurrently swallows directory creation and serialization failures, which can hide recording issues and make later validation failures harder to diagnose.Suggested patch
async fn save_to_cassette(test_name: &str, response: &Value) { let cassettes_dir = PathBuf::from("tests/cassettes/anthropic"); - std::fs::create_dir_all(&cassettes_dir).ok(); + fs::create_dir_all(&cassettes_dir).expect("Failed to create cassette directory"); let cassette_path = cassettes_dir.join(format!("{}.json", test_name)); let interactions = vec![response.clone()]; - if let Ok(content) = serde_json::to_string_pretty(&interactions) { - fs::write(&cassette_path, content).expect("Failed to write cassette"); - debug!( - "Successfully saved interaction to cassette: {:?}", - cassette_path - ); - } + let content = + serde_json::to_string_pretty(&interactions).expect("Failed to serialize cassette"); + fs::write(&cassette_path, content).expect("Failed to write cassette"); + debug!( + "Successfully saved interaction to cassette: {:?}", + cassette_path + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/anthropic/test.rs` around lines 21 - 33, The cassette persistence code currently swallows failures (std::fs::create_dir_all(&cassettes_dir).ok() and the if let Ok(...) branch), so update the save_to_cassette flow to fail fast: propagate or return errors from std::fs::create_dir_all(&cassettes_dir) and from serde_json::to_string_pretty(&interactions) (or replace .ok() / if let with ?/expect with clear messages) instead of ignoring them; ensure cassette_path and interactions logic remains but that create_dir_all, serde_json::to_string_pretty, and fs::write failures are surfaced from the function (e.g., by making save_to_cassette return Result and using ? on those calls) so persistence problems are not swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/providers/anthropic/test.rs`:
- Around line 122-123: Add an explicit non-empty assertion for "choices" before
any indexing into choices[0] in the Anthropics tests: ensure you assert that
response["choices"].as_array().map(|a| !a.is_empty()).unwrap_or(false) (or
equivalent) is true immediately before the lines that set let message =
&response["choices"][0]["message"]; and the analogous tool-calls validation
block; update both occurrences (the cassette-validation path and the tool-calls
path) so failures clearly report an empty choices array instead of an
out‑of‑bounds panic.
- Around line 21-33: The cassette persistence code currently swallows failures
(std::fs::create_dir_all(&cassettes_dir).ok() and the if let Ok(...) branch), so
update the save_to_cassette flow to fail fast: propagate or return errors from
std::fs::create_dir_all(&cassettes_dir) and from
serde_json::to_string_pretty(&interactions) (or replace .ok() / if let with
?/expect with clear messages) instead of ignoring them; ensure cassette_path and
interactions logic remains but that create_dir_all,
serde_json::to_string_pretty, and fs::write failures are surfaced from the
function (e.g., by making save_to_cassette return Result and using ? on those
calls) so persistence problems are not swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 822fc859-b1a3-42e6-9d56-bb4c27fc45c5
📒 Files selected for processing (1)
src/providers/anthropic/test.rs
- save_to_cassette now uses expect() instead of silently swallowing errors - Add explicit non-empty choices assertion before indexing choices[0] Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/providers/anthropic/test.rs (3)
146-176: Consider a request builder/helper for test setup clarity.
ChatCompletionRequestinitialization is repeated with many identical fields across integration/unit tests. Centralizing defaults in a helper (then overriding only relevant fields) will make intent clearer and avoid accidental divergence.Also applies to: 274-304, 436-464, 476-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/anthropic/test.rs` around lines 146 - 176, Tests repeatedly construct ChatCompletionRequest/ChatCompletionMessage with many identical fields; factor out a test helper or builder (e.g., a function like default_chat_request() or ChatCompletionRequestBuilder) that returns a prefilled ChatCompletionRequest using default values for fields such as temperature, stream, max_tokens, and nested ChatCompletionMessage with ChatMessageContent::String, then update the tests to call that helper and only override fields relevant to each test (refer to ChatCompletionRequest, ChatCompletionMessage, and ChatMessageContent to locate the constructors to replace).
435-472: Add an assertion for warning emission on the drop path.The conversion behavior (
top_pdropped whentemperatureis set) is covered well; adding a log assertion would fully lock in the contract described by this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/anthropic/test.rs` around lines 435 - 472, Update the test function test_request_drops_top_p_when_both_temperature_and_top_p_set to also assert that a warning is emitted when AnthropicChatCompletionRequest::from drops top_p: enable/log-capture for the test (using the project's logging/tracing test helper), perform the conversion into anthropic_request, and assert that a warning-level record containing "top_p" (or another clear message emitted by the conversion code) was logged in addition to the existing assertions about anthropic_request.temperature and anthropic_request.top_p.
94-139: Extract shared cassette validation helper to reduce drift.The two validation branches duplicate path resolution, file loading, JSON parsing, and singleton interaction checks. A small helper (e.g.,
load_single_interaction(test_name) -> Value) will keep assertions consistent and reduce maintenance cost.♻️ Suggested refactor sketch
+fn load_single_interaction(test_name: &str) -> Value { + let cassette_path = PathBuf::from("tests/cassettes/anthropic") + .join(format!("{}.json", test_name)); + assert!( + cassette_path.exists(), + "Cassette file does not exist. Run with RECORD_MODE=1 ANTHROPIC_API_KEY=sk-ant-... to create it" + ); + let cassette_content = fs::read_to_string(&cassette_path) + .expect("Failed to read cassette file"); + let interactions: Vec<Value> = serde_json::from_str(&cassette_content) + .expect("Failed to parse cassette JSON"); + assert_eq!(interactions.len(), 1, "Cassette should have exactly 1 interaction"); + interactions.into_iter().next().unwrap() +}Also applies to: 204-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/anthropic/test.rs` around lines 94 - 139, Extract the duplicated cassette-loading and singleton-interaction checks into a helper like load_single_interaction(test_name: &str) -> Value that performs PathBuf::from("tests/cassettes/anthropic").join(format!("{}.json", test_name)), asserts the file exists, reads it (fs::read_to_string), parses JSON into Vec<Value>, asserts interactions.len() == 1, and returns interactions.remove(0) (the single Value); then replace the duplicated blocks in both branches with a call to load_single_interaction(test_name) and keep the remaining per-test assertions (id/choices/usage/message content) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/providers/anthropic/test.rs`:
- Around line 146-176: Tests repeatedly construct
ChatCompletionRequest/ChatCompletionMessage with many identical fields; factor
out a test helper or builder (e.g., a function like default_chat_request() or
ChatCompletionRequestBuilder) that returns a prefilled ChatCompletionRequest
using default values for fields such as temperature, stream, max_tokens, and
nested ChatCompletionMessage with ChatMessageContent::String, then update the
tests to call that helper and only override fields relevant to each test (refer
to ChatCompletionRequest, ChatCompletionMessage, and ChatMessageContent to
locate the constructors to replace).
- Around line 435-472: Update the test function
test_request_drops_top_p_when_both_temperature_and_top_p_set to also assert that
a warning is emitted when AnthropicChatCompletionRequest::from drops top_p:
enable/log-capture for the test (using the project's logging/tracing test
helper), perform the conversion into anthropic_request, and assert that a
warning-level record containing "top_p" (or another clear message emitted by the
conversion code) was logged in addition to the existing assertions about
anthropic_request.temperature and anthropic_request.top_p.
- Around line 94-139: Extract the duplicated cassette-loading and
singleton-interaction checks into a helper like
load_single_interaction(test_name: &str) -> Value that performs
PathBuf::from("tests/cassettes/anthropic").join(format!("{}.json", test_name)),
asserts the file exists, reads it (fs::read_to_string), parses JSON into
Vec<Value>, asserts interactions.len() == 1, and returns interactions.remove(0)
(the single Value); then replace the duplicated blocks in both branches with a
call to load_single_interaction(test_name) and keep the remaining per-test
assertions (id/choices/usage/message content) as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9bb1c99-57b3-4424-b1a0-798b39738f0c
📒 Files selected for processing (1)
src/providers/anthropic/test.rs
doronkopit5
changed the title
doronkopit5
changed the title
OzBenSimhonTraceloop
approved these changes
Apr 15, 2026
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
temperatureandtop_p("temperature and top_p cannot both be specified for this model")temperature(Anthropic's recommended primary knob) and droptop_p, logging a warningAnthropicChatCompletionRequest::from()conversionResearch
Investigated how competitor gateways (LiteLLM, Portkey, OpenRouter) handle this — none of them do. Projects that fix it (OpenHands, n8n, LobeHub) all converge on the same approach: drop
top_p, keeptemperature.Test plan
cargo buildpassescargo testpasses (79 unit tests)temperatureandtop_p→ succeeds,top_pdropped, warning loggedtop_p→ preservedtemperature→ works as before🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests