Conversation
|
Genmin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Greptile SummaryThis PR refactors
Confidence Score: 3/5Not safe to merge as-is; the change silently breaks message ordering for any caller that had interleaved system messages alongside developer messages. A P1 backwards-incompatible behavioral change in a shared utility pulls the score below the P1 ceiling; the fix is targeted but lacks a feature flag, and the missing test case (developer message after a user turn) leaves the regression risk unverified. litellm/llms/base_llm/base_utils.py — the core merge logic and its impact on callers with interleaved system/developer messages.
|
| Filename | Overview |
|---|---|
| litellm/llms/base_llm/base_utils.py | Refactors map_developer_role_to_system_role to merge all system/developer messages into a single leading system message; introduces a backwards-incompatible reordering for existing callers that have interleaved system messages. |
| tests/llm_translation/test_base_llm_base_utils.py | New test file covering the revised merge logic; tests are mock-only and cover core scenarios, but does not test the case where developer messages appear after user messages (the primary backwards-compat risk). |
Reviews (1): Last reviewed commit: "fix: merge developer role into leading s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for m in messages: | ||
| if m["role"] in {"developer", "system"}: | ||
| if system_message is None: | ||
| system_message = dict(m) | ||
| system_message["role"] = "system" | ||
| system_contents.append(m["content"]) | ||
| else: | ||
| non_system_messages.append(m) | ||
|
|
||
| if m["role"] == "developer": | ||
| verbose_logger.debug( | ||
| "Translating developer role to system role for non-OpenAI providers." | ||
| ) # ensure user knows what's happening with their input. | ||
| new_messages.append({"role": "system", "content": m["content"]}) | ||
| else: | ||
| new_messages.append(m) | ||
| return new_messages | ||
|
|
||
| if system_message is None: | ||
| return non_system_messages | ||
|
|
||
| system_message["content"] = _merge_system_message_contents(system_contents) | ||
| return [cast(AllMessageValues, system_message), *non_system_messages] |
There was a problem hiding this comment.
The new implementation moves every system/developer message — regardless of its original position — to the front as a single merged block. This silently reorders conversations for any existing caller that had interleaved system messages. Consider a prior call with [system("A"), user("Q"), developer("B")]:
- Before:
[system("A"), user("Q"), system("B")]— developer converted in place, user message stays between two system messages - After:
[system("A\n\nB"), user("Q")]— the developer message is no longer after the user turn
Similarly, multiple developer-only messages that previously resulted in multiple system messages at their original positions now collapse into one at the front. Per the backwards-compatibility rule, behavioral changes like this should be gated behind a feature flag rather than applied unconditionally to all callers.
Rule Used: What: avoid backwards-incompatible changes without... (source)
| if merged_blocks: | ||
| merged_blocks.append({"type": "text", "text": "\n\n"}) | ||
| merged_blocks.extend(content_blocks) |
There was a problem hiding this comment.
Inserting {"type": "text", "text": "\n\n"} as a dedicated content block between merged segments is non-standard. Some providers validate or render each content block independently, and a block that contains only whitespace can result in empty rendered content or validation errors. A more portable approach is to append the separator to the trailing text of the preceding block (or to the leading text of the next block) instead of injecting it as its own element.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Updated this to address the review concern and the patch coverage gate. The merge now only collapses the leading system/developer block that Responses API creates; any developer messages after a user turn are converted to system in place, preserving the prior ordering behavior. I also moved the focused tests into the core-utils test shard so Codecov should receive coverage for the changed utility. |
|
Added the one missing coverage case Codecov was still flagging: a non-leading system message is now covered alongside the later developer-role conversion path. Local validation:
|
…ct, terminal LaTeX - BerriAI/litellm#26888: leading developer/system run merge for Responses API (merge-as-is) - BerriAI/litellm#26887: drop search tools at append-point when mixed with function decls (merge-as-is) - QwenLM/qwen-code#3439: terminal LaTeX renderer with width-calc hook (needs-discussion)
|
Closing in favor of #39282, which merges translated developer messages into one leading system message and also covers the Codex first turn. |
3 participants
Summary
Fixes #26879
Tests