Repository navigation
fix: gate Anthropic thinking blocks on request - #8485
MatejKosec wants to merge 4 commits into
Conversation
73efa96 to
af23b77
Compare
WalkthroughIntroduced conditional gating for Anthropic thinking content block emission based on context configuration. Modified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/llm/src/protocols/anthropic/stream_converter.rs (1)
63-109:⚠️ Potential issue | 🟠 MajorConfirm fallback path silently enables thinking blocks.
Production code at
lib/llm/src/http/service/anthropic.rs:371falls back toAnthropicStreamConverter::new()whenanthropic_ctxisNone. This hardcodesemit_thinking = trueregardless of the client's actualthinkingsetting, silently bypassing the gating mechanism when request context is unavailable.Verify whether this fallback is intentional or if a safeguard is needed to respect the client's thinking preference even when context is missing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/protocols/anthropic/stream_converter.rs` around lines 63 - 109, The constructor AnthropicStreamConverter::new currently hardcodes emit_thinking = true which causes the fallback in lib/llm/src/http/service/anthropic.rs to silently enable thinking blocks when anthropic_ctx is None; change the behavior so the fallback does not assume the client's preference: either make AnthropicStreamConverter::new default emit_thinking = false (by delegating to new_with_thinking with false) or update the caller in anthropic.rs to call new_with_thinking(model, estimated_input_tokens, false) (or pass an explicit client-controlled flag) so thinking blocks are only enabled when explicitly requested; adjust with_context usage if you change the default to ensure context-driven thinking still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/llm/src/protocols/anthropic/stream_converter.rs`:
- Around line 63-109: The constructor AnthropicStreamConverter::new currently
hardcodes emit_thinking = true which causes the fallback in
lib/llm/src/http/service/anthropic.rs to silently enable thinking blocks when
anthropic_ctx is None; change the behavior so the fallback does not assume the
client's preference: either make AnthropicStreamConverter::new default
emit_thinking = false (by delegating to new_with_thinking with false) or update
the caller in anthropic.rs to call new_with_thinking(model,
estimated_input_tokens, false) (or pass an explicit client-controlled flag) so
thinking blocks are only enabled when explicitly requested; adjust with_context
usage if you change the default to ensure context-driven thinking still works.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc0a23ea-a719-4293-81a4-199dfd961e77
📒 Files selected for processing (2)
lib/llm/src/protocols/anthropic/stream_converter.rslib/llm/src/protocols/anthropic/types.rs
af23b77 to
a6b4737
Compare
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
a6b4737 to
4a81663
Compare
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
|
/ok to test d00c3ff |
|
/rerun |
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Summary
thinkingblock emission on the original Messages APIthinking.type == "enabled"request settingreasoning_contentas ordinary text when thinking is omitted or explicitly disabled, avoiding silent data loss while keeping the Anthropic block shape compatibleRoot cause
Dynamo already preserved the Anthropic request context, but the response converters always serialized parsed
reasoning_contentas Anthropicthinkingblocks. Official Anthropic docs tiethinkingcontent blocks to requests that enable extended thinking. Returning that block type when the client did not opt in breaks clients expecting Anthropic-compatible response shapes.The revised behavior only gates the block type. If a backend produces reasoning anyway, Dynamo returns it as text unless the request explicitly enabled Anthropic thinking.
Validation
cargo fmt -p dynamo-llmgit diff --checkcargo check -p dynamo-llm --no-default-features --libcargo test -p dynamo-protocols anthropiccargo test -p dynamo-llm protocols::anthropicwas attempted locally but macOS cannot complete this package test target: the default build hits Linux-only NUMA/fallocate code, and--no-default-featuresgets to link then fails on an existing native-lstdc++dependency. Linux CI should execute the added tests.Reviewers requested: @rmccorm4 and @ishandhanani.