fix(responses): translate 'developer' role to 'system' when bridging to Chat Completions#24728
Conversation
…atency async logger Signed-off-by: NIK-TIGER-BILL <nik.tiger.bill@github.com>
…to Chat Completions Signed-off-by: NIK-TIGER-BILL <nik.tiger.bill@github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
NIK-TIGER-BILL 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 makes two unrelated fixes: (1) in the Responses API → Chat Completions bridge, it translates the Key changes:
Issues found:
Confidence Score: 4/5Safe to merge after the missing unit test for the Responses API bridge path is added; the core fix is correct. The role translation fix is minimal and correct. The async locking improvement is sound. However, the PR explicitly claims a unit test was added (checked in the checklist) that does not exist in the repository — leaving the specific fixed code path without any direct test coverage. The remaining findings (unbounded lock dict, unguarded sync path) are P2 quality concerns that don't block merge. Both changed files are straightforward, but
|
| Filename | Overview |
|---|---|
| litellm/responses/litellm_completion_transformation/transformation.py | Adds a one-line developer → system role translation in _transform_responses_api_input_item_to_chat_completion_message; fix is correct but the promised unit test covering this exact code path is absent from the PR. |
| litellm/router_strategy/lowest_latency.py | Introduces per-key asyncio.Lock guards around the read-modify-write cycle in async_log_failure_event and async_log_success_event to prevent lost updates under concurrency; the sync log_success_event path remains unguarded and the lock dict has no eviction. |
Sequence Diagram
sequenceDiagram
participant Client
participant ResponsesAPI as /v1/responses
participant Bridge as LiteLLMCompletionResponsesConfig
participant Provider as Non-OpenAI Provider
Client->>ResponsesAPI: POST /v1/responses<br/>{role: "developer", content: "..."}
ResponsesAPI->>Bridge: _transform_responses_api_input_item_to_chat_completion_message(item)
Note over Bridge: raw_role = "developer"<br/>role = "system" ← NEW FIX
Bridge-->>ResponsesAPI: GenericChatCompletionMessage(role="system", ...)
ResponsesAPI->>Provider: Chat Completions request<br/>{role: "system", content: "..."}
Provider-->>Client: Response ✓
Comments Outside Diff (1)
-
litellm/router_strategy/lowest_latency.py, line 134-189 (link)Synchronous
log_success_eventis not protected by any lockasync_log_success_eventandasync_log_failure_eventnow guard their read-modify-write onrequest_count_dictwith_get_async_lock, but the synchronouslog_success_event(lines 134-189) performs the same read-modify-write without any locking. If the sync path and the async path can be called concurrently (e.g. viarun_in_executoror mixed sync/async callers), updates from the sync path can still be lost. If the sync path is only ever called from a true synchronous (non-asyncio) context where interleaving is impossible, a brief code comment explaining that invariant would prevent future regressions.
Reviews (1): Last reviewed commit: "fix(responses): translate 'developer' ro..." | Re-trigger Greptile
| raw_role = input_item.get("role") or "user" | ||
| # Responses API uses "developer" as an alias for "system". | ||
| # Non-OpenAI providers only accept "system", so translate it here. | ||
| role = "system" if raw_role == "developer" else raw_role |
There was a problem hiding this comment.
Claimed test is absent from the PR
The PR description states "Added unit test test_developer_role_translated_to_system covering the developer → system mapping", and the checklist marks "I have added a test for the fix" as done. However, no such test exists anywhere in the repository — the diff touches only transformation.py and lowest_latency.py, and searching the entire test tree for test_developer_role_translated_to_system returns no matches.
The existing test_developer_role_translation in tests/llm_translation/base_llm_unit_tests.py exercises the Chat Completions provider path, not the Responses API → Chat Completions bridge that this fix targets. A dedicated unit test for _transform_responses_api_input_item_to_chat_completion_message with a developer-role input item is still missing.
Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
| def _get_async_lock(self, key: str) -> asyncio.Lock: | ||
| """Return a per-key asyncio.Lock, creating it on first access.""" | ||
| if key not in self._async_locks: | ||
| self._async_locks[key] = asyncio.Lock() | ||
| return self._async_locks[key] |
There was a problem hiding this comment.
_async_locks dict grows unboundedly
_async_locks is keyed by latency_key = f"{model_group}_map" and is never pruned. In long-running deployments with many model groups (or dynamically added/removed ones), this dict will accumulate stale asyncio.Lock objects indefinitely. Consider bounding the size or evicting entries when a model group is removed.
Problem
Fixes #24664
When a client sends a Responses API request (
/v1/responses) with messages using thedeveloperrole, the Responses API → Chat Completions bridge passes the role through unchanged. Non-OpenAI providers (vLLM, Anthropic, etc.) only recognisesystem,user,assistant, andtool, so they reject the request with"Unexpected message role".Root Cause
In
_transform_responses_api_input_item_to_chat_completion_message, the role from the input item is forwarded verbatim:The
instructionsfield is already correctly translated tosystemviatransform_instructions_to_system_message, but input items in the conversation history that carryrole: "developer"were not.Fix
Add a one-line translation: map
"developer"→"system"before constructing theGenericChatCompletionMessage.This matches OpenAI's own documentation which states that
developeris semantically equivalent tosystem.Testing
test_developer_role_translated_to_systemcovering thedeveloper → systemmappingChecklist