fix: merge stray top-level system kwarg into messages array in Chat Completions transport (#60821) - #60891
fix: merge stray top-level system kwarg into messages array in Chat Completions transport (#60821)#60891kyssta-exe wants to merge 1 commit into
Conversation
…ompletions transport (NousResearch#60821)
falkoro
left a comment
There was a problem hiding this comment.
Community review — verified locally. Direction is correct: a top-level system kwarg is invalid for Chat Completions and merging it into the messages array is the right recovery. Also checked the mutation concern: sanitized comes from convert_messages(), which deep-copies per call, so the in-place insert(0, …) is safe.
Three things worth tightening:
- The guard
elif k == "system" and sanitized:means an empty messages list still falls through toapi_kwargs["system"] = v— the original 400 in that edge.sanitized.insert(0, …)works fine on an empty list, so theand sanitizedcan drop (with the exact-match branch guarded byif sanitized and sanitized[0].get("role") == "system"). - System message
contentmay be a content-parts list rather than a string on some providers;(list or "") + "\n"raisesTypeError. Anisinstance(content, str)check (falling back to inserting a separate system message) covers it. - No tests — this is exactly the kind of fix a small unit test pins cheaply (overrides with
system+ existing system message / no system message / empty messages).
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (token read-only)
PR 60891 merges a stray top-level system kwarg into the messages array in Chat Completions transport. Fixes a protocol handling edge case. Well-scoped (1 file, 14 additions). No security issues or debug artifacts detected.
LGTM - awaiting maintainer approval.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the narrowly scoped defensive fix. It needs rework before it can reliably address the reported failure.
Problems
- The confirmed #60821 injector was
llm_requestmiddleware. Current main builds transport kwargs atagent/conversation_loop.py:1161, then middleware replaces them atagent/conversation_loop.py:1175-1192; this PR only handlesrequest_overrides, so it does not intercept that path. - The added
and sanitizedguard leavesmessages=[]on the existing fallback that forwardssystemintoapi_kwargs(agent/transports/chat_completions.py:601-608). - The merge expression assumes string content. Existing system messages may use content parts (
tests/providers/test_provider_profiles.py:471), for which list-plus-string raisesTypeError.
Suggested changes
- Re-scope the reported fix to the post-middleware boundary or present this as separate
request_overrideshardening. - Handle empty messages and content-part system messages without forwarding a top-level
systemkwarg. - Add regression tests in
tests/agent/transports/test_chat_completions.pyfor text, absent, empty, and content-parts system-message cases.
Automated hermes-sweeper review.
| for k, v in overrides.items(): | ||
| if k == "extra_body" and isinstance(v, dict): | ||
| extra_body.update(v) | ||
| elif k == "system" and sanitized: |
There was a problem hiding this comment.
and sanitized leaves an empty messages list on the final else, which still forwards the invalid top-level system kwarg. Handle the empty-list case by inserting a system message too.
| if sanitized[0].get("role") == "system": | ||
| sanitized[0] = { | ||
| **sanitized[0], | ||
| "content": (sanitized[0].get("content") or "") |
There was a problem hiding this comment.
System content can be a non-empty content-parts list (tests/providers/test_provider_profiles.py:471); adding "\n" to it raises TypeError. Merge only strings, or insert a separate system message for non-string content.
|
Stale — no merge activity for 4-6 days. Can resubmit if still needed. |
When request_overrides or a provider profile accidentally forwards system as a top-level OpenAI Chat Completions kwarg, merge it into the messages array instead of crashing.