Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions openrag/api/schemas/user/chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,15 @@


class OpenAIMessage(BaseModel):
role: Literal["user", "assistant", "system"]
content: str
# Allow to have extra openAI attributes, like `tool_calls`,
# `function_call`, etc. Pydantic's default `extra="ignore"`
# drops them.
model_config = ConfigDict(extra="allow")

role: Literal["user", "assistant", "system", "tool", "developer"]

# content can be None when using `tool_calls`
content: str | None = None


class OpenAIChatCompletionRequest(BaseModel):
Expand Down Expand Up @@ -63,6 +70,9 @@ def _ignore_top_logprobs_without_logprobs(self) -> "OpenAIChatCompletionRequest"


class OpenAICompletionRequest(BaseModel):
# Mirrors OpenAIChatCompletionRequest
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
Expand Down
12 changes: 10 additions & 2 deletions openrag/services/orchestrators/query_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ async def generate_query(
if RAGMODE(self._rag_mode) is RAGMODE.SIMPLERAG:
return SearchQueries(query_list=[Query(query=last_user)])

chat_history = "".join(f"{m['role']}: {m['content']}\n" for m in messages)
chat_history = "".join(f"{m['role']}: {m.get('content') or ''}\n" for m in messages)
contextualizer = await self._prompt_service.resolve_prompt(
"query_contextualizer",
names=[self._retrieval_prompt_name("query_contextualizer_prompt_name", partition)],
Expand Down Expand Up @@ -857,11 +857,19 @@ def _split_leading_system_prompt(raw_messages: list[dict], truncated: list[dict]
portion of that leading run still inside the tail is stripped from it — a
system message elsewhere in history that merely lands first after
chat_history_depth truncation is never mistaken for the pin and dropped.

``content`` is read defensively: ``OpenAIMessage`` allows a null/absent
content (the assistant turn carrying ``tool_calls``), and the router dumps
with ``exclude_none=True``, so the key is genuinely optional. A content-free
system message still counts toward the leading run — it is stripped from the
history like its siblings — it just contributes nothing to the pin.
"""
parts: list[str] = []
i = 0
while i < len(raw_messages) and raw_messages[i]["role"] == "system":
parts.append(raw_messages[i]["content"])
content = raw_messages[i].get("content")
Comment thread
Ahmath-Gadji marked this conversation as resolved.
if content:
parts.append(content)
i += 1

offset = len(raw_messages) - len(truncated)
Expand Down
129 changes: 129 additions & 0 deletions tests/unit/api/schemas/test_api_schema_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 thread
coderabbitai[bot] marked this conversation as resolved.


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
Expand Down Expand Up @@ -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]
59 changes: 59 additions & 0 deletions tests/unit/services/orchestrators/test_query_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,33 @@ async def test_generate_query_chatbotrag_falls_back_on_garbage():
assert sq.query_list[0].query == "raw question" # fallback to raw user query


@pytest.mark.asyncio
async def test_generate_query_renders_history_with_a_content_free_turn():
"""The chat-history string is built over *every* message, so the assistant
turn that carries ``tool_calls`` — no ``content`` at all once the router
dumps with ``exclude_none=True`` — reaches it too. ChatBotRag is the default
``rag.mode``, so the SimpleRag early return is no shield: reading the key
unguarded 500s on a plain tool-call replay.
"""
llm = FakeLLM(chat_responses=['{"requires_retrieval": true, "query_list": [{"query": "q"}]}'])
svc = _svc(llm=llm, mode="ChatBotRag")

sq = await svc.generate_query(
[
{"role": "user", "content": "weather in Paris?"},
{"role": "assistant", "tool_calls": [{"id": "c1"}]},
{"role": "tool", "content": "18C", "tool_call_id": "c1"},
{"role": "user", "content": "and tomorrow?"},
]
)

assert sq.query_list[0].query == "q"
# The turn is still rendered (role kept, empty body) rather than skipped, so
# the history handed to the contextualizer keeps its shape.
history = llm.chat_calls[0][0][1]["content"]
assert "assistant: \n" in history


# --------------------------------------------------------------------------- #
# chat / complete
# --------------------------------------------------------------------------- #
Expand Down Expand Up @@ -1675,3 +1702,35 @@ async def resolve_prompt(self, prompt_type, names=None):
assert docs == [] and web == []
assert out["messages"][0]["role"] == "system"
assert "CONVERSATIONAL" in out["messages"][0]["content"]


def test_split_leading_system_prompt_tolerates_content_free_system_turn():
"""``OpenAIMessage`` allows a null content and the router dumps with
``exclude_none=True``, so a leading system message can reach here with no
``content`` key at all. Reading it unguarded raised KeyError — a 500 on a
request the schema had just accepted.
"""
raw = [
{"role": "system"},
{"role": "system", "content": "be terse"},
{"role": "user", "content": "hi"},
]

pinned, rest = qs._split_leading_system_prompt(raw, raw)

# The content-free turn is still consumed by the leading run (stripped from
# the history) — it just contributes nothing to the pinned prompt.
assert pinned == "be terse"
assert rest == [{"role": "user", "content": "hi"}]


def test_split_leading_system_prompt_returns_none_when_every_system_turn_is_empty():
"""A leading run made only of content-free system messages pins nothing,
rather than joining empty strings into a blank custom prompt.
"""
raw = [{"role": "system"}, {"role": "system", "content": ""}, {"role": "user", "content": "hi"}]

pinned, rest = qs._split_leading_system_prompt(raw, raw)

assert pinned is None
assert rest == [{"role": "user", "content": "hi"}]
Loading