Python: Add response/request customization hooks to OpenAIChatCompletionClient - #7028
Conversation
Fix two issues with reasoning content handling in the Chat Completions client: 1. (microsoft#6979) reasoning_details plaintext buried as encrypted data: The client dumped the entire reasoning_details array into Content.protected_data without setting Content.text, causing AG-UI to emit ReasoningEncryptedValueEvent instead of visible ReasoningMessageContentEvent for plaintext reasoning providers (e.g. OpenRouter). Now extracts readable text from reasoning_details entries into Content.text while preserving protected_data for round-trip fidelity. 2. (microsoft#6978) Mistral list content causes crash: Mistral reasoning models return content as a list of typed chunks ([{"type": "thinking", ...}, {"type": "text", ...}]) instead of a plain string. _parse_text_from_openai assumed content was always a string, causing a Pydantic ValidationError downstream. Now detects list content and parses thinking chunks as Content.from_text_reasoning and text chunks as Content.from_text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR fixes parsing of “reasoning” outputs in the Python OpenAIChatCompletionClient so downstream consumers (notably AG-UI) can display plaintext reasoning correctly and avoid crashes when providers return structured chunked content.
Changes:
- Add
_extract_reasoning_text()and use it to populateContent.textfor plaintextreasoning_detailswhile preserving full payload round-tripped inprotected_data. - Refactor
_parse_text_from_openai()to returnlist[Content]and introduce_parse_chunked_content()to handle Mistral-stylecontent: [...]chunks (thinking+text). - Add regression tests covering plaintext
reasoning_detailsextraction and chunked list-content parsing in both streaming and non-streaming paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/packages/openai/agent_framework_openai/_chat_completion_client.py | Adds plaintext reasoning extraction + chunked content parsing; updates response parsing to emit correct Content items. |
| python/packages/openai/tests/openai/test_openai_chat_completion_client.py | Adds tests for plaintext reasoning extraction and Mistral chunked content in both streaming and non-streaming modes. |
- Use cast() for proper type narrowing in _extract_reasoning_text and
_parse_chunked_content to satisfy pyright strict mode
- Handle {"content": "..."} string shape in _extract_reasoning_text
(addresses review comment about missing format coverage)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
model_construct bypasses Pydantic runtime validation but mypy still checks declared types. Use cast(Any, ...) for the list content args. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 'summary' field extraction in _extract_reasoning_text for
reasoning.summary entries from OpenRouter
- Handle message.reasoning and message.reasoning_content top-level
fields (plaintext reasoning without reasoning_details) in both
streaming and non-streaming paths
- reasoning_details takes priority when both fields are present
- Preserve original Mistral chunk list in additional_properties
('_source_content_list') so _prepare_message_for_openai can
reconstruct the structured list content for multi-turn reasoning
- Add 5 new tests covering all new behaviors
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove leading underscore from _skip_structured_siblings variable since it is accessed (not a dummy variable). Ruff's used-dummy-variable rule flags variables with leading underscores that are read. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 76%
✓ Correctness
The PR correctly fixes both issues (#6978 and #6979) with well-structured parsing logic and thorough tests. The
_extract_reasoning_text()helper handles documented provider formats (includingreasoning.summaryentries), the fallback tomessage.reasoning/reasoning_contentfields is properly gated behind anelif, and the Mistral round-trip via_source_content_listis tested end-to-end. I found no high-severity correctness issues.
✓ Security Reliability
The PR is well-structured with proper defensive type checks and graceful handling of untrusted provider response shapes. The
_extract_reasoning_texthelper safely handles all expected formats. The_parse_chunked_contentmethod correctly validates chunk types before processing. The main concern is a minor reliability edge case in theskip_structured_siblingsmechanism in_prepare_message_for_openai, which could silently drop unrelated text/reasoning content if it follows Mistral-style chunked content in the same Message. However, given the current parsing architecture, this situation does not arise in practice. No critical security or reliability issues found.
✓ Test Coverage
The PR adds 11 new tests covering the core happy paths for reasoning text extraction, Mistral chunked content, streaming, and round-trip serialization. The tests are well-structured with meaningful assertions. However, there is a notable gap: no test verifies that opaque/encrypted reasoning_details (entries without 'text' or 'summary' fields) correctly yield Content.text=None, which is the key invariant distinguishing genuinely encrypted data from displayable reasoning. The existing test
test_parse_text_reasoning_content_from_response(line 787) tests this shape but was written before the extraction logic existed and doesn't assert thetextfield value — it now silently extracts 'summary' text where it previously had None.
✓ Failure Modes
The PR correctly fixes the two reported issues (reasoning plaintext extraction and Mistral list-content parsing). The
skip_structured_siblingsmechanism in_prepare_message_for_openaiuses a type-based heuristic that can silently drop non-siblingtext_reasoningcontent (e.g., fromreasoning_details) if it follows a Mistral chunked source without intervening tool-call items. This is unlikely with current providers but represents a latent silent-failure path. No other significant failure modes were found.
✓ Design Approach
The new reasoning/chunked-content handling fixes the reported cases, but the round-trip design still has two silent data-loss edges in
_prepare_message_for_openai(): it only recognizes_source_content_listwhen the marked content istext_reasoning, and its skip flag suppresses every later text/text_reasoning item in the same message instead of only the siblings from that structured list.
Automated review by giles17's agents
…ho-back - Honor the _source_content_list marker regardless of the first emitted content's type by handling it before the type match, so a chunk list beginning with a text chunk still round-trips as one structured message (addresses github-actions review comment on results[0]). - Tag every chunked-content item with a shared _structured_content_group id and skip only exact group siblings during serialization, instead of suppressing all later text/reasoning content. - Record provenance of top-level reasoning/reasoning_content fields in _reasoning_source_field and echo the value back under the same key on the next request, which providers such as vLLM require (addresses Kimahriman review comment). Replaces the prior behavior that replayed surfaced reasoning as visible answer text. - Factor the duplicated reasoning parsing into _parse_reasoning_content. - Add tests for provenance capture, reasoning/reasoning_content round-trip, reasoning-only messages, text-first chunk round-trip, and unrelated sibling preservation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f3c0308-51bf-4b66-8b53-87a8546743f5
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
Confident Giles will followup appropriately
…rsing' into fix/chat-completion-reasoning-parsing
GitHub Actions
Inactive
…pare hooks Following review feedback (microsoft#7028), keep OpenAIChatCompletionClient free of provider-specific quirks for 'almost OpenAI-compatible' endpoints. Instead of branching in core for OpenRouter/vLLM/Mistral, expose two optional callables so callers adapt the client themselves: - response_parser (OpenAIChatResponseContentsParser): post-processes the Content list parsed from each response choice/streaming delta, to surface non-standard fields (e.g. reasoning/reasoning_content/reasoning_details) for display. - message_preparer (OpenAIChatMessagePreparer): post-processes the outgoing request message dicts built from each framework Message, to echo provider-specific fields back on later turns (e.g. vLLM reasoning) for multi-turn continuity. Both default to None (no-op; byte-identical stock OpenAI behavior). This reverts the provider-specific reasoning/chunked-content parsing and round-trip markers previously added to core; Mistral chunked content is now handled by agent-framework-mistral. - Add the two callables to RawOpenAIChatCompletionClient / OpenAIChatCompletionClient constructors and invoke them at the parse and prepare seams. - Export the type aliases from the package and the core lazy openai namespace (+ .pyi). - Replace the removed-behavior tests with tests for the two hooks. - Document the hooks in packages/openai/AGENTS.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f3c0308-51bf-4b66-8b53-87a8546743f5
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
documentation
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 50%
✓ Correctness
The Mistral crash remains because the new hook runs only after default content parsing.
✓ Security Reliability
This revision replaces the provider-specific parsing branches with two optional, opt-in extension points (
response_parser,message_preparer) that default toNone, so stock OpenAI behavior is unchanged and no new parsing of untrusted provider payloads is added in core. From a security/reliability standpoint the diff introduces no new deserialization, secret handling, subprocess, or network surface: the hooks are caller-supplied callables invoked on data the caller already owns, andjson.dumps(reasoning_details)behavior is unchanged from before. IMPORTANT CAVEAT: my file-read and shell tools failed with permission errors for every attempt in this session, so I could not perform the repository context pass. Per evidence discipline I am therefore emitting no inline findings, since I could not open_chat_completion_client.py,Content, or the surrounding streaming aggregation code to confirm or refute any suspicion. Treat this as a diff-only, non-blocking review rather than a verified pass.
✓ Failure Modes
I was unable to complete the required context pass: every read tool (bash, grep, glob, view) in this session returned 'unexpected user permission response', so I could not open any repository file to verify constructor forwarding, hook invocation coverage, or serialization behavior. Reviewing the diff alone, the change is a narrow, opt-in extension: two
Callabletype aliases are added, stored on the client, and invoked only when non-None (_chat_completion_client.py:861,:908,:1147). Default behavior with no hooks is unchanged, andtest_no_hooks_keeps_default_behaviorpins the stockreasoning_detailspath. From a failure-modes standpoint the hooks propagate exceptions rather than swallowing them, there is no retry/partial-write/cleanup surface introduced, and the streaming path was correctly restructured to scope per-choice contents before extending the aggregate list. I found no concrete failure path I can support with verified file:line evidence, and per evidence discipline I am reporting no findings rather than speculating from unread files.
✓ Design Approach
I was unable to complete the required context pass: every tool invocation in this session (view, grep, bash) was denied, so I could not open
_chat_completion_client.py, its helpers, or the surrounding tests to verify any hypothesis against source. Per the evidence discipline for design_approach findings, I am emitting zero findings rather than speculating from the diff alone. One observation that is fully supported by the diff text itself, without needing file reads: the <pr_rationale> block describes an implementation (_extract_reasoning_text(), a refactored_parse_text_from_openai()returninglist[Content], a_parse_chunked_content()static method, and five named tests such astest_parse_mistral_chunked_content_from_response) that does not appear anywhere in this diff. The diff instead adds two generic extension points (OpenAIChatResponseContentsParser/OpenAIChatMessagePreparer) wired into_parse_response_from_openai,_parse_response_update_from_openai, and_prepare_message_for_openai, with a different test set — which matches the unresolved review feedback at line 127 asking for a user-supplied callable instead of provider branches. The PR body appears stale relative to the current head and should be refreshed so reviewers and future readers can tell how (or whether) #6978 and #6979 are still addressed by the hook-based design.
Automated review by giles17's agents
Structured list content (e.g. Mistral reasoning models returning content as a list of chunks) was wrapped verbatim into a text Content, producing a malformed Content whose text is a list that crashes downstream (issue microsoft#6978). Default text parsing now skips non-string content so a configured response_parser receives a clean slate to expand it. Applies to both streaming and non-streaming paths. Add tests for the skip and for a response_parser expanding chunked content. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f3c0308-51bf-4b66-8b53-87a8546743f5
GitHub Actions
Inactive
Giles Odigwe (giles17)
changed the title
GitHub Actions
Inactive
- response_parser now receives the already-selected ChatCompletionMessage / ChoiceDelta instead of Choice | ChunkChoice, so callers no longer duplicate the streaming dispatch (removes the Any/hasattr pattern from tests). The client owns the dispatch; parsers read provider fields directly. - message_preparer now runs once per Message for every role: the build logic moved to _build_openai_messages and the hook is applied at a single exit point in _prepare_message_for_openai, so system/developer messages no longer bypass it. - Round-trip example/test now correlates surfaced reasoning via an additional_properties marker on message.contents with bounded, order-aware, one-to-one dict removal, instead of fragile request-string matching. Adds a test proving an answer whose text equals the reasoning text is no longer dropped. - Update packages/openai/AGENTS.md for the new parser signature and guidance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f3c0308-51bf-4b66-8b53-87a8546743f5
GitHub Actions
Inactive
GitHub Actions
Inactive
Motivation & Context
OpenAIChatCompletionClienttargets the OpenAI Chat Completions wire format, but it is widelypointed at "almost OpenAI-compatible" endpoints (OpenRouter, vLLM, Mistral, DeepSeek, Ollama, …)
that diverge on the edges — most visibly in how they represent reasoning (
reasoning,reasoning_content,reasoning_details) and, for some Mistral models, by returningcontentas alist of typed chunks instead of a string. Branching in the stock client for each provider quirk does
not scale and puts the client on the critical path of every third-party implementation.
Following reviewer feedback, this keeps the stock client provider-agnostic and instead exposes two
optional callables so callers can adapt parsing and request-building to their specific endpoint
without subclassing. It also hardens default parsing so structured (non-string) content can no longer
produce a malformed
Contentthat crashes downstream.Description & Review Guide
What are the major changes?
RawOpenAIChatCompletionClient/OpenAIChatCompletionClient:response_parser(OpenAIChatResponseContentsParser):(choice, default_contents) -> contents.Post-processes the
Contentlist parsed from each response choice / streaming delta, e.g. tosurface a provider's non-standard reasoning field for display.
message_preparer(OpenAIChatMessagePreparer):(message, default_dicts) -> dicts.Post-processes the outgoing request message dicts built from each framework
Message, e.g. toecho a provider's reasoning field back on later turns (required by vLLM-style endpoints for
multi-turn continuity).
None— a no-op, so stock OpenAI/Azure behavior is unchanged.contentinstead of wrapping it verbatim into amalformed
textContent; this prevents a downstream crash and gives aresponse_parsera cleanslate to expand structured chunks.
agent_framework.openainamespace (and its.pyi).What is the impact of these changes?
None.hooks rather than in-core branches; Mistral chunked content is handled by the dedicated
agent-framework-mistralclient.What do you want reviewers to focus on?
contentguard in_parse_text_from_openai.Related Issue
Related to #6978 and #6979. Rather than fix these with provider-specific branches in the stock client,
this adds the supported extension path to resolve them: a
response_parsersurfaces plaintextreasoning (#6979) and a
message_preparerechoes it back for multi-turn continuity, while thenon-string-content guard removes the #6978 crash and the dedicated
agent-framework-mistralclienthandles Mistral chunked content.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.