Conversation
|
|
Greptile SummaryThe PR extends Anthropic request shaping to discard signed thinking blocks whose text is missing, non-string, empty, or whitespace-only, preventing malformed replayed history from reaching Anthropic-compatible endpoints.
Confidence Score: 4/5The PR appears safe to merge after the non-blocking excess-commentary issue is cleaned up. The stricter predicate consistently drops malformed empty thinking blocks while preserving valid signed and redacted blocks, and the added tests exercise the relevant request transformation without network access. Files Needing Attention: litellm/litellm_core_utils/prompt_templates/factory.py; tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/prompt_templates/factory.py | Correctly strengthens thinking-block validation, but adds substantially more explanatory commentary than the straightforward logic needs. |
| tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py | Adds meaningful regression cases in the mapped test file, though the tests contain unnecessarily extensive docstrings. |
Reviews (1): Last reviewed commit: "fix(anthropic): drop thinking blocks wit..." | Re-trigger Greptile
|
|
||
| Anthropic also rejects a `thinking` block whose `thinking` text is empty or | ||
| whitespace-only ("each thinking block must contain thinking"), regardless of | ||
| signature. This shape reaches us when a caller replays a `thinking_blocks` | ||
| history item that originated from a non-Anthropic reasoning provider (e.g. an | ||
| OpenAI Responses-API turn with no summary text) through this Anthropic-shaped | ||
| request path (`/v1/chat/completions` -> anthropic/vertex_ai's claude models), | ||
| which is the same failure the Anthropic Responses-bridge adapter guards | ||
| against (see PR #36033) for its own separate content-block path. |
There was a problem hiding this comment.
Excessive predicate commentary
The extended request-history explanation around this straightforward signature-and-text predicate, along with similarly extensive test docstrings, duplicates implementation rationale and makes the behavior harder to scan and maintain. Keep the commentary focused on the non-obvious provider constraint.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
The job exits on a workflow startup invariant check: Same failure on the latest runs for #38055, #38053 and #38051. Leaving it to whoever owns that workflow config rather than touching it from here. |
|
CI note: code-quality / check_workflow_startup_safety failure is pre-existing on main and unrelated to this PR's changed files. All 3 shards cap timeout at 55m but need 60m (35m setup + 20m pytest + 5m overhead) on both base f005afa and head; staging already fixed to 60m and passes locally, and this PR only touches factory.py. No code fix required from this PR; rebase/label will clear it. |
edc4f7f to
649aa76
Compare
649aa76 to
30b7b30
Compare
30b7b30 to
ceacb95
Compare
…st missing signature 🧠🚫 _is_unsignable_thinking_block() only checked block["signature"], so a thinking block with a valid-looking signature but empty (or whitespace-only) thinking text sailed through _drop_unsignable_thinking_blocks and into anthropic_messages_pt(). Anthropic rejects that with: 400 messages.N.content.M.thinking: each thinking block must contain thinking This is reachable whenever a thinking_blocks history item gets replayed through this Anthropic-shaped request path (e.g. a non-Anthropic reasoning turn with no summary text), the same class of bug PR BerriAI#36033 fixed on the Responses adapter's own separate code path. Now the signature check runs first (unsigned blocks are still dropped, same as before), then an additional check drops the block if `thinking` is missing, not a string, or strips to empty. redacted_thinking blocks are untouched since they don't have type == "thinking".
ceacb95 to
dfba491
Compare
TLDR
Problem this solves:
thinkingblock with a valid-looking signature but emptythinkingtext is not droppedcompletion()/acompletion()foranthropic/*andvertex_ai/claude-*How it solves it:
_is_unsignable_thinking_block()now also returnsTruewhenthinkingis missing, not a string, or empty after.strip()redacted_thinkingblocks are untouchedThe sequential-mode branch of
anthropic_messages_pt()already guards this case withlen(thinking_block) > 0and the comment "don't pass empty text blocks. anthropic api raises errors." But_drop_unsignable_thinking_blocks(), the filter used to buildthinking_blocksearlier in the same function and the only guard on theanthropic_messages_pt()->AnthropicConfig.transform_request()path, calls_is_unsignable_thinking_block()alone. The primary path was left unguarded. #36033 (Responses streaming adapter) and #27850 (Bedrock Converse) fixed the same class of bug elsewhere; this is the same fix on_is_unsignable_thinking_block().User Flow
Before: a developer whose app replays assistant history through
anthropic/claude-*orvertex_ai/claude-*gets a 400 whenever that history contains athinking_blocksentry with an emptythinkingfield (for example, forwarded from a non-Anthropic reasoning provider's turn that had no summary text)completion()call wheremessages[1]["thinking_blocks"] = [{"type": "thinking", "thinking": "", "signature": "sig_abc123"}]{"type": "thinking", "thinking": "", "signature": "sig_abc123"}400 messages.N.content.M.thinking: each thinking block must contain thinkingand the call raisesAfter: the same history no longer reaches Anthropic with an empty thinking block
thinkingis empty, independent of the signatureRelevant issues
None filed. Found by code reading in
prompt_templates/factory.pyand reproduced with a unit test (below).Pre-Submission checklist
uv run pytest tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py -v, 102 passed (97 pre-existing + 5 new)Screenshots / Proof of Fix
This is a pure request-transform bug, so the proof is a unit test calling
anthropic_messages_pt()directly with the malformed history shape and asserting the outbound content list. No live API key and no mocks; the function does no I/O.Before (f005afa)
Empty-but-signed thinking block
anthropic_messages_pt()called withmessages[1]["thinking_blocks"] = [{"type": "thinking", "thinking": "", "signature": "sig_abc123_looks_valid"}]pytest -k test_anthropic_messages_pt_drops_empty_but_signed_thinking_block-> FAILED:assert 'thinking' not in ['thinking', 'text'](the empty block was kept)Whitespace-only thinking text
_is_unsignable_thinking_block({"type": "thinking", "thinking": " \n\t ", "signature": "sig_abc123_looks_valid"})pytest -k test_is_unsignable_thinking_block_treats_whitespace_only_as_empty-> FAILED:assert False is True(the function said the block was signable)After (9fa8d06)
Empty-but-signed thinking block
pytest -k test_anthropic_messages_pt_drops_empty_but_signed_thinking_block-> PASSED,thinkingtype is absent from the outbound content listWhitespace-only thinking text
pytest -k test_is_unsignable_thinking_block_treats_whitespace_only_as_empty-> PASSED, function returnsTrueFull file:
uv run pytest tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py -v-> 102 passed, 0 failedAlso ran
ruff format --checkandruff checkon the two changed files. Did not run the fullmake test-unitsuite (left to CI per CONTRIBUTING.md) or any live Anthropic/Vertex calls.Type
Bug Fix
Test
Caveats (if any)
Low
.strip(), so whitespace-only text counts as empty. The siblinglen(thinking_block) > 0check is always combined withand not _is_unsignable_thinking_block(m), so the two agree in practice; this one is the stricter of the two because_drop_unsignable_thinking_blocks()calls it standalone.apply_additional_drop_params(). It does not touch this function.Final Attestation