fix(api): forward vendor fields on chat messages and completions - #833
fix(api): forward vendor fields on chat messages and completions#833paultranvan wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughChangesOpenAI compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change forwards vendor-specific fields while preserving existing validation behavior, with unit-test coverage for the affected paths; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/api/schemas/user/chat.py`:
- Around line 7-13: Update the chat router’s request logging to avoid
serializing full request.messages now that UserMessage allows extras via
model_config. In the logging path using truncate(str(request.messages)), log
only safe message metadata or explicitly redact extra fields, while preserving
passthrough of fields needed by downstream LLM processing.
- Around line 7-15: Update the message schema’s role declaration in the model
containing model_config to accept "tool" alongside the existing roles, allowing
tool-result messages with tool_call_id to validate and pass through. Add a
regression test covering a role="tool" message that includes tool_call_id.
In `@tests/unit/api/schemas/test_api_schema_imports.py`:
- Around line 117-137: Extend
test_chat_message_passes_through_extra_openai_fields to assert preservation of
tool_calls[0].function.name and function.arguments, plus tool_call_id and
function_call using the corresponding message fixtures. Keep the existing name
and tool-call ID assertions, and ensure nested OpenAI fields are verified after
model_dump(exclude_none=True).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 939bd10a-b875-4e34-bfa5-f0515a6e396b
📒 Files selected for processing (2)
openrag/api/schemas/user/chat.pytests/unit/api/schemas/test_api_schema_imports.py
95b9993 to
fc2d87c
Compare
fc2d87c to
85e3757
Compare
| 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") |
There was a problem hiding this comment.
Same defensive-read pattern is missing at query_service.py:363 (generate_query):
chat_history = "".join(f"{m['role']}: {m['content']}\n" for m in messages)m['content'] is unguarded there. Since content can now be absent (assistant tool_calls turn, dropped by exclude_none=True), and default rag.mode is ChatBotRag (not SimpleRag), a tool-call-replay history 500s with KeyError: 'content'. Verified against this branch's actual generate_query. Suggest m.get('content') or '' there too.
85e3757 to
7e6349c
Compare
7e6349c to
c6a706e
Compare
2 participants
Ported from a production hotfix: OpenRag was silently dropping OpenAI fields it
did not declare, so they never reached the downstream LLM.
OpenAIMessageAn OpenAI message is more than
role/content—name,tool_calls,function_call,tool_call_id. Pydantic's defaultextra="ignore"dropped allof them at parse time, before the router dumped the payload, so the history
forwarded to the model was silently truncated.
OpenAIChatCompletionRequestalready carriesextra="allow"with the comment"Accept and forward vendor-specific OpenAI params" — this applies the same policy
one level down, where the vendor fields actually live.
Concretely,
QueryService._sanitize_messagesalready branches onmsg.get("tool_calls")/msg.get("function_call")to leave a content-freeassistant turn alone. Those branches were unreachable: the schema stripped the
fields before the sanitizer ever saw them.
OpenAICompletionRequestSame passthrough on the legacy
/completionsendpoint, for consistency.extra="allow"only admits undeclared keys, so the deliberaten/best_ofbounds (resource-exhaustion guard) still validate — covered by a test.
Tests
Three unit tests in
tests/unit/api/schemas/test_api_schema_imports.py:name,tool_calls) survivemodel_dump_sanitize_messagespreserves atool_callsassistant turn end-to-end/completionsforwardssuffix/userwhile still rejectingn=9Full unit suite: no new failures (2204 → 2207 passing).
Summary by CodeRabbit
New Features
toolanddevelopermessage roles.Bug Fixes