Python: fix: reject invalid chat completion payloads#6290
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds defensive validation around parsing OpenAI Chat Completions responses and introduces a focused regression test to ensure invalid/primitive responses produce a clear ChatClientException rather than an attribute error.
Changes:
- Add runtime validation in
_parse_response_from_openaito reject responses missingchoiceswith a clearer exception. - Include a new unit test covering the invalid-response path and asserting helpful error content.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| python/packages/openai/agent_framework_openai/_chat_completion_client.py | Adds early validation and improved error message when the response object isn’t a Chat Completions-like object. |
| python/packages/openai/tests/openai/test_openai_chat_completion_client.py | Adds a regression test ensuring invalid responses raise ChatClientException with a helpful message. |
| def _parse_response_from_openai(self, response: ChatCompletion, options: Mapping[str, Any]) -> ChatResponse: | ||
| """Parse a response from OpenAI into a ChatResponse.""" | ||
| if not hasattr(response, "choices"): |
| preview = repr(response) | ||
| if len(preview) > 500: | ||
| preview = f"{preview[:497]}..." |
| raise ChatClientException( | ||
| maybe_append_azure_endpoint_guidance( | ||
| f"{type(self)} service returned an invalid OpenAI Chat Completions API response: " | ||
| f"expected an object with 'choices', got {type(response).__name__}: {preview}", | ||
| azure_endpoint=self.azure_endpoint, | ||
| ) | ||
| ) |
| exception_message = str(exc_info.value) | ||
| assert "invalid OpenAI Chat Completions API response" in exception_message | ||
| assert "expected an object with 'choices'" in exception_message | ||
| assert "got str" in exception_message | ||
| assert "plain backend error" in exception_message | ||
| assert "'str' object has no attribute 'system_fingerprint'" not in exception_message |
|
Thank you for your contribution, @he-yufeng. To keep the review queue manageable, we currently limit community contributors to 10 open pull requests at a time. This PR would put you at 20 open pull requests, so we are closing it automatically. Please focus on getting your existing PRs reviewed, merged, or closed before opening another one. If a maintainer asked you to open this PR, they can apply the |
Summary
response.system_fingerprintChatClientExceptionwith the unexpected payload type and a bounded previewFixes #6234.
To verify
uv run --group dev pytest packages/openai/tests/openai/test_openai_chat_completion_client.py::test_parse_response_rejects_non_chat_completion_object -quv run --group dev pytest packages/openai/tests/openai/test_openai_chat_completion_client.py::test_parse_response_rejects_non_chat_completion_object packages/openai/tests/openai/test_openai_chat_completion_client.py::test_parse_response_with_dict_response_format -quv run ruff check packages/openai/agent_framework_openai/_chat_completion_client.py packages/openai/tests/openai/test_openai_chat_completion_client.pyuv run python -m py_compile packages/openai/agent_framework_openai/_chat_completion_client.py packages/openai/tests/openai/test_openai_chat_completion_client.pyuv run pyright packages/openai/agent_framework_openai/_chat_completion_client.pygit diff --check