Repository navigation
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR keeps mid-conversation Bedrock Converse system reminders in the message sequence instead of hoisting them into the cache-sensitive system prefix
Confidence Score: 4/5This PR is not safe to merge until multi-result tool ordering and the explicit regression-test and source-comment requirements are addressed The reordering emits a postponed system reminder after the first of several tool results, so downstream conversion combines ordinary text with tool-result content in a Bedrock turn Files Needing Attention: litellm/llms/bedrock/chat/converse_transformation.py
|
| Filename | Overview |
|---|---|
| litellm/llms/bedrock/chat/converse_transformation.py | Changes system-message placement and cache handling, but breaks ordering when an assistant call has multiple consecutive tool results |
Reviews (1): Last reviewed commit: "fix(bedrock): keep mid-conversation syst..." | Re-trigger Greptile
| if self._opens_with_tool_result(message): | ||
| run: Final = self._system_run_before(messages, index) | ||
| prev_idx: Final = index - len(run) - 1 | ||
| if run and prev_idx >= 0 and self._assistant_has_tool_calls(messages[prev_idx]): | ||
| return (message, *run) |
| } | ||
| return cast(AllMessageValues, converted) | ||
|
|
||
| def _transform_system_message( |
There was a problem hiding this comment.
Regression Test Is Missing This bug fix adds no regression test, violating the repository requirement to prevent this behavior from recurring unnoticed before merging
Context Used: CLAUDE.md (source)
| cache_point["ttl"] = ttl | ||
| return cache_point | ||
|
|
There was a problem hiding this comment.
Comments Violate Repository Policy This straightforward comment, plus similar comments at lines 1333 and 1366, violates the source-comment requirement and must be removed before merging
Context Used: CLAUDE.md (source)
| text_block["cache_control"] = m["cache_control"] | ||
| text_blocks.append(text_block) | ||
| converted: dict = { | ||
| "role": "user", |
There was a problem hiding this comment.
Medium: Mid-conversation system instructions lose authority
Bedrock receives this content with the user role, and the downstream message processor merges it with adjacent user turns. For applications that add authorization or safety policy reminders during a conversation, an end user can now supply conflicting instructions at the same priority; the explanatory text does not restore system-role precedence. Preserve these entries in the top-level system field, reject unsupported mid-conversation system messages, or make this lossy conversion an explicit opt-in rather than changing their trust level by default.
PR overviewThis pull request changes the Bedrock Converse transformation to retain mid-conversation system messages at their original position in the message sequence. It adjusts how these messages are represented when preparing requests for Bedrock. One security concern remains open: these system instructions are sent with user-level authority and may be merged with nearby user turns. In applications that rely on mid-conversation messages for authorization or safety policy reminders, a user could provide conflicting instructions at the same priority, weakening policy enforcement; no issues have yet been addressed. Open issues (1)
Fixed/addressed: 0 · PR risk: 4/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
8fdd415 to
cd51315
Compare
- non-dict guard in _opens_with_tool_result - in-place conversion without tool context - str/list cache_control preservation in mid-conversation path - drop unreachable non-system guard in hoisted loop
5dc43fa to
efef6f4
Compare
|
#42630 is merged and keeps mid-conversation system messages in place on the Converse path too, so closing this as agreed. Thanks for the fix. |
Problem
AmazonConverseConfig._transform_system_messagehoists EVERYrole:systemmessage to the top-level Bedrocksystemblock, regardless of position. A mid-conversation system reminder appended each turn mutates thesystemprefix every turn, so Bedrock implicit prompt caching (keyed on exact prompt prefix) never matches — full prefix re-written each request (Fixes #41043).The Invoke path fixed the same bug class via #32578/#32831 (
supports_mid_conversation_systemflag, keep in place). The Converse path never got the same treatment.Solution
Only the leading run of system entries is hoisted to the top-level
systemblock. Mid-conversation system entries are converted to user turns in place (with an operator-note prefix, mirroringAnthropicMessagesTransformation._system_role_message_as_user), because Converse only accepts user/assistant roles inmessages[]. This keeps thesystemprefix and everything before the entry byte-identical, preserving the implicit cache.Details:
cache_controlover to the user turn.Impact
bedrock/converse/*requests with per-turn system reminders now reuse the cached prefix instead of re-writing it.Evidence
tests/test_litellm/llms/bedrock/chat/test_converse_transformation.py: 261 passed.systemblocks byte-identical across turns T1->T2; downstream_bedrock_converse_messages_ptemits only user/assistant roles; tool-call/result ordering preserved;cache_controlcarried over.Fixes #41043.