Conversation
Fixes mudler#7772 The issue was caused by duplicate registration of the MCP endpoint /mcp/v1/chat/completions in both openai.go and localai.go, leading to a race condition where requests would randomly hit different handlers with incompatible behaviors. Changes: - Removed duplicate MCP route registration from openai.go - Kept the localai.MCPStreamEndpoint as the canonical handler - Added all three MCP route patterns for backward compatibility: * /v1/mcp/chat/completions * /mcp/v1/chat/completions * /mcp/chat/completions - Added comments to clarify route ownership and prevent future conflicts - Fixed formatting in ui_api.go The localai.MCPStreamEndpoint handler is more feature-complete as it supports both streaming and non-streaming modes, while the removed openai.MCPCompletionEndpoint only supported synchronous requests. This eliminates the ~50% failure rate where the cogito library would receive "Invalid http method" errors when internal HTTP requests were routed to the wrong handler. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: majiayu000 <1835304752@qq.com>
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback by consolidating and refactoring MCP (Model Context Protocol) endpoint implementations. The main changes involve removing duplicate MCP endpoints from the OpenAI routes and consolidating functionality into a single LocalAI endpoint that supports both streaming and non-streaming modes.
- Removed duplicate MCP completion handler from OpenAI routes
- Renamed and updated MCP endpoint to support both streaming and non-streaming modes
- Added route registration for
/mcp/chat/completionspath - Fixed indentation formatting issue in UI API routes
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/http/routes/ui_api.go | Fixed indentation of closing brace in error response |
| core/http/routes/openai.go | Removed MCP completion handler and related middleware from OpenAI routes |
| core/http/routes/localai.go | Updated comments, renamed handler function, and added additional route registration for MCP endpoint |
| core/http/endpoints/openai/mcp.go | Deleted entire file containing duplicate MCP completion implementation |
| core/http/endpoints/localai/mcp.go | Renamed function from MCPStreamEndpoint to MCPEndpoint and updated documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // MCP Stream endpoint | ||
| // MCP endpoint - supports both streaming and non-streaming modes | ||
| // Note: streaming mode is NOT compatible with the OpenAI apis. We have a set which streams more states. |
There was a problem hiding this comment.
The comment states "streaming mode is NOT compatible with the OpenAI apis", but it should specify "OpenAI API" (singular and capitalized) for consistency with technical documentation standards.
| // Note: streaming mode is NOT compatible with the OpenAI apis. We have a set which streams more states. | |
| // Note: streaming mode is NOT compatible with the OpenAI API. We have a set which streams more states. |
|
|
||
| // MCP Stream endpoint | ||
| // MCP endpoint - supports both streaming and non-streaming modes | ||
| // Note: streaming mode is NOT compatible with the OpenAI apis. We have a set which streams more states. |
There was a problem hiding this comment.
The comment states "We have a set which streams more states" which is unclear. Consider rewording to "This endpoint streams additional state information beyond what the standard OpenAI API provides" or similar phrasing to clarify what "a set which streams more states" means.
| // Note: streaming mode is NOT compatible with the OpenAI apis. We have a set which streams more states. | |
| // Note: streaming mode is NOT compatible with the OpenAI APIs. This endpoint streams additional state information beyond what the standard OpenAI API provides. |
| } | ||
|
|
||
| // MCPStreamEndpoint is the SSE streaming endpoint for MCP chat completions | ||
| // MCPEndpoint is the endpoint for MCP chat completions. Supports SSE mode, but it is not compatible with the OpenAI apis. |
There was a problem hiding this comment.
The comment states "SSE mode, but it is not compatible with the OpenAI apis". The phrase should use "OpenAI API" (singular and capitalized) for consistency with technical documentation standards.
3 participants
Description
This PR fixes #
Notes for Reviewers
Signed commits