-
Notifications
You must be signed in to change notification settings - Fork 55
fix(api): forward vendor fields on chat messages and completions #833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
4b081d3
0bdb8a1
b36a685
dc8fd14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,64 @@ def test_chat_request_passes_through_extra_openai_params(): | |
| assert dump["seed"] == 42 | ||
|
|
||
|
|
||
| def test_chat_message_passes_through_extra_openai_fields(): | ||
| """An OpenAI message is more than role/content: `name` disambiguates speakers | ||
| and `tool_calls`/`tool_call_id` carry function calling. Dropping them here | ||
| silently truncated the history sent to the LLM | ||
| """ | ||
| request = OpenAIChatCompletionRequest.model_validate( | ||
| { | ||
| "messages": [ | ||
| {"role": "user", "content": "hi", "name": "alice"}, | ||
| { | ||
| "role": "assistant", | ||
| "content": "", | ||
| "tool_calls": [{"id": "c1", "type": "function", "function": {"name": "f", "arguments": "{}"}}], | ||
| }, | ||
| ] | ||
| } | ||
| ) | ||
| messages = request.model_dump(exclude_none=True)["messages"] | ||
|
|
||
| assert messages[0]["name"] == "alice" | ||
| assert messages[1]["tool_calls"][0]["id"] == "c1" | ||
|
Comment on lines
+117
to
+137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Files matching target:"
git ls-files 'tests/unit/api/schemas/test_api_schema_imports.py' || true
if [ -f tests/unit/api/schemas/test_api_schema_imports.py ]; then
echo
echo "Outline/target section:"
wc -l tests/unit/api/schemas/test_api_schema_imports.py
sed -n '1,220p' tests/unit/api/schemas/test_api_schema_imports.py | cat -n
fiRepository: linagora/openrag Length of output: 10310 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Chat schema files:"
fd -a 'chat\.py$' openrag api tests | sed 's#^\./##' | sort
echo
for f in $(fd 'chat\.py$' openrag | sort); do
echo "--- $f ($(wc -l < "$f") lines) ---"
cat -n "$f"
done
echo
echo "References to OpenAIMessage / tool_call_id / function_call in api.schemas:"
rg -n "OpenAIMessage|tool_call_id|function_call|name|extra|model_config|ConfigDict" api/schemas -g '*.py'Repository: linagora/openrag Length of output: 37832 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Pydantic availability/version:"
python3 - <<'PY'
try:
import pydantic
import pydantic.version
print(pydantic.__version__, pydantic.version.version_info())
except Exception as e:
print("pydantic import failed:", type(e).__name__, str(e))
PY
echo
if python3 - <<'PY'
import pydantic
print(pydantic.__version__)
PY
then
python3 - <<'PY'
from pydantic import BaseModel, ConfigDict
import json
class Inner(BaseModel):
model_config = ConfigDict(extra="allow")
role: str
content: str
class Request(BaseModel):
model_config = ConfigDict(extra="allow")
messages: list[Inner]
data = {
"messages": [
{"role": "user", "content": "hi", "name": "alice"},
{
"role": "assistant",
"content": "",
"tool_calls": [
{
"id": "c1",
"type": "function",
"tool_call_id": "t1",
"function": {"name": "f", "arguments": "{}"}
}
],
"function_call": {"name": "g"}
},
{"role": "tool", "tool_call_id": "t1", "content": "response"},
]
}
request = Request.model_validate(data)
dump = request.model_dump(exclude_none=True)
print(json.dumps(dump, indent=2))
checks = {
"name": dump["messages"][0].get("name"),
"tool_call_id": dump["messages"][1]["tool_calls"][0].get("tool_call_id"),
"function_call": dump["messages"][1].get("function_call"),
"nested_name": dump["messages"][1]["tool_calls"][0]["function"].get("name"),
"nested_arguments": dump["messages"][1]["tool_calls"][0]["function"].get("arguments"),
"tool_role_call_id": dump["messages"][2].get("tool_call_id"),
}
print("checks:", checks)
PY
fiRepository: linagora/openrag Length of output: 378 🌐 Web query:
💡 Result: In the OpenAI Chat Completions API, the interaction between tool calls and assistant messages is managed through specific parameters and message roles [1][2]. The Citations:
🌐 Web query:
💡 Result:
If you want, I can show the exact JSON shape for a Citations: 🌐 Web query:
💡 Result: In the OpenAI Python Chat Completions API, when providing the result of a tool call back to the model, you must use a message with the role set to "tool" [1][2]. For this "tool" role message, the following fields are required: 1. role: Must be the literal string "tool" [1][3]. 2. tool_call_id: A string matching the id of the specific tool call you are responding to [1][2][4]. 3. content: The output of the tool execution (as a string) [1][2]. Additionally, while not strictly listed as a required parameter in some schema definitions compared to the three above, it is standard practice to include the name field (the name of the tool function that was called) in your message dictionary to ensure correct identification by the model [2][5][4]. Example structure for appending a tool result to your messages list: messages.append({ "role": "tool", "tool_call_id": tool_call.id, "name": tool_call.function.name, "content": json.dumps(function_result) }) It is important to note that a message with the role "tool" must directly follow the assistant's message that requested the tool call, and the tool_call_id must strictly correspond to the id provided in that assistant message [2][4]. Attempting to reuse an object type like ChatCompletionMessage (which is typically for assistant responses) for this "tool" role message will often result in a validation error, as the role "tool" is specifically required [5]. Citations:
Cover the full message passthrough contract. This test only checks 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| def test_sanitize_messages_keeps_tool_calls_reaching_it(): | ||
| """_sanitize_messages leaves a content-free assistant turn alone when it | ||
| carries tool_calls — reachable only now that the schema forwards the field | ||
| """ | ||
| from services.orchestrators.query_service import QueryService | ||
|
|
||
| request = OpenAIChatCompletionRequest.model_validate( | ||
| { | ||
| "messages": [ | ||
| { | ||
| "role": "assistant", | ||
| "content": "", | ||
| "tool_calls": [{"id": "c1", "type": "function", "function": {"name": "f", "arguments": "{}"}}], | ||
| } | ||
| ] | ||
| } | ||
| ) | ||
| sanitized = QueryService._sanitize_messages(request.model_dump(exclude_none=True)["messages"]) | ||
|
|
||
| assert sanitized[0]["content"] == "" | ||
|
|
||
|
|
||
| def test_completion_request_passes_through_extra_openai_params(): | ||
| """Legacy /completions mirrors the chat request: undeclared vendor params are | ||
| forwarded, while the declared bounds on n/best_of still apply | ||
| """ | ||
| request = OpenAICompletionRequest.model_validate({"prompt": "hi", "suffix": "!", "user": "alice"}) | ||
| dump = request.model_dump(exclude_none=True) | ||
|
|
||
| assert dump["suffix"] == "!" | ||
| assert dump["user"] == "alice" | ||
| with pytest.raises(ValidationError): | ||
| OpenAICompletionRequest.model_validate({"prompt": "hi", "n": 9, "user": "alice"}) | ||
|
|
||
|
|
||
| def test_completion_request_omits_unset_nulls(): | ||
| """The /completions router dumps with exclude_none=True (matching chat), so | ||
| optional params left unset are not sent as explicit null to strict providers | ||
|
|
@@ -167,3 +225,74 @@ def test_completion_request_bounds_n_and_best_of(): | |
| for bad in ({"n": 0}, {"n": 9}, {"best_of": 0}, {"best_of": 9}): | ||
| with pytest.raises(ValidationError): | ||
| OpenAICompletionRequest(prompt="x", **bad) | ||
|
|
||
|
|
||
| def test_chat_message_accepts_tool_role_with_tool_call_id(): | ||
| """A tool-result turn is `role="tool"` + `tool_call_id`. `extra="allow"` only | ||
| preserves undeclared fields *after* the declared ones validate, so an | ||
| unlisted role rejected the whole message before its extras mattered | ||
| """ | ||
| message = OpenAIMessage.model_validate({"role": "tool", "content": "42", "tool_call_id": "c1"}) | ||
| dump = message.model_dump() | ||
|
|
||
| assert dump["role"] == "tool" | ||
| assert dump["tool_call_id"] == "c1" | ||
|
|
||
|
|
||
| def test_chat_message_accepts_null_content_with_tool_calls(): | ||
| """The assistant turn that *carries* tool_calls has `content: null` in the | ||
| OpenAI API — the exact shape `_sanitize_messages` documents as legitimately | ||
| content-free. A required `content: str` rejected it before it got there | ||
| """ | ||
| message = OpenAIMessage.model_validate( | ||
| { | ||
| "role": "assistant", | ||
| "content": None, | ||
| "tool_calls": [{"id": "c1", "type": "function", "function": {"name": "f", "arguments": "{}"}}], | ||
| } | ||
| ) | ||
|
|
||
| assert message.content is None | ||
| assert message.model_dump()["tool_calls"][0]["id"] == "c1" | ||
|
|
||
|
|
||
| def test_chat_message_accepts_developer_role(): | ||
| """`developer` is OpenAI's replacement for `system` on newer models; rejecting | ||
| it 422s a request the downstream LLM would have accepted | ||
| """ | ||
| assert OpenAIMessage.model_validate({"role": "developer", "content": "be terse"}).role == "developer" | ||
|
|
||
|
|
||
| def test_chat_request_accepts_replayed_tool_call_history(): | ||
| """The realistic end-to-end shape: a client replaying a conversation that | ||
| already used tools, then asking a new question. Every intermediate turn must | ||
| survive parsing for the history reaching the LLM to stay faithful | ||
| """ | ||
| request = OpenAIChatCompletionRequest.model_validate( | ||
| { | ||
| "messages": [ | ||
| {"role": "user", "content": "weather in Paris?"}, | ||
| { | ||
| "role": "assistant", | ||
| "content": None, | ||
| "tool_calls": [ | ||
| { | ||
| "id": "c1", | ||
| "type": "function", | ||
| "function": {"name": "get_weather", "arguments": '{"c":"Paris"}'}, | ||
| } | ||
| ], | ||
| }, | ||
| {"role": "tool", "content": "18C", "tool_call_id": "c1"}, | ||
| {"role": "assistant", "content": "It's 18C in Paris."}, | ||
| {"role": "user", "content": "and tomorrow?"}, | ||
| ] | ||
| } | ||
| ) | ||
| messages = request.model_dump(exclude_none=True)["messages"] | ||
|
|
||
| assert [m["role"] for m in messages] == ["user", "assistant", "tool", "assistant", "user"] | ||
| assert messages[1]["tool_calls"][0]["function"]["name"] == "get_weather" | ||
| assert messages[2]["tool_call_id"] == "c1" | ||
| # exclude_none drops the null content rather than forwarding `content: null` | ||
| assert "content" not in messages[1] | ||
Uh oh!
There was an error while loading. Please reload this page.