fix: improve OpenAI message content compatibility - #682
Conversation
📝 WalkthroughWalkthroughThis PR adds streaming support to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mcp_agent/workflows/llm/augmented_llm_openai.py (1)
598-619: 💤 Low value
generate_streamfallback implementation is correct, but onlyTEXT_DELTAandCOMPLETEevents are emitted.Tool calls are fully executed inside
generate()before this method runs, so callers listening forTOOL_USE_START/TOOL_USE_END/TOOL_RESULT/ITERATION_*events will receive nothing. Since this is an intentional non-streaming fallback (the docstring calls this out), consider adding a comment or TODO to signal the gap to future implementors of true streaming.💡 Suggested TODO
async def generate_stream( self, message: MessageTypes, request_params: RequestParams | None = None, ) -> AsyncIterator[StreamEvent]: """ Emit stream events using the non-streaming OpenAI generation path. OpenAI-native token streaming is not implemented here yet, but this keeps the provider concrete and gives callers a consistent event interface. + + Note: Only TEXT_DELTA and COMPLETE events are emitted. Tool-use and + iteration events (TOOL_USE_START, TOOL_USE_END, TOOL_RESULT, ITERATION_*) + are not emitted because generate() executes tool calls synchronously. + # TODO: Replace with true streaming using client.chat.completions.create(stream=True) """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_agent/workflows/llm/augmented_llm_openai.py` around lines 598 - 619, The generate_stream fallback in augmented_llm_openai.py (function generate_stream) only emits TEXT_DELTA and COMPLETE events and therefore does not emit TOOL_USE_START/TOOL_USE_END/TOOL_RESULT or ITERATION_* events because tool calls are fully executed inside generate(); add a clear TODO/comment at the top of generate_stream explaining this limitation (that this is a non-streaming fallback and tool/iteration events are not emitted) and reference that true streaming should implement OpenAI-native token streaming to emit those events, so future implementors know where to extend behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/mcp_agent/workflows/llm/augmented_llm_openai.py`:
- Around line 598-619: The generate_stream fallback in augmented_llm_openai.py
(function generate_stream) only emits TEXT_DELTA and COMPLETE events and
therefore does not emit TOOL_USE_START/TOOL_USE_END/TOOL_RESULT or ITERATION_*
events because tool calls are fully executed inside generate(); add a clear
TODO/comment at the top of generate_stream explaining this limitation (that this
is a non-streaming fallback and tool/iteration events are not emitted) and
reference that true streaming should implement OpenAI-native token streaming to
emit those events, so future implementors know where to extend behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4aa03b3a-3ab8-48d4-8437-1ae41ef57551
📒 Files selected for processing (2)
src/mcp_agent/workflows/llm/augmented_llm_openai.pytests/workflows/llm/test_augmented_llm_openai.py
Summary
generate_streamimplementation forOpenAIAugmentedLLM, so the provider remains instantiable after streaming became an abstract LLM APIMCPOpenAITypeConverterNotes
Refs #25, especially the
from_mcp_message_param()follow-up reported in the issue thread. I intentionally left the tool-resultexecute_tool_call()path alone because #680 and #651 already cover that primary fix.Testing
uv run --extra openai --group dev pytest tests/workflows/llm/test_augmented_llm_openai.py -quv run --extra openai --group dev ruff check src/mcp_agent/workflows/llm/augmented_llm_openai.py tests/workflows/llm/test_augmented_llm_openai.pyuv run --extra openai --group dev ruff format --check src/mcp_agent/workflows/llm/augmented_llm_openai.py tests/workflows/llm/test_augmented_llm_openai.pygit diff --checkSummary by CodeRabbit
Release Notes
New Features
Improvements