From 4b081d35ef4f5487f2da5c34d304dbd912453b09 Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Tue, 28 Jul 2026 15:30:02 +0200 Subject: [PATCH 1/4] fix(api): forward vendor fields on chat messages and completions --- openrag/api/schemas/user/chat.py | 13 +++++ .../api/schemas/test_api_schema_imports.py | 58 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/openrag/api/schemas/user/chat.py b/openrag/api/schemas/user/chat.py index 4ea5ab7e0..d7f0a20eb 100644 --- a/openrag/api/schemas/user/chat.py +++ b/openrag/api/schemas/user/chat.py @@ -4,6 +4,14 @@ class OpenAIMessage(BaseModel): + # Same passthrough policy as the request below, applied per message. An + # OpenAI message carries more than role/content — `name`, `tool_calls`, + # `function_call`, `tool_call_id` — and pydantic's default `extra="ignore"` + # dropped them here, before the router dumped the payload, so they never + # reached the LLM. `QueryService._sanitize_messages` already branches on + # `tool_calls`/`function_call`, which until now could not survive parsing. + model_config = ConfigDict(extra="allow") + role: Literal["user", "assistant", "system"] content: str @@ -61,6 +69,11 @@ def _ignore_top_logprobs_without_logprobs(self) -> "OpenAIChatCompletionRequest" class OpenAICompletionRequest(BaseModel): + # Mirrors OpenAIChatCompletionRequest: forward vendor-specific params rather + # than silently dropping them. The bounds below still apply — `extra="allow"` + # only admits *undeclared* keys, so `n`/`best_of` stay validated. + model_config = ConfigDict(extra="allow") + model: str | None = Field(None, description="model name") prompt: str # Bound n/best_of: each multiplies generation cost, so leaving them unbounded diff --git a/tests/unit/api/schemas/test_api_schema_imports.py b/tests/unit/api/schemas/test_api_schema_imports.py index 306350bb2..51a6e887e 100644 --- a/tests/unit/api/schemas/test_api_schema_imports.py +++ b/tests/unit/api/schemas/test_api_schema_imports.py @@ -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" + + +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 From 0bdb8a13164f445c203d5ad003c9b8a482c3c41e Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Wed, 29 Jul 2026 16:08:41 +0200 Subject: [PATCH 2/4] fix(api): keep message extras out of the chat request debug log --- openrag/api/routers/user/chat.py | 18 ++++++- .../api/routers/user/test_chat_logging.py | 48 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 tests/unit/api/routers/user/test_chat_logging.py diff --git a/openrag/api/routers/user/chat.py b/openrag/api/routers/user/chat.py index 9e315d110..63020dea1 100644 --- a/openrag/api/routers/user/chat.py +++ b/openrag/api/routers/user/chat.py @@ -28,7 +28,7 @@ truncate, ) from api.routers.user.source_links import build_document_source_link -from api.schemas.user.chat import OpenAIChatCompletionRequest, OpenAICompletionRequest +from api.schemas.user.chat import OpenAIChatCompletionRequest, OpenAICompletionRequest, OpenAIMessage from core.config import load_config from core.models.preset import resolve_partition_chat_llm from core.utils.exceptions import OpenRAGError @@ -427,6 +427,20 @@ def check_tokens_limit( ) +def _loggable_messages(messages: list[OpenAIMessage]) -> str: + """Render messages for the debug log using only the declared fields. + + ``OpenAIMessage`` sets ``extra="allow"`` so vendor keys and tool-call + payloads (``tool_calls``, ``function_call``, ``tool_call_id``) survive + parsing and reach the LLM. That passthrough is a transport concern: it must + not also widen what we retain in logs, where tool-call *arguments* — often + the structured, sensitive half of a conversation — would land verbatim. + Project each message back down to role/content, the surface this log + already had before the passthrough, and keep the existing length bound. + """ + return truncate(str([m.model_dump(include={"role", "content"}) for m in messages])) + + @router.post( "/chat/completions", summary="OpenAI compatible chat completion endpoint using RAG", @@ -471,7 +485,7 @@ async def openai_chat_completion( detail="The last message must be a non-empty user message", ) - log.debug("Received chat completion request with messages: {}", truncate(str(request.messages))) + log.debug("Received chat completion request with messages: {}", _loggable_messages(request.messages)) if is_direct_llm_model(request, config): partitions = None diff --git a/tests/unit/api/routers/user/test_chat_logging.py b/tests/unit/api/routers/user/test_chat_logging.py new file mode 100644 index 000000000..0711fd523 --- /dev/null +++ b/tests/unit/api/routers/user/test_chat_logging.py @@ -0,0 +1,48 @@ +"""The chat request debug log must not widen with the message passthrough. + +``OpenAIMessage`` accepts extras (``tool_calls``, ``function_call``, +``tool_call_id``, vendor keys) so they survive parsing and reach the LLM. +That passthrough is a transport concern: it must not also enlarge what the +router writes to logs, where tool-call arguments would otherwise be retained. +""" + +from api.routers.user.chat import _loggable_messages +from api.schemas.user.chat import OpenAIMessage + + +def test_extras_are_not_logged(): + """Fields admitted only by `extra="allow"` stay out of the log line.""" + message = OpenAIMessage( + role="assistant", + content="calling a tool", + tool_calls=[ + { + "id": "call_1", + "type": "function", + "function": {"name": "transfer", "arguments": '{"iban": "FR7630001007941234567890185"}'}, + } + ], + vendor_field={"internal": "secret"}, + ) + + rendered = _loggable_messages([message]) + + assert "tool_calls" not in rendered + assert "FR7630001007941234567890185" not in rendered + assert "vendor_field" not in rendered + + +def test_declared_fields_are_still_logged(): + """Role and content keep their pre-existing debugging value.""" + rendered = _loggable_messages([OpenAIMessage(role="user", content="what is openrag?")]) + + assert "user" in rendered + assert "what is openrag?" in rendered + + +def test_output_is_truncated(): + """The log line stays bounded regardless of message size.""" + rendered = _loggable_messages([OpenAIMessage(role="user", content="x" * 5000)]) + + assert "[truncated" in rendered + assert len(rendered) < 1200 From b36a685da0fd3cdff4c82739862d3b1bc4abc79d Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Wed, 29 Jul 2026 17:58:43 +0200 Subject: [PATCH 3/4] Revert "fix(api): keep message extras out of the chat request debug log" This reverts commit 0bdb8a13164f445c203d5ad003c9b8a482c3c41e. --- openrag/api/routers/user/chat.py | 18 +------ .../api/routers/user/test_chat_logging.py | 48 ------------------- 2 files changed, 2 insertions(+), 64 deletions(-) delete mode 100644 tests/unit/api/routers/user/test_chat_logging.py diff --git a/openrag/api/routers/user/chat.py b/openrag/api/routers/user/chat.py index 63020dea1..9e315d110 100644 --- a/openrag/api/routers/user/chat.py +++ b/openrag/api/routers/user/chat.py @@ -28,7 +28,7 @@ truncate, ) from api.routers.user.source_links import build_document_source_link -from api.schemas.user.chat import OpenAIChatCompletionRequest, OpenAICompletionRequest, OpenAIMessage +from api.schemas.user.chat import OpenAIChatCompletionRequest, OpenAICompletionRequest from core.config import load_config from core.models.preset import resolve_partition_chat_llm from core.utils.exceptions import OpenRAGError @@ -427,20 +427,6 @@ def check_tokens_limit( ) -def _loggable_messages(messages: list[OpenAIMessage]) -> str: - """Render messages for the debug log using only the declared fields. - - ``OpenAIMessage`` sets ``extra="allow"`` so vendor keys and tool-call - payloads (``tool_calls``, ``function_call``, ``tool_call_id``) survive - parsing and reach the LLM. That passthrough is a transport concern: it must - not also widen what we retain in logs, where tool-call *arguments* — often - the structured, sensitive half of a conversation — would land verbatim. - Project each message back down to role/content, the surface this log - already had before the passthrough, and keep the existing length bound. - """ - return truncate(str([m.model_dump(include={"role", "content"}) for m in messages])) - - @router.post( "/chat/completions", summary="OpenAI compatible chat completion endpoint using RAG", @@ -485,7 +471,7 @@ async def openai_chat_completion( detail="The last message must be a non-empty user message", ) - log.debug("Received chat completion request with messages: {}", _loggable_messages(request.messages)) + log.debug("Received chat completion request with messages: {}", truncate(str(request.messages))) if is_direct_llm_model(request, config): partitions = None diff --git a/tests/unit/api/routers/user/test_chat_logging.py b/tests/unit/api/routers/user/test_chat_logging.py deleted file mode 100644 index 0711fd523..000000000 --- a/tests/unit/api/routers/user/test_chat_logging.py +++ /dev/null @@ -1,48 +0,0 @@ -"""The chat request debug log must not widen with the message passthrough. - -``OpenAIMessage`` accepts extras (``tool_calls``, ``function_call``, -``tool_call_id``, vendor keys) so they survive parsing and reach the LLM. -That passthrough is a transport concern: it must not also enlarge what the -router writes to logs, where tool-call arguments would otherwise be retained. -""" - -from api.routers.user.chat import _loggable_messages -from api.schemas.user.chat import OpenAIMessage - - -def test_extras_are_not_logged(): - """Fields admitted only by `extra="allow"` stay out of the log line.""" - message = OpenAIMessage( - role="assistant", - content="calling a tool", - tool_calls=[ - { - "id": "call_1", - "type": "function", - "function": {"name": "transfer", "arguments": '{"iban": "FR7630001007941234567890185"}'}, - } - ], - vendor_field={"internal": "secret"}, - ) - - rendered = _loggable_messages([message]) - - assert "tool_calls" not in rendered - assert "FR7630001007941234567890185" not in rendered - assert "vendor_field" not in rendered - - -def test_declared_fields_are_still_logged(): - """Role and content keep their pre-existing debugging value.""" - rendered = _loggable_messages([OpenAIMessage(role="user", content="what is openrag?")]) - - assert "user" in rendered - assert "what is openrag?" in rendered - - -def test_output_is_truncated(): - """The log line stays bounded regardless of message size.""" - rendered = _loggable_messages([OpenAIMessage(role="user", content="x" * 5000)]) - - assert "[truncated" in rendered - assert len(rendered) < 1200 From dc8fd14550b0a3d308c3bd4f46d1278933e30b9c Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Wed, 29 Jul 2026 18:02:40 +0200 Subject: [PATCH 4/4] fix(api): accept tool/developer roles and null message content --- openrag/api/schemas/user/chat.py | 17 ++++- .../api/schemas/test_api_schema_imports.py | 71 +++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/openrag/api/schemas/user/chat.py b/openrag/api/schemas/user/chat.py index d7f0a20eb..07ec382b3 100644 --- a/openrag/api/schemas/user/chat.py +++ b/openrag/api/schemas/user/chat.py @@ -12,8 +12,21 @@ class OpenAIMessage(BaseModel): # `tool_calls`/`function_call`, which until now could not survive parsing. model_config = ConfigDict(extra="allow") - role: Literal["user", "assistant", "system"] - content: str + # `tool` carries a function result (paired with the `tool_call_id` extra); + # `developer` is OpenAI's replacement for `system` on newer models. Both are + # valid OpenAI roles, and `extra="allow"` could not rescue them: extras are + # only preserved *after* the declared fields validate, so an unlisted role + # 422'd the whole request before `tool_call_id` ever mattered. Nothing + # downstream matches on role exhaustively — `_sanitize_messages` only tests + # for `assistant` — so unknown roles pass through to the LLM untouched. + role: Literal["user", "assistant", "system", "tool", "developer"] + # Nullable because the assistant turn that *carries* `tool_calls` has no + # content in the OpenAI API — the case `_sanitize_messages` documents as + # "legitimately content-free and left untouched", previously unreachable + # since a required `str` rejected it first. The router still enforces that + # the *last* message is a non-empty user turn, so the RAG path's + # ``messages[-1]["content"]`` stays safe. + content: str | None = None class OpenAIChatCompletionRequest(BaseModel): diff --git a/tests/unit/api/schemas/test_api_schema_imports.py b/tests/unit/api/schemas/test_api_schema_imports.py index 51a6e887e..5c1793313 100644 --- a/tests/unit/api/schemas/test_api_schema_imports.py +++ b/tests/unit/api/schemas/test_api_schema_imports.py @@ -225,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]